|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 7/8] xen-blkback: frontend feature control
On 02/07/2018 12:08 PM, Roger Pau Monné wrote:
> On Thu, Nov 02, 2017 at 06:06:15PM +0000, Joao Martins wrote:
>> Toolstack may write values to the "require" subdirectory in the
>> backend main directory (e.g. backend/vbd/X/Y/). Read these values
>> and use them when announcing those to the frontend. When backend
>> scans frontend features the values set in the require directory
>> take precedence, hence making no significant changes in feature
>> parsing.
>>
>> xenbus_read_feature() reads from require subdirectory and prints that
>> value and otherwise writing a default_val in the entry. We then replace
>> all instances of xenbus_printf to use these previously seeded features.
>> A backend_features struct is introduced and all values set there are
>> used in place of the module parameters being used.
>>
>> Note, however that feature-barrier, feature-flush-support and
>> feature-discard aren't probed because first two are physical
>> device dependent and feature-discard already has tunables to
>> adjust.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> ---
>> drivers/block/xen-blkback/blkback.c | 2 +-
>> drivers/block/xen-blkback/common.h | 1 +
>> drivers/block/xen-blkback/xenbus.c | 66
>> ++++++++++++++++++++++++++++++++-----
>> 3 files changed, 60 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/blkback.c
>> b/drivers/block/xen-blkback/blkback.c
>> index c90e933330b6..05b3f124c871 100644
>> --- a/drivers/block/xen-blkback/blkback.c
>> +++ b/drivers/block/xen-blkback/blkback.c
>> @@ -1271,7 +1271,7 @@ static int dispatch_rw_block_io(struct xen_blkif_ring
>> *ring,
>> unlikely((req->operation != BLKIF_OP_INDIRECT) &&
>> (nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) ||
>> unlikely((req->operation == BLKIF_OP_INDIRECT) &&
>> - (nseg > MAX_INDIRECT_SEGMENTS))) {
>> + (nseg > ring->blkif->vbd.max_indirect_segs))) {
>> pr_debug("Bad number of segments in request (%d)\n", nseg);
>> /* Haven't submitted any bio's yet. */
>> goto fail_response;
>> diff --git a/drivers/block/xen-blkback/common.h
>> b/drivers/block/xen-blkback/common.h
>> index a7832428e0da..ff12f2d883b9 100644
>> --- a/drivers/block/xen-blkback/common.h
>> +++ b/drivers/block/xen-blkback/common.h
>> @@ -229,6 +229,7 @@ struct xen_vbd {
>> unsigned int discard_secure:1;
>> unsigned int feature_gnt_persistent:1;
>> unsigned int overflow_max_grants:1;
>> + unsigned int max_indirect_segs;
>
> Please place this above, in order to get less padding between fields.
>
/nods
>> };
>>
>> struct backend_info;
>> diff --git a/drivers/block/xen-blkback/xenbus.c
>> b/drivers/block/xen-blkback/xenbus.c
>> index 48d796ea3626..31683f29d5fb 100644
>> --- a/drivers/block/xen-blkback/xenbus.c
>> +++ b/drivers/block/xen-blkback/xenbus.c
>> @@ -25,11 +25,19 @@
>>
>> /* On the XenBus the max length of 'ring-ref%u'. */
>> #define RINGREF_NAME_LEN (20)
>> +#define REQUIRE_PATH_LEN (256)
>> +
>> +struct backend_features {
>> + unsigned max_queues;
>> + unsigned max_ring_order;
>
> unsigned int.
>
>> + unsigned pers_grants;
>
> bool?
>
> Since you are already doing this, is it worth adding support to
> disable other features like flush or discard?
>
Hmm, possibly. But I didn't really know if you folks wanted discard because it
already has a tunable? I guess probably yes given libxl is stateless, it could
be good for behaviour consistency. flush/barrier depended on whether the queue
had it enabled or not so I left it out thinking there was some other way to
mangle these features.
>> +};
>>
>> struct backend_info {
>> struct xenbus_device *dev;
>> struct xen_blkif *blkif;
>> struct xenbus_watch backend_watch;
>> + struct backend_features features;
>> unsigned major;
>> unsigned minor;
>> char *mode;
>> @@ -602,6 +610,40 @@ int xen_blkbk_barrier(struct xenbus_transaction xbt,
>> return err;
>> }
>>
>> +static int xenbus_read_feature(const char *dir, const char *node,
>> + unsigned int default_val)
>> +{
>> + char reqnode[REQUIRE_PATH_LEN];
>> + unsigned int val;
>> +
>> + snprintf(reqnode, REQUIRE_PATH_LEN, "%s/require", dir);
>> + val = xenbus_read_unsigned(reqnode, node, default_val);
>> + return val;
>> +}
>
> You could avoid the temporary buffer by doing something like:
>
>> +
>> +static void xen_blkbk_probe_features(struct xenbus_device *dev,
>> + struct backend_info *be)
>> +{
>> + struct backend_features *ft = &be->features;
>> + struct xen_vbd *vbd = &be->blkif->vbd;
>> +
>
> #define READ_FEAT(path, feat, default) \
> xenbus_read_unsigned(path, "require/" node, default)
>
>> + vbd->max_indirect_segs = xenbus_read_feature(dev->nodename,
>> + "feature-max-indirect-segments",
>> + MAX_INDIRECT_SEGMENTS);
>> +
>> + ft->max_queues = xenbus_read_feature(dev->nodename,
>> + "multi-queue-max-queues",
>> + xenblk_max_queues);
>> +
>> + ft->max_ring_order = xenbus_read_feature(dev->nodename,
>> + "max-ring-page-order",
>> + xen_blkif_max_ring_order);
>> +
>> + ft->pers_grants = xenbus_read_feature(dev->nodename,
>> + "feature-persistent",
>> + 1);
> #undef READ_FEAT
>
> Aren't you missing some check here or elsewhere to make sure the
> proposed values don't exceed the maximum ones supported by blback?
>
> I would expect something like:
>
> vbd->max_indirect_segs = min(vbd->max_indirect_segs, MAX_INDIRECT_SEGMENTS);
>
> And the same with max_queues and max_ring_oder.
>
This was deliberate to some extent. How do we define the value of max_queues?
Relying on the module parameters seems wrong as those *also* represent default
values for all guests. So I wouldn't cap to xen_blkbk_max_queues because it
would be left out to toolstack to pick. indirect_segs and max_ring_order there
are actual maximum values defined in macros so those definitely make sense to
check/validate.
> It might also be a good idea to print some message when the proposed
> value by the toolstack is not supported by the backend.
Yeap, I agree.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |