Re: [PATCH] xen, blkback: fix persistent grants negotiation

["Durrant, Paul" <xadimgnik@gmail.com>]

On 11/01/2022 11:11, Roger Pau Monné wrote:
> On Thu, Jan 06, 2022 at 09:10:13AM +0000, Maximilian Heyne wrote:
>> Given dom0 supports persistent grants but the guest does not.
>> Then, when attaching a block device during runtime of the guest, dom0
>> will enable persistent grants for this newly attached block device:
>>
>>    $ xenstore-ls -f | grep 20674 | grep persistent
>>    /local/domain/0/backend/vbd/20674/768/feature-persistent = "0"
>>    /local/domain/0/backend/vbd/20674/51792/feature-persistent = "1"
> 
> The mechanism that we use to advertise persistent grants support is
> wrong. 'feature-persistent' should always be set if the backend
> supports persistent grant (like it's done for other features in
> xen_blkbk_probe). The usage of the feature depends on whether both
> parties support persistent grants, and the xenstore entry printed by
> blkback shouldn't reflect whether persistent grants are in use, but
> rather whether blkback supports the feature.
> 
>>
>> Here disk 768 was attached during guest creation while 51792 was
>> attached at runtime. If the guest would have advertised the persistent
>> grant feature, there would be a xenstore entry like:
>>
>>    /local/domain/20674/device/vbd/51792/feature-persistent = "1"
>>
>> Persistent grants are also used when the guest tries to access the disk
>> which can be seen when enabling log stats:
>>
>>    $ echo 1 > /sys/module/xen_blkback/parameters/log_stats
>>    $ dmesg
>>    xen-blkback: (20674.xvdf-0): oo   0  |  rd    0  |  wr    0  |  f    0 |  ds    0 | pg:    1/1056
>>
>> The "pg: 1/1056" shows that one persistent grant is used.
>>
>> Before commit aac8a70db24b ("xen-blkback: add a parameter for disabling
>> of persistent grants") vbd->feature_gnt_persistent was set in
>> connect_ring. After the commit it was intended to be initialized in
>> xen_vbd_create and then set according to the guest feature availability
>> in connect_ring. However, with a running guest, connect_ring might be
>> called before xen_vbd_create and vbd->feature_gnt_persistent will be
>> incorrectly initialized. xen_vbd_create will overwrite it with the value
>> of feature_persistent regardless whether the guest actually supports
>> persistent grants.
>>
>> With this commit, vbd->feature_gnt_persistent is set only in
>> connect_ring and this is the only use of the module parameter
>> feature_persistent. This avoids races when the module parameter changes
>> during the block attachment process.
>>
>> Note that vbd->feature_gnt_persistent doesn't need to be initialized in
>> xen_vbd_create. It's next use is in connect which can only be called
>> once connect_ring has initialized the rings. xen_update_blkif_status is
>> checking for this.
>>
>> Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
>> Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
>> ---
>>   drivers/block/xen-blkback/xenbus.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
>> index 914587aabca0c..51b6ec0380ca4 100644
>> --- a/drivers/block/xen-blkback/xenbus.c
>> +++ b/drivers/block/xen-blkback/xenbus.c
>> @@ -522,8 +522,6 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
>>   	if (q && blk_queue_secure_erase(q))
>>   		vbd->discard_secure = true;
>>   
>> -	vbd->feature_gnt_persistent = feature_persistent;
>> -
>>   	pr_debug("Successful creation of handle=%04x (dom=%u)\n",
>>   		handle, blkif->domid);
>>   	return 0;
>> @@ -1090,10 +1088,9 @@ static int connect_ring(struct backend_info *be)
>>   		xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
>>   		return -ENOSYS;
>>   	}
>> -	if (blkif->vbd.feature_gnt_persistent)
>> -		blkif->vbd.feature_gnt_persistent =
>> -			xenbus_read_unsigned(dev->otherend,
>> -					"feature-persistent", 0);
>> +
>> +	blkif->vbd.feature_gnt_persistent = feature_persistent &&
>> +		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> 
> I'm not sure it's correct to potentially read feature_persistent
> multiple times like it's done here.
> 
> A device can be disconnected and re-attached multiple times, and that
> implies multiple calls to connect_ring which could make the state of
> feature_gnt_persistent change across reconnections if the value of
> feature_persistent is changed. I think that would be unexpected.
> 

Would that not be legitimate though? What happens if blkfront is 
upgraded (or downgraded)? Each connection should be 'groundhog day' for 
the backend IMO.

   Paul

> There are also similar issues with
> xenblk_max_queues/xen_blkif_max_ring_order changing after
> xen_blkbk_probe has been executed. We likely need to stash all those
> parameters so what's on xenbus is consistent with the limits enforced
> in blkback.
> 
> Thanks, Roger.
>