Re: [PATCH RFC 7/8] xen-blkback: frontend feature control

[Joao Martins <joao.m.martins@oracle.com>]

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@oracle.com>
>> ---
>>  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel