From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: Mike Snitzer <snitzer@kernel.org>
Cc: "Jens Axboe" <axboe@kernel.dk>,
"Roger Pau Monné" <roger.pau@citrix.com>,
"Alasdair Kergon" <agk@redhat.com>,
"Marek Marczykowski-Górecki" <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
Date: Thu, 2 Feb 2023 15:57:55 -0500 [thread overview]
Message-ID: <Y9wj2MpEXI/P2/6/@itl-email> (raw)
In-Reply-To: <Y9wVcskXyOk3bbzC@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3286 bytes --]
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:
>
> > 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:
> > >
> > > > This work aims to allow userspace to create and destroy block devices
> > > > in a race-free and leak-free way,
> > >
> > > "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.
> >
> > Will do in v2.
> >
> > > > and to allow them to be exposed to
> > > > other Xen VMs via blkback without leaks or races. It’s marked as RFC
> > > > for a few reasons:
> > > >
> > > > - The code has been only lightly tested. It might be unstable or
> > > > insecure.
> > > >
> > > > - The DM_DEV_CREATE ioctl gains a new flag. Unknown flags were
> > > > previously ignored, so this could theoretically break buggy userspace
> > > > tools.
> > >
> > > 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.
> >
> > Patch 2/7 is the diskseq change. v2 will contain a revised and tested
> > version with a greatly expanded commit message.
>
> 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.
> > >
> > > 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.
> >
> > 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?
>
> Nothing specific about code, was just a general reminder (based on how
> terse the 2/7 header was).
>
> Mike
Thanks for the feedback!
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
prev parent reply other threads:[~2023-02-02 20:58 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-26 3:33 [RFC PATCH 0/7] Allow race-free block device handling Demi Marie Obenour
2023-01-26 3:33 ` [RFC PATCH 1/7] block: Support creating a struct file from a block device Demi Marie Obenour
2023-01-30 8:08 ` Christoph Hellwig
2023-01-30 19:22 ` Demi Marie Obenour
2023-01-31 8:53 ` Christoph Hellwig
2023-01-31 16:27 ` Demi Marie Obenour
2023-02-01 7:45 ` Christoph Hellwig
2023-02-01 16:18 ` Demi Marie Obenour
2023-02-02 8:49 ` Ming Lei
2023-02-02 17:24 ` Demi Marie Obenour
2023-01-26 3:33 ` [RFC PATCH 2/7] Allow userspace to get an FD to a newly-created DM device Demi Marie Obenour
2023-01-26 3:33 ` [RFC PATCH 3/7] Implement diskseq checks in blkback Demi Marie Obenour
2023-01-26 3:33 ` [RFC PATCH 4/7] Increment diskseq when releasing a loop device Demi Marie Obenour
2023-01-30 8:09 ` Christoph Hellwig
2023-01-26 3:33 ` [RFC PATCH 6/7] Minor blkback cleanups Demi Marie Obenour
2023-01-26 3:33 ` [RFC PATCH 7/7] xen/blkback: Inform userspace that device has been opened Demi Marie Obenour
2023-02-02 16:50 ` [RFC PATCH 0/7] Allow race-free block device handling Mike Snitzer
2023-02-02 18:41 ` Demi Marie Obenour
2023-02-02 19:56 ` Mike Snitzer
2023-02-02 20:57 ` Demi Marie Obenour [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y9wj2MpEXI/P2/6/@itl-email \
--to=demi@invisiblethingslab.com \
--cc=agk@redhat.com \
--cc=axboe@kernel.dk \
--cc=dm-devel@redhat.com \
--cc=jgross@suse.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marmarek@invisiblethingslab.com \
--cc=roger.pau@citrix.com \
--cc=snitzer@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).