Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iac594737:Fastmail
Date:   Thu, 8 Jun 2023 11:23:07 -0400
From:   Demi Marie Obenour <demi@invisiblethingslab.com>
To:     Roger Pau =?utf-8?B?TW9ubsOp?= <roger.pau@citrix.com>
Cc:     Jens Axboe <axboe@kernel.dk>, Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@kernel.org>, dm-devel@redhat.com,
        Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= 
        <marmarek@invisiblethingslab.com>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 16/16] xen-blkback: Inform userspace that device has
 been opened
Message-ID: <ZIHyXrAj5+DXAblD@itl-email>
References: <20230530203116.2008-1-demi@invisiblethingslab.com>
 <20230530203116.2008-17-demi@invisiblethingslab.com>
 <ZH75OTMA6N3zYrH2@Air-de-Roger>
 <ZH9tcGh0N2fEcwjH@itl-email>
 <ZIBDgKKDhDj+//Q0@Air-de-Roger>
 <ZICwaWidZxhaGp8v@itl-email>
 <ZIGbUDpqjwbR5zmz@Air-de-Roger>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="PU98An8gDVvBtzYA"
Content-Disposition: inline
In-Reply-To: <ZIGbUDpqjwbR5zmz@Air-de-Roger>
Precedence: bulk


--PU98An8gDVvBtzYA
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Date: Thu, 8 Jun 2023 11:23:07 -0400
From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: Roger Pau =?utf-8?B?TW9ubsOp?= <roger.pau@citrix.com>
Cc: Jens Axboe <axboe@kernel.dk>, Alasdair Kergon <agk@redhat.com>,
	Mike Snitzer <snitzer@kernel.org>, dm-devel@redhat.com,
	Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= <marmarek@invisiblethingslab.com>,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 16/16] xen-blkback: Inform userspace that device has
 been opened

On Thu, Jun 08, 2023 at 11:11:44AM +0200, Roger Pau Monn=C3=A9 wrote:
> On Wed, Jun 07, 2023 at 12:29:26PM -0400, Demi Marie Obenour wrote:
> > On Wed, Jun 07, 2023 at 10:44:48AM +0200, Roger Pau Monn=C3=A9 wrote:
> > > On Tue, Jun 06, 2023 at 01:31:25PM -0400, Demi Marie Obenour wrote:
> > > > On Tue, Jun 06, 2023 at 11:15:37AM +0200, Roger Pau Monn=C3=A9 wrot=
e:
> > > > > On Tue, May 30, 2023 at 04:31:16PM -0400, Demi Marie Obenour wrot=
e:
> > > > > > Set "opened" to "0" before the hotplug script is called.  Once =
the
> > > > > > device node has been opened, set "opened" to "1".
> > > > > >=20
> > > > > > "opened" is used exclusively by userspace.  It serves two purpo=
ses:
> > > > > >=20
> > > > > > 1. It tells userspace that the diskseq Xenstore entry is suppor=
ted.
> > > > > >=20
> > > > > > 2. It tells userspace that it can wait for "opened" to be set t=
o 1.
> > > > > >    Once "opened" is 1, blkback has a reference to the device, so
> > > > > >    userspace doesn't need to keep one.
> > > > > >=20
> > > > > > Together, these changes allow userspace to use block devices wi=
th
> > > > > > delete-on-close behavior, such as loop devices with the autocle=
ar flag
> > > > > > set or device-mapper devices with the deferred-remove flag set.
> > > > >=20
> > > > > 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 attem=
pting
> > > > > to reload blkback.
> > > >=20
> > > > Should blkback stop itself from being unloaded if delete-on-close i=
s in
> > > > use?
> > >=20
> > > Hm, maybe.  I guess that's the best we can do right now.
> >=20
> > I=E2=80=99ll implement this.
>=20
> Let's make this a separate patch.

Good idea.

> > > > > Isn't there some existing way to check whether a device is opened?
> > > > > (stat syscall maybe?).
> > > >=20
> > > > Knowing that the device has been opened isn=E2=80=99t enough.  The =
block script
> > > > needs to be able to wait for blkback (and not something else) to op=
en
> > > > the device.  Otherwise it will be confused if the device is opened =
by
> > > > e.g. udev.
> > >=20
> > > Urg, no, the block script cannot wait indefinitely for blkback to open
> > > the device, as it has an execution timeout.  blkback is free to only
> > > open the device upon guest frontend connection, and that (when using
> > > libxl) requires the hotplug scripts execution to be finished so the
> > > guest can be started.
> >=20
> > I=E2=80=99m a bit confused here.  My understanding is that blkdev_get_b=
y_dev()
> > already opens the device, and that happens in the xenstore watch
> > handler.  I have tested this with delete-on-close device-mapper devices,
> > and it does work.
>=20
> Right, but on a very contended system there's no guarantee of when
> blkback will pick up the update to "physical-device" and open the
> device, so far the block script only writes the physical-device node
> and exits.  With the proposed change the block script will also wait
> for blkback to react to the physcal-device write, hence making VM
> creation slower.

Only block scripts that choose to wait for device open suffer
this performance penalty.  My current plan is to only do so for
delete-on-close devices which are managed by the block script
itself.  Other devices will not suffer a performance hit.

In the long term, I would like to solve this problem entirely by using
an ioctl to configure blkback.  The ioctl would take a file descriptor
argument, avoiding the need for a round-trip through xenstore.  This
also solves a security annoyance with the current design, which is that
the device is opened by a kernel thread and so the security context of
whoever requested the device to be opened is lost.

> > > > > I would like to avoid adding more xenstore blkback state if such
> > > > > information can be fetched from other methods.
> > > >=20
> > > > I don=E2=80=99t think it can be, unless the information is passed v=
ia a
> > > > completely different method.  Maybe netlink(7) or ioctl(2)?  Arguab=
ly
> > > > this information should not be stored in Xenstore at all, as it exp=
oses
> > > > backend implementation details to the frontend.
> > >=20
> > > Could you maybe use sysfs for this information?
> >=20
> > Probably?  This would involve adding a new file in sysfs.
> >=20
> > > We have all sorts of crap in xenstore, but it would be best if we can
> > > see of placing stuff like this in another interface.
> >=20
> > Fair.
>=20
> Let's see if that's a suitable approach, and we can avoid having to
> add an extra node to xenstore.

I thought about this some more and realized that in Qubes OS, we might
want to include the diskseq in the information dom0 gets about each
exported block device.  This would allow dom0 to write the xenstore node
itself, but it would require some way for dom0 to be informed about
blkback having this feature.
--=20
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

--PU98An8gDVvBtzYA
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmSB8l4ACgkQsoi1X/+c
IsFSYA//XhC38a6SDufwf6y+xaRXnVDYSdUsP21JYKyDgiz9s6lEag0nuPNrUkLt
+pQdAxPoPq99fFKrhleiVEbr1GtEBonRU6qMCe/cUJ/BqTkUyJ75EOeRtxFjr7DI
QC9dbaly+Q+WkiLwmtHiaWgk56TaGp+GqYrG2Zj6w0fmIwOmZ59IRaDeIqwJYA6p
T4vagAEMK+f3poK6sWfEhZE2IJ5tGqyZwxvpVPY3ldnm5wS9n5WmNZF0rM7lKGzF
J8S8ITQut9DtW9NicTQfFz1jjIMDs3KFReSPchQ41+IRUx6nM3sjr6RnE2kBCER8
io/OtNbe6TVv5LxDe2bDLpEzPesjl3RNrx+mgC/LpIaO8JhvZEYpF49XmgQbXRhe
HMob73O0quNIUMQLqQ18zunfqE4wIq8Yfv4E146jRaItTN+AZT4oAFZZJb8f43BS
c95GNTtjdguBVeLKb3Vi9dH9bgqtO9CbDop9PeP1T05q2VHxMTEGhf2XkZoOCCxI
oMIV9IEbkBcVQT/nVZa56Suqgp6XKy/llGIrvk45bhraLT8ljpoHUeZnjQaJxJEO
YWr7ImrznVH+57dQbSlkQrWKZcp8f0dzzfoDGGWXP6UTTA9JiiVMkqjmk6YZEyQL
VGban/xP4mC8GD10OtAsc/b3n0w87wxVYjYT9hdDtgIBUzjcGP4=
=1nZV
-----END PGP SIGNATURE-----

--PU98An8gDVvBtzYA--