Date:   Thu, 2 Feb 2023 14:56:34 -0500
From:   Mike Snitzer <snitzer@kernel.org>
To:     Demi Marie Obenour <demi@invisiblethingslab.com>
Cc:     Jens Axboe <axboe@kernel.dk>,
        Roger Pau =?iso-8859-1?Q?Monn=E9?= <roger.pau@citrix.com>,
        Alasdair Kergon <agk@redhat.com>,
        Marek =?iso-8859-1?Q?Marczykowski-G=F3recki?= 
        <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: <Y9wVcskXyOk3bbzC@redhat.com>
References: <20230126033358.1880-1-demi@invisiblethingslab.com>
 <Y9vp3XDEQAl7TLND@redhat.com>
 <Y9wEF3rWfpiCKc2i@itl-email>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Y9wEF3rWfpiCKc2i@itl-email>
Precedence: bulk

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.

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

So send a separate patch, but I'm inclined to _not_ accept it because
it does potentially break some userspace.
 
> > > - 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