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

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> >   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.