loop change deprecation bdev->bd_holder_lock

Commit Message

Christian Brauner Oct. 23, 2023, 3:35 p.m. UTC

On Mon, Oct 23, 2023 at 09:40:44AM +0200, Christian Brauner wrote:
> > 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?
> 
> Yeah, why don't we try that. In it's current form the concept isn't very
> useful and arguably is broken which no one has really noticed for years.
> 
> * codesearch.debian.net has zero users
> * codesearch on github has zero users
> * cs.android.com has zero users
> 
> Only definitions, strace, ltp, and stress-ng that sort of test this
> functionality. I say we try to deprecate this.

I just realized that if we're able to deprecate LOOP_CHANGE_FD we remove
one of the most problematic/weird cases for partitions and filesystems.

So right now, we can attach a filesystem image to a loop device and then
have partitions be claimed by different filesystems:

* create an image with two partitions
* format first partition as xfs, second as ext4

sudo losetup -f --show --read-only -P img1
sudo mount /dev/loop0p1 /mnt1
sudo mount /dev/loop0p2 /mnt2

What happens here is all very wrong afaict. When we issue, e.g., a
change fd event on the first partition:

sudo ./loop_change_fd /dev/loop0p1 img2

we call disk_force_media_change() but that only works on disk->part0
which means that we don't even cleanly shutdown the filesystem on the
partition we're trying to mess around with.

Later on we then completely fall on our faces when we fail to remove the
partitions because they're still in active use.

So I guess, ideally, we'd really force removal of the partitions and
shut down the filesystem but we can't easily do that because of locking
requirements where we can't acquire s_umount beneath open_mutex. Plus,
this wouldn't fit with the original semantics.

There's probably ways around this but I don't think that's actually
worth it for LOOP_CHANGE_FD. The places where forced removal of
partitions really matters is del_gendisk(). And there we've got it
working correctly and are able to actually remove partitions that still
have active users.

For now, we should give up any pretense that disk_force_media_change()
does anything useful for loop change fd and simply remove it completely.
It's either useless, or it breaks the original semantics of loop change
fd although I don't think anyone's ever used it the way I described
above.

So?

From 0d9b5c4963791f0c3ba8529ca56233be87122c59 Mon Sep 17 00:00:00 2001
From: Christian Brauner <[e-mail]>
Date: Mon, 23 Oct 2023 17:24:56 +0200
Subject: [PATCH] loop: drop disk_force_media_change()

The disk_force_media_change() call is currently only useful for
filesystems and in that case it's not very useful. Consider attaching a
filesystem image to a loop device and then have partitions be claimed by
different filesystems:

* create an image with two partitions
* format first partition as xfs, second as ext4

sudo losetup -f --show --read-only -P img1
sudo mount /dev/loop0p1 /mnt1
sudo mount /dev/loop0p2 /mnt2

When we issue e.g., a loop change fd event on the first partition:

sudo ./loop_change_fd /dev/loop0p1 img2

we call disk_force_media_change() but that only works on disk->part0
which means that we don't even cleanly shutdown the filesystem on the
partition we're trying to mess around with.

Later on we then completely fall on our faces when we fail to remove the
partitions because they're still in active use.

Give up any pretense that disk_force_media_change() does anything useful
for loop change fd and simply remove it completely. It's either useless,
or it's meaningless for the original semantics of loop change fd. Anyone
who uses this is on their own anyway.

Signed-off-by: Christian Brauner <[e-mail]>
---
 drivers/block/loop.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Oct. 24, 2023, 7:03 a.m. UTC | #1

On Mon, Oct 23, 2023 at 05:35:25PM +0200, Christian Brauner wrote:
> I just realized that if we're able to deprecate LOOP_CHANGE_FD we remove
> one of the most problematic/weird cases for partitions and filesystems.

> change fd event on the first partition:
> 
> sudo ./loop_change_fd /dev/loop0p1 img2
> 
> we call disk_force_media_change() but that only works on disk->part0
> which means that we don't even cleanly shutdown the filesystem on the
> partition we're trying to mess around with.

Yes, disk_force_media_change has that general problem back from the
early Linux days (it had a different name back then, though).  I think
it is because traditionally removable media in Linux never had
partitions, e.g. the CDROM drivers typically only allocated a single
minor number so they could not be scanned.  But that has changed because
the interfaces got used for different use cases, and we also had
dynamic majors for a long time that now allow partitions.  And there
are real use cases even for traditional removable media, e.g. MacOS
CDROMs traditionally did have partitions.

> For now, we should give up any pretense that disk_force_media_change()
> does anything useful for loop change fd and simply remove it completely.
> It's either useless, or it breaks the original semantics of loop change
> fd although I don't think anyone's ever used it the way I described
> above.

Maybe we can just drop the CHANGE_FD ioctl and see if anyone screams?

Christian Brauner Oct. 24, 2023, 8:44 a.m. UTC | #2

(Sorry for the broken "Subject:" btw in the first mail.)

On Tue, Oct 24, 2023 at 12:03:25AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 23, 2023 at 05:35:25PM +0200, Christian Brauner wrote:
> > I just realized that if we're able to deprecate LOOP_CHANGE_FD we remove
> > one of the most problematic/weird cases for partitions and filesystems.
> 
> > change fd event on the first partition:
> > 
> > sudo ./loop_change_fd /dev/loop0p1 img2
> > 
> > we call disk_force_media_change() but that only works on disk->part0
> > which means that we don't even cleanly shutdown the filesystem on the
> > partition we're trying to mess around with.
> 
> Yes, disk_force_media_change has that general problem back from the
> early Linux days (it had a different name back then, though).  I think
> it is because traditionally removable media in Linux never had
> partitions, e.g. the CDROM drivers typically only allocated a single
> minor number so they could not be scanned.  But that has changed because
> the interfaces got used for different use cases, and we also had
> dynamic majors for a long time that now allow partitions.  And there
> are real use cases even for traditional removable media, e.g. MacOS
> CDROMs traditionally did have partitions.
> 
> > For now, we should give up any pretense that disk_force_media_change()
> > does anything useful for loop change fd and simply remove it completely.
> > It's either useless, or it breaks the original semantics of loop change
> > fd although I don't think anyone's ever used it the way I described
> > above.
> 
> Maybe we can just drop the CHANGE_FD ioctl and see if anyone screams?

Yes, I suggested that in the prior mail. I think we should do that.
We'd need changes to LTP and blktests but there are no active users in
either codesearch.debian, cs.github, or cs.android.

Patch

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 9f2d412fc560..87c98e35abac 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -603,7 +603,8 @@  static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 		goto out_err;
 
 	/* and ... switch */
-	disk_force_media_change(lo->lo_disk);
+	invalidate_bdev(lo->lo_disk->part0);
+	set_bit(GD_NEED_PART_SCAN, &lo->lo_disk->state);
 	blk_mq_freeze_queue(lo->lo_queue);
 	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
 	lo->lo_backing_file = file;