[PATCH] loop: cleanup partitions when detaching loop device

[PATCH] loop: cleanup partitions when detaching loop device

[Phillip Susi]

Any partitions added by user space to the loop device were being
left in place after detaching the loop device.  This was because
the detach path issued a BLKRRPART to clean up partitions if
LO_FLAGS_PARTSCAN was set, meaning that the partitions were auto
scanned on attach.  Replace this BLKRRPART with code that
unconditionally cleans up partitions on detach instead.

Signed-off-by: Phillip Susi <psusi@cfl.rr.com>
---
 drivers/block/loop.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 68b205a..49a5eb7 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1016,8 +1016,19 @@ static int loop_clr_fd(struct loop_device *lo)
 	lo->lo_state = Lo_unbound;
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
-	if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev)
-		ioctl_by_bdev(bdev, BLKRRPART, 0);
+	if (bdev)
+	{
+		struct disk_part_iter piter;
+		struct hd_struct *part;
+
+		mutex_lock(&bdev->bd_mutex);
+		invalidate_partition(bdev->bd_disk, 0);
+		disk_part_iter_init(&piter, bdev->bd_disk, DISK_PITER_INCL_EMPTY);
+		while ((part = disk_part_iter_next(&piter)))
+			delete_partition(bdev->bd_disk, part->partno);
+		disk_part_iter_exit(&piter);
+		mutex_unlock(&bdev->bd_mutex);
+	}
 	lo->lo_flags = 0;
 	if (!part_shift)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
-- 
1.7.5.4

[PATCH] loop: cleanup partitions when detaching loop device

[Phillip Susi]

Any partitions added by user space to the loop device were being
left in place after detaching the loop device.  This was because
the detach path issued a BLKRRPART to clean up partitions if
LO_FLAGS_PARTSCAN was set, meaning that the partitions were auto
scanned on attach.  Replace this BLKRRPART with code that
unconditionally cleans up partitions on detach instead.

Signed-off-by: Phillip Susi <psusi@ubuntu.com>
---
 drivers/block/loop.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ae12512..38f0239 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1039,12 +1039,24 @@ static int loop_clr_fd(struct loop_device *lo)
 	lo->lo_state = Lo_unbound;
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
-	if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev)
-		ioctl_by_bdev(bdev, BLKRRPART, 0);
 	lo->lo_flags = 0;
 	if (!part_shift)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
 	mutex_unlock(&lo->lo_ctl_mutex);
+	if (bdev)
+	{
+		struct disk_part_iter piter;
+		struct hd_struct *part;
+
+		mutex_lock_nested(&bdev->bd_mutex, 1);
+		invalidate_partition(bdev->bd_disk, 0);
+		disk_part_iter_init(&piter, bdev->bd_disk, DISK_PITER_INCL_EMPTY);
+		while ((part = disk_part_iter_next(&piter)))
+			delete_partition(bdev->bd_disk, part->partno);
+		disk_part_iter_exit(&piter);
+		mutex_unlock(&bdev->bd_mutex);
+	}
+
 	/*
 	 * Need not hold lo_ctl_mutex to fput backing file.
 	 * Calling fput holding lo_ctl_mutex triggers a circular
-- 
1.8.1.2

Re: [PATCH] loop: cleanup partitions when detaching loop device

[Andrew Morton]

On Sun,  3 Mar 2013 13:49:13 -0500 Phillip Susi <psusi@ubuntu.com> wrote:

> Any partitions added by user space to the loop device were being
> left in place after detaching the loop device.  This was because
> the detach path issued a BLKRRPART to clean up partitions if
> LO_FLAGS_PARTSCAN was set, meaning that the partitions were auto
> scanned on attach.  Replace this BLKRRPART with code that
> unconditionally cleans up partitions on detach instead.

huh.  What is the user-visible effect of this bug?  Just a memory leak
or something more serious?

If "something more serious", why did this problem remain hidden for so
long?

> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1039,12 +1039,24 @@ static int loop_clr_fd(struct loop_device *lo)
>  	lo->lo_state = Lo_unbound;
>  	/* This is safe: open() is still holding a reference. */
>  	module_put(THIS_MODULE);
> -	if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev)
> -		ioctl_by_bdev(bdev, BLKRRPART, 0);
>  	lo->lo_flags = 0;
>  	if (!part_shift)
>  		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
>  	mutex_unlock(&lo->lo_ctl_mutex);
> +	if (bdev)
> +	{

scripts/checkpatch.pl is your friend.

Can you please suggest a code comment which we can slip in here to tell
readers what's going on and why we're doing this?

> +		struct disk_part_iter piter;
> +		struct hd_struct *part;
> +
> +		mutex_lock_nested(&bdev->bd_mutex, 1);
> +		invalidate_partition(bdev->bd_disk, 0);
> +		disk_part_iter_init(&piter, bdev->bd_disk, DISK_PITER_INCL_EMPTY);
> +		while ((part = disk_part_iter_next(&piter)))
> +			delete_partition(bdev->bd_disk, part->partno);
> +		disk_part_iter_exit(&piter);
> +		mutex_unlock(&bdev->bd_mutex);
> +	}

Re: [PATCH] loop: cleanup partitions when detaching loop device

[Phillip Susi]

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

On 03/14/2013 06:07 PM, Andrew Morton wrote:
> huh.  What is the user-visible effect of this bug?  Just a memory
> leak or something more serious?

Not serious, but user-visible in that the partition devnodes still
show up after detaching the backing file, and I think the partitions
remained in place after attaching a new file even though it had
different or no partitions at all.

> scripts/checkpatch.pl is your friend.

Oops.  I see you pushed a patch fixing up the brace position and
breaking the long line.  Did you want me to squash it and resubmit, or
just sign off on it?  If the latter, consider it signed off.

> Can you please suggest a code comment which we can slip in here to
> tell readers what's going on and why we're doing this?

How about "Remove all partitions, since BLKRRPART won't remove user
added partitions when max_part=0"?

>> +		struct disk_part_iter piter; +		struct hd_struct *part; + +
>> mutex_lock_nested(&bdev->bd_mutex, 1); +
>> invalidate_partition(bdev->bd_disk, 0); +
>> disk_part_iter_init(&piter, bdev->bd_disk,
>> DISK_PITER_INCL_EMPTY); +		while ((part =
>> disk_part_iter_next(&piter))) +			delete_partition(bdev->bd_disk,
>> part->partno); +		disk_part_iter_exit(&piter); +
>> mutex_unlock(&bdev->bd_mutex); +	}
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iQEcBAEBAgAGBQJRQn1XAAoJEJrBOlT6nu75QIEIAIM4skpZoKWYzAYXCK84JjP2
7Z0toyWfpQ63ku7grFPMST8wpGrsq31zcwa7zvya2Tg0ivi0vHOZmw0QBVic3mw0
Ce88iVkCDQSoASwPdHRLwNLj7Lj/cvHkwqZIHbDXR5u15v1sr3NTrCDMP33kZlPi
Z8TbX+3fTTMYriYXUOI8fmqnC+d1gj8w+fsNNAB23/mVgN6ed9uGMQqEQjXGQOnQ
By9R7dD5SZ9hqstLBSCvochBVfNjWm2rMtKTnixcY1qTDGCLWZ2sUpxns6WTjAZl
lWL/S80kt+sbpkQorXWWDZJAvyRhQny4703rwPRhzgMWPDY7qJkn465tDT4S/ic=
=3/XD
-----END PGP SIGNATURE-----