Feedback-ID: iac594737:Fastmail
Date:   Thu, 2 Feb 2023 15:57:55 -0500
From:   Demi Marie Obenour <demi@invisiblethingslab.com>
To:     Mike Snitzer <snitzer@kernel.org>
Cc:     Jens Axboe <axboe@kernel.dk>,
        Roger Pau =?utf-8?B?TW9ubsOp?= <roger.pau@citrix.com>,
        Alasdair Kergon <agk@redhat.com>,
        Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= 
        <marmarek@invisiblethingslab.com>, Juergen Gross <jgross@suse.com>,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        xen-devel@lists.xenproject.org, dm-devel@redhat.com
Subject: Re: [RFC PATCH 0/7] Allow race-free block device handling
Message-ID: <Y9wj2MpEXI/P2/6/@itl-email>
References: <20230126033358.1880-1-demi@invisiblethingslab.com>
 <Y9vp3XDEQAl7TLND@redhat.com>
 <Y9wEF3rWfpiCKc2i@itl-email>
 <Y9wVcskXyOk3bbzC@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="cgmN2P5oJuiMWNP7"
Content-Disposition: inline
In-Reply-To: <Y9wVcskXyOk3bbzC@redhat.com>
Precedence: bulk


--cgmN2P5oJuiMWNP7
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Date: Thu, 2 Feb 2023 15:57:55 -0500
From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: Mike Snitzer <snitzer@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	Roger Pau =?utf-8?B?TW9ubsOp?= <roger.pau@citrix.com>,
	Alasdair Kergon <agk@redhat.com>,
	Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= <marmarek@invisiblethingslab.com>,
	Juergen Gross <jgross@suse.com>, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
	dm-devel@redhat.com
Subject: Re: [RFC PATCH 0/7] Allow race-free block device handling

On Thu, Feb 02, 2023 at 02:56:34PM -0500, Mike Snitzer wrote:
> On Thu, Feb 02 2023 at  1:41P -0500,
> Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
>=20
> > On Thu, Feb 02, 2023 at 11:50:37AM -0500, Mike Snitzer wrote:
> > > On Wed, Jan 25 2023 at 10:33P -0500,
> > > Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
> > >=20
> > > > This work aims to allow userspace to create and destroy block devic=
es
> > > > in a race-free and leak-free way,
> > >=20
> > > "race-free and leak-free way" implies there both races and leaks in
> > > existing code. You're making claims that are likely very specific to
> > > your Xen use-case.  Please explain more carefully.
> >=20
> > Will do in v2.
> >=20
> > > > and to allow them to be exposed to
> > > > other Xen VMs via blkback without leaks or races.  It=E2=80=99s mar=
ked as RFC
> > > > for a few reasons:
> > > >=20
> > > > - The code has been only lightly tested.  It might be unstable or
> > > >   insecure.
> > > >=20
> > > > - The DM_DEV_CREATE ioctl gains a new flag.  Unknown flags were
> > > >   previously ignored, so this could theoretically break buggy users=
pace
> > > >   tools.
> > >=20
> > > Not seeing a reason that type of DM change is needed. If you feel
> > > strongly about it send a separate patch and we can discuss it.
> >=20
> > Patch 2/7 is the diskseq change.  v2 will contain a revised and tested
> > version with a greatly expanded commit message.
>=20
> I'm aware that 2/7 is where you make the DM change to disallow unknown
> flags, what I'm saying is I don't see a reason for that change.

Thanks for the clarification.

> Certainly doesn't look to be a requirement for everything else in that
> patch.

Indeed it is not.  I will make it a separate patch.

> So send a separate patch, but I'm inclined to _not_ accept it because
> it does potentially break some userspace.

Is it okay to add DM_FILE_DESCRIPTOR_FLAG (with the same meaning as in
2/7) _without_ rejecting unknown flags?  The same patch would bump the
minor version number, so userspace would still be able to tell if the
kernel supported DM_FILE_DESCRIPTOR_FLAG.  If you wanted, I could ignore
DM_FILE_DESCRIPTOR_FLAG unless the minor number passed by userspace is
sufficiently recent.

Another option would be to make userspace opt-in to strict parameter
checking by passing 5 as the major version instead of 4.  Userspace
programs that passed 4 would get the old behavior, while userspace
programs that passed 5 would get strict parameter checking and be able
to use new features such as DM_FILE_DESCRIPTOR_FLAG.

> > > > - I have no idea if I got the block device reference counting and
> > > >   locking correct.
> > >=20
> > > Your headers and justifcation for this line of work are really way too
> > > terse. Please take the time to clearly make the case for your changes
> > > in both the patch headers and code.
> >=20
> > I will expand the commit message in v2, but I am not sure what you want
> > me to add to the code comments.  Would you mind explaining?
>=20
> Nothing specific about code, was just a general reminder (based on how
> terse the 2/7 header was).
>=20
> Mike

Thanks for the feedback!
--=20
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

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

iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmPcI9YACgkQsoi1X/+c
IsF28hAAhImjjtlWhG/0QL5ADEU4goFI++sArqOlA2RYmyVjye52flwl4Fzd+px3
qEsA6mLllD0x2SxfycOirT3D5d0Gsa8BVPs+WiaWed5Rc+LececX+YGQ6HgEUZBM
Ig69iNxrCcA5CARoNWd/RsHZSuxSE9ltgzo00ghW+LNRKVyYAn0nuAWddWLqYrvr
wVdonaDhI5IC15GPQ4CK4T21JTO16NsUjld1A03pgr0ubzV+UpwWGNycwjSpz2KB
WzHAq5NTLvda6dLUmt1o/z7dJR04gPA8ICyF/zKXzkjuOgeTvQOTY25iv5O3xQYv
1WHAJJqxb8twWMw+bU92sjoUJy4jdqr5DQ7n+ga5eEG0YAPBkRwcPuKdJdotbUFF
8VP90nUdWDGlmc+w20GlNzziVOyxLFbVgxFO1evJ0Kq023gzvcgs69RqV1mLfP44
hD18hdcC2ytp8TwhNrERzmBxWjEtD1MphgGXAiJQ0qQEyYlKAYNUyolM8lTnTQR9
Xccwj6ow7pMDIV+DkNuD4Ebiu7dx4WhjyTc4jx9NHSUUDeuff0v4PzFCiXr/x/dN
0Sml2ojt43HPUfLXaepFNwLCqL54zGNNa2gsH0gyl0iwAFMAgNbMcUAx10lCOmDj
d7i37cInvPYgsSr2WUgz7mbiLlQN3Eh8tf5ut+q+OOOq+cRPKCs=
=P5Xt
-----END PGP SIGNATURE-----

--cgmN2P5oJuiMWNP7--