On Fri 20-10-23 13:31:20, Christian Brauner wrote:
> On Thu, Oct 19, 2023 at 06:40:27AM -0700, Christoph Hellwig wrote:
> > On Thu, Oct 19, 2023 at 10:33:36AM +0200, Christian Brauner wrote:
> > > some device removal. I also messed around with the loop code and
> > > specifically used LOOP_CHANGE_FD to force a disk_force_media_change() on
> > > a filesystem.
> >
> > Can you wire that up for either blktests or xfstests as well?
>
> Yeah, I'll try to find some time to do this.
>
> So while I was testing this I realized that the behavior of
> LOOP_CHANGE_FD changed in commit 9f65c489b68d ("loop: raise media_change
> event") and I'm not clear whether this is actually correct or not.
>
> loop(4) states
>
> "Switch the backing store of the loop device to the new file identified
> file descriptor specified in the (third) ioctl(2) argument, which is an
> integer. This operation is possible only if the loop device is
> read-only and the new backing store is the same size and type as the
> old backing store."
>
> So the original use-case for this ioctl seems to have been to silently
> swap out the backing store. Specifcially it seems to have been used once
> upon a time for live images together with device mapper over read-only
> loop devices. Where device mapper can direct the writes to some other
> location or sm.
LOOP_CHANGE_FD was an old hacky way to switch from read-only installation
image to a full-blown RW filesystem without reboot. I'm not sure about
details how it was supposed to work but reportedly Al Viro implemented that
for Fedora installation back in the old days.
> Assuming that's correct, I think as long as you have something like
> device mapper that doesn't use blk_holder_ops it would still work. But
> that commit changed behavior for filesystems because we now do:
>
> loop_change_fd()
> -> disk_force_media_change()
> -> bdev_mark_dead()
> -> bdev->bd_holder_ops->mark_dead::fs_mark_dead()
>
> So in essence if you have a filesystem on a loop device via:
>
> truncate -s 10G img1
> mkfs.xfs -f img1
> LOOP_DEV=$(sudo losetup -f --read-only --show img1)
>
> truncate -s 10G img2
> sudo ./loop_change_fd $LOOP_DEV ./img2
>
> We'll shut down that filesystem. I personally think that's correct and
> it's buggy not to do that when we have the ability to inform the fs
> about media removal but technically it kinda defeats the original
> use-case for LOOP_CHANGE_FD.
I agree that it breaks the original usecase for LOOP_CHANGE_FD. I'd say it
also shows nobody is likely using LOOP_CHANGE_FD anymore. Maybe time to try
deprecating it?
> In practice however, I don't think it matters because I think no one is
> using LOOP_CHANGE_FD in that way. Right now all this is a useful for is
> a bdev_mark_dead() test.
:)
> And one final question:
>
> dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
> disk_force_media_change(lo->lo_disk);
> /* more stuff */
> dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
>
> What exactly does that achieve? Does it just delay the delivery of the
> uevent after the disk sequence number was changed in
> disk_force_media_change()? Because it doesn't seem to actually prevent
> uevent generation.
Well, if you grep for dev_get_uevent_suppress() you'll notice there is
exactly *one* place looking at it - the generation of ADD event when adding
a partition bdev. I'm not sure what's the rationale behind this
functionality.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
