Message-ID: <YNNTTUYRlpXDqMgX@gardel-login>
Date:   Wed, 23 Jun 2021 17:29:17 +0200
From:   Lennart Poettering <mzxreary@...inter.de>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Luca Boccassi <bluca@...ian.org>,
        Matteo Croce <mcroce@...ux.microsoft.com>,
        linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Jens Axboe <axboe@...nel.dk>, linux-kernel@...r.kernel.org,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Damien Le Moal <damien.lemoal@....com>,
        Tejun Heo <tj@...nel.org>,
        Javier Gonz??lez <javier@...igon.com>,
        Niklas Cassel <niklas.cassel@....com>,
        Johannes Thumshirn <johannes.thumshirn@....com>,
        Hannes Reinecke <hare@...e.de>,
        Matthew Wilcox <willy@...radead.org>,
        JeffleXu <jefflexu@...ux.alibaba.com>
Subject: Re: [PATCH v3 6/6] loop: increment sequence number

On Mi, 23.06.21 15:25, Christoph Hellwig (hch@...radead.org) wrote:

> On Wed, Jun 23, 2021 at 02:13:25PM +0100, Luca Boccassi wrote:
> > On Wed, 2021-06-23 at 12:57 +0100, Christoph Hellwig wrote:
> > > On Wed, Jun 23, 2021 at 12:58:58PM +0200, Matteo Croce wrote:
> > > > From: Matteo Croce <mcroce@...rosoft.com>
> > > >
> > > > On a very loaded system, if there are many events queued up from multiple
> > > > attach/detach cycles, it's impossible to match them up with the
> > > > LOOP_CONFIGURE or LOOP_SET_FD call, since we don't know where the position
> > > > of our own association in the queue is[1].
> > > > Not even an empty uevent queue is a reliable indication that we already
> > > > received the uevent we were waiting for, since with multi-partition block
> > > > devices each partition's event is queued asynchronously and might be
> > > > delivered later.
> > > >
> > > > Increment the disk sequence number when setting or changing the backing
> > > > file, so the userspace knows which backing file generated the event:
> > >
> > > Instead of manually incrementing the sequence here, can we make loop
> > > generate the DISK_EVENT_MEDIA_CHANGE event on a backing device (aka
> > > media) change?
> >
> > Hi,
> >
> > This was answered in the v1 thread:
> >
> > https://lore.kernel.org/linux-fsdevel/20210315201331.GA2577561@casper.infradead.org/t/#m8a677028572e826352cbb1e19d1b9c1f3b6bff4b
> >
> > The fundamental issue is that we'd be back at trying to correlate
> > events to loopdev instances, which does not work reliably - hence this
> > patch series. With the new ioctl, we can get the id immediately and
> > without delay when we create the device, with no possible races. Then
> > we can handle events reliably, as we can correlate correctly in all
> > cases.
>
> I very much disagree with your reply there.  The device now points to
> a different media.  Both for the loop device, a floppy or a CD changer
> probably by some kind of user action.  In the last cast it might even
> by done entirely locally through a script just like the loop device.

I am not sure I grok your point.

but let me try to explain why I think it's better to make media
changes *also* trigger seqno changes, and not make media change events
the *only* way to trigger seqno changes.

1. First of all, loopback devices currently don't hook into the media
   change logic (which so far is focussed on time-based polling
   actually, for both CDs and floppies). Adding this would change
   semantics visibly to userspace (since userspace would suddenly see
   another action=change + DISK_MEDIA_CHANGE=1 uevent suddenly that it
   needs to handle correctly). One can certainly argue that userspace
   must be ready to get additional uevents like this any time, but
   knowing userspace a bit I am pretty sure this will confuse some
   userspace that doesn't expect this. I mean loopback block devices
   already issue "change" uevents on attachment and detachment, one
   that userpace typically expects, but adding the media change one
   probably means sending two (?) of these out for each
   attachment. One being the old one from the loopback device itself,
   and then another one for the media change from the mdia change
   logic. That's not just noisy, but also ugly.

2. We want seqnums to be allocated for devices not only when doing
   media change (e.g. when attaching or detaching a loopback device)
   but also when allocating a block device, so that even before the
   first media change event a block device has a sequence number. This
   means allocating a sequence number for block devices won't be
   limited to the media change code anyway.

3. Doing the sequence number bumping in media change code exclusively
   kinda suggests this was something we could properly abstract away,
   to be done only there, and that the rest of the block subsystems
   wouldn#t have to bother much. But I am pretty sure that's not
   correct: in particular in the loopback block device code (but in
   other block subsystems too I guess) we really want to be able to
   atomically attach a loopback block device and return the seqnum of
   that very attachmnt so that we can focus on uevents for it. Thus,
   attachment, allocation and returning the seqnum to userspace in the
   LOOP_CONFIGURE ioctl (or whatever is appropriate) kinda go hand in
   hand.

4. The media change protocol follows a protocol along with the eject
   button handling (DISK_EVENT_EJECT_REQUEST), the locking of the tray
   and the time based polling. i.e. it's specifically focussed on
   certain hw features, none of which really apply to loopback block
   devices, which have no trays to lock, but eject buttons to handle,
   and where media change is always triggered internally by privileged
   code, never externally by the user. I doubt it makes sense to mix
   these up. Currently udev+udisks in userspace implement that
   procotol for cdroms/floppies, and I doubt we would want to bother
   to extend this for loopback devices in userspace. In fact, if media
   change events are added to loopback devices, we probably would have
   to change userspace to ignore them.

TLDR: making loopback block devices generate media is a (probably
minor, but unclear) API break, probably means duplicating uevent
traffic for it, won't abstract things away anyway, won't allocate a
seqnum on device allocation, and won't actually use anything of the
real media change functionality.

Or to say this differently: if the goal is to make loopback block
devices to send CDROM compatible media change events to userspace,
then I think it would probably make more sense to attach the
DISK_MEDIA_CHANGE=1 property to the attachment/detachment uevents the
loopback devices *already* generate, rather than to try to shoehorn the
existing media change infrastructure into the loopback device logic,
trying to reuse it even though nothing of it is really needed.

But that said, I am not aware of anyone ever asking for CDROM
compatible EDISK_MEDIA_CHANGE=1 uevents for loopback devices. I really
wouldn't bother with that. Sounds like nothing anyone want with a
chance of breaking things everywhere. (i.e. remember the
"bind"/"unbind" uevent fiasco?)

Lennart

--
Lennart Poettering, Berlin

Powered by blists - more mailing lists