Re: [PATCH v2 16/16] xen-blkback: Inform userspace that device has been opened

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Jun 06, 2023 at 11:15:37AM +0200, Roger Pau Monné wrote:
> On Tue, May 30, 2023 at 04:31:16PM -0400, Demi Marie Obenour wrote:
> > Set "opened" to "0" before the hotplug script is called.  Once the
> > device node has been opened, set "opened" to "1".
> > 
> > "opened" is used exclusively by userspace.  It serves two purposes:
> > 
> > 1. It tells userspace that the diskseq Xenstore entry is supported.
> > 
> > 2. It tells userspace that it can wait for "opened" to be set to 1.
> >    Once "opened" is 1, blkback has a reference to the device, so
> >    userspace doesn't need to keep one.
> > 
> > Together, these changes allow userspace to use block devices with
> > delete-on-close behavior, such as loop devices with the autoclear flag
> > set or device-mapper devices with the deferred-remove flag set.
> 
> There was some work in the past to allow reloading blkback as a
> module, it's clear that using delete-on-close won't work if attempting
> to reload blkback.

Should blkback stop itself from being unloaded if delete-on-close is in
use?

> Isn't there some existing way to check whether a device is opened?
> (stat syscall maybe?).

Knowing that the device has been opened isn’t enough.  The block script
needs to be able to wait for blkback (and not something else) to open
the device.  Otherwise it will be confused if the device is opened by
e.g. udev.

> I would like to avoid adding more xenstore blkback state if such
> information can be fetched from other methods.

I don’t think it can be, unless the information is passed via a
completely different method.  Maybe netlink(7) or ioctl(2)?  Arguably
this information should not be stored in Xenstore at all, as it exposes
backend implementation details to the frontend.

> > diff --git a/drivers/block/xen-blkback/xenbus.c 
> > b/drivers/block/xen-blkback/xenbus.c
> > index 
> > 9c3eb148fbd802c74e626c3d7bcd69dcb09bd921..519a78aa9073d1faa1dce5c1b36e95ae58da534b
> >  100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -699,6 +713,14 @@ static int xen_blkbk_probe(struct xenbus_device *dev,
> >     if (err)
> >             pr_warn("%s write out 'max-ring-page-order' failed\n", 
> > __func__);
> >  
> > +   /*
> > +    * This informs userspace that the "opened" node will be set to "1" when
> > +    * the device has been opened successfully.
> > +    */
> > +   err = xenbus_write(XBT_NIL, dev->nodename, "opened", "0");
> > +   if (err)
> > +           goto fail;
> > +
> 
> You would need to set "opened" before registering the xenstore backend
> watch AFAICT, or else it could be racy.

Will fix in the next version.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature