loop device auto release patch

DISCUSSION Phillip Susi 4 participants 6 messages Jul 15, 2011 – Aug 4, 2011 1 message not in the archive Block layer

lore

Share & tools

lore

Phillip Susi

author

2011-07-15 19:12 UTC #1 lore raw

In reply to a message not in the archive

Quoted 5 lines · Ayan George

Hi Philip,
This is the patch I'd like to submit for the loop device. I'm in the process of testing it now. I'm pretty confident it will work.

Looks good to me. Forwarding to Andrew Morton. Andrew, please disregard my previous patch as I think this one is better.

Quoted 1 line

From c783ba6a26eff42d3cb0061e4fcb8ee8a16b3e67 Mon Sep 17 00:00:00 2001

From: Ayan George <[email protected]>
Date: Thu, 14 Jul 2011 14:16:58 -0400
Subject: [PATCH] Always invalidate cleared loop block devices

The API for loop_clr_fd() is confusing -- the second argument (bdev) isn't necessesary as struct loop_device contains a pointer to the block device it is assocated with.

There is a cases where loop_clr_fd() is called with NULL for bdev which prevents the block device from ever being invalidated with invalidate_bdev() and prevents a uevent from being emitted.

This patch removes the bdev argument from loop_clr_fd(), unconditionally invalidates lo->lo_device when cleared, and unconditionally emits a uevent for removed loops.

Launchpad bug for reference:

BugLink https://bugs.launchpad.net/bugs/548546

Signed-off-by Ayan George <[email protected]>

---
 drivers/block/loop.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

drivers/block/loop.c +14 ~~-10~~

index 76c8da7..a40e12b 100644
@@ -987,11 +987,13 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer,
 	return err;
 }
 
-static int loop_clr_fd(struct loop_device *lo, struct block_device *bdev)
+static int loop_clr_fd(struct loop_device *lo)
 {
 	struct file *filp = lo->lo_backing_file;
 	gfp_t gfp = lo->old_gfp_mask;
 
+	struct block_device *bdev = lo->lo_device;
+
 	if (lo->lo_state != Lo_bound)
 		return -ENXIO;

@@ -1022,15 +1024,17 @@ static int loop_clr_fd(struct loop_device *lo, struct block_device *bdev)
 	memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
-	if (bdev)
-		invalidate_bdev(bdev);
+
+	invalidate_bdev(bdev);
+
 	set_capacity(lo->lo_disk, 0);
 	loop_sysfs_exit(lo);
-	if (bdev) {
-		bd_set_size(bdev, 0);
-		/* let user-space know about this change */
-		kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
-	}
+
+	bd_set_size(bdev, 0);
+
+	/* let user-space know about this change */
+	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
+
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	lo->lo_state = Lo_unbound;
 	/* This is safe: open() is still holding a reference. */

@@ -1298,7 +1302,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	case LOOP_CLR_FD:
 		/* loop_clr_fd would have unlocked lo_ctl_mutex on success */
-		err = loop_clr_fd(lo, bdev);
+		err = loop_clr_fd(lo);
 		if (!err)
 			goto out_unlocked;
 		break;

@@ -1509,7 +1513,7 @@ static int lo_release(struct gendisk *disk, fmode_t mode)
 		 * In autoclear mode, stop the loop thread
 		 * and remove configuration after last close.
 		 */
-		err = loop_clr_fd(lo, NULL);
+		err = loop_clr_fd(lo);
 		if (!err)
 			goto out_unlocked;
 	} else {

Signature · 1 line

1.7.4.1

Andrew Morton

2011-07-15 19:19 UTC #2 lore raw

Quoted 9 lines · On Fri, 15 Jul 2011 15:12:50 -0400 Phillip Susi <[email protected]> wrote:

Quoted 5 lines · Ayan George
Hi Philip,
This is the patch I'd like to submit for the loop device. I'm in the process of testing it now. I'm pretty confident it will work.
Looks good to me. Forwarding to Andrew Morton. Andrew, please disregard my previous patch as I think this one is better.

Would prefer to wait until Ayan has completed testing it. It would be good if you were to test it too please.

Quoted 11 lines

[0001-Always-invalidate-cleared-loop-block-devices.patch text/x-patch (2.9KB)]
Quoted 1 line
From c783ba6a26eff42d3cb0061e4fcb8ee8a16b3e67 Mon Sep 17 00:00:00 2001
From: Ayan George <[email protected]>
Date: Thu, 14 Jul 2011 14:16:58 -0400
Subject: [PATCH] Always invalidate cleared loop block devices
The API for loop_clr_fd() is confusing -- the second argument (bdev) isn't necessesary as struct loop_device contains a pointer to the block device it is assocated with.

Quoted 7 lines

There is a cases where loop_clr_fd() is called with NULL for bdev which prevents the block device from ever being invalidated with invalidate_bdev() and prevents a uevent from being emitted.
This patch removes the bdev argument from loop_clr_fd(), unconditionally invalidates lo->lo_device when cleared, and unconditionally emits a uevent for removed loops.

The patch appears to do two unrelated things. That's generally frowned upon, but doesn't bother me much if the patch is small.

Still, splitting it into two patches (in which the bugfix is staged first) would be advantageous for people who might wish to backport the fix into earlier kernels.

Ayan George

2011-07-15 22:29 UTC #3 lore raw

Quoted 4 lines · Andrew Morton

Still, splitting it into two patches (in which the bugfix is staged first) would be advantageous for people who might wish to backport the fix into earlier kernels.

Okay -- I'll submit two smaller patches and I'll early next week.

-ayan

Phillip Susi

author

2011-08-04 01:58 UTC #4 lore raw

In reply to Andrew Morton

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 6 lines · Andrew Morton

The patch appears to do two unrelated things. That's generally frowned upon, but doesn't bother me much if the patch is small.
Still, splitting it into two patches (in which the bugfix is staged first) would be advantageous for people who might wish to backport the fix into earlier kernels.

It looks like this got stalled and someone emailed me asking what happened to it. I'm not sure that splitting the patch in two makes sense. I don't see how it does two unrelated things. The uevent problem was caused by the argument being NULL. This patch just removes the argument since it is entirely unnecessary. Given that the argument is gone and can no longer be passed as NULL, the tests for NULL are rendered moot, and so removing them seems quite related. I suppose you could do the first and not the second, but then you would be leaving cruft behind.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk45/MAACgkQJ4UciIs+XuL7+gCfUA8TpE2SRRCo+/VzRJMCrwg6 OEsAmwWphcaIF/wdguyHch6gEizv1Tjb
=pGGG
-----END PGP SIGNATURE-----

Andrew Morton

2011-08-04 02:03 UTC #5 lore raw

Quoted 8 lines · On Wed, 03 Aug 2011 21:58:24 -0400 Phillip Susi <[email protected]> wrote:

Quoted 6 lines · Andrew Morton
The patch appears to do two unrelated things. That's generally frowned upon, but doesn't bother me much if the patch is small.
Still, splitting it into two patches (in which the bugfix is staged first) would be advantageous for people who might wish to backport the fix into earlier kernels.

Quoted 9 lines

It looks like this got stalled and someone emailed me asking what happened to it. I'm not sure that splitting the patch in two makes sense. I don't see how it does two unrelated things. The uevent problem was caused by the argument being NULL. This patch just removes the argument since it is entirely unnecessary. Given that the argument is gone and can no longer be passed as NULL, the tests for NULL are rendered moot, and so removing them seems quite related. I suppose you could do the first and not the second, but then you would be leaving cruft behind.

Am still waiting for someone to say that this is tested.

And what is "my previous patch"? "emit uevent on auto release"?

Ayan George

2011-08-04 02:12 UTC #6 lore raw

In reply to Phillip Susi

Quoted 11 lines · Phillip Susi

Quoted 6 lines · Andrew Morton
The patch appears to do two unrelated things. That's generally frowned upon, but doesn't bother me much if the patch is small.
Still, splitting it into two patches (in which the bugfix is staged first) would be advantageous for people who might wish to backport the fix into earlier kernels.
It looks like this got stalled and someone emailed me asking what happened to it. I'm not sure that splitting the patch in two makes sense. I don't see how it does two unrelated things.

I agree -- I can't think of a meaningful way to split it up. I am stalled in testing it however.

I watched the values of lo->lo_device and bdev using systemtap and noticed cases where they could be out of sync which *might* make this patch invalid. I'd like a little more time to investigate.

-ayan