Re: loop device auto release patch

Re: loop device auto release patch

[Phillip Susi]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

On 7/14/2011 2:56 PM, Ayan George wrote:
>
> 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.


[-- Attachment #2: 0001-Always-invalidate-cleared-loop-block-devices.patch --]
[-- Type: text/x-patch, Size: 2920 bytes --]

>From c783ba6a26eff42d3cb0061e4fcb8ee8a16b3e67 Mon Sep 17 00:00:00 2001
From: Ayan George <ayan.george@canonical.com>
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 <ayan.george@canonical.com>
---
 drivers/block/loop.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 76c8da7..a40e12b 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -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 {
-- 
1.7.4.1

Re: loop device auto release patch

[Andrew Morton]

On Fri, 15 Jul 2011 15:12:50 -0400
Phillip Susi <psusi@cfl.rr.com> wrote:

> On 7/14/2011 2:56 PM, Ayan George wrote:
> >
> > 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.
 
> 
> [0001-Always-invalidate-cleared-loop-block-devices.patch  text/x-patch (2.9KB)]
> >From c783ba6a26eff42d3cb0061e4fcb8ee8a16b3e67 Mon Sep 17 00:00:00 2001
> From: Ayan George <ayan.george@canonical.com>
> 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.

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.

Re: loop device auto release patch

[Ayan George]

On 07/15/2011 03:19 PM, Andrew Morton wrote:
> 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

Re: loop device auto release patch

[Phillip Susi]

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

On 07/15/2011 03:19 PM, Andrew Morton wrote:
> 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-----

Re: loop device auto release patch

[Andrew Morton]

On Wed, 03 Aug 2011 21:58:24 -0400 Phillip Susi <psusi@cfl.rr.com> wrote:

> On 07/15/2011 03:19 PM, Andrew Morton wrote:
> > 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.

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

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

Re: loop device auto release patch

[Ayan George]

Phillip Susi <psusi@cfl.rr.com> wrote:
> On 07/15/2011 03:19 PM, Andrew Morton wrote:
> > 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