* [PATCH 0/2] block: loop: delete partitions after clearing & changing fd
@ 2020-07-07 8:45 Ming Lei
2020-07-07 8:45 ` [PATCH 1/2] block: loop: share code of reread partitions Ming Lei
2020-07-07 8:45 ` [PATCH 2/2] block: loop: delete partitions after clearing & changing fd Ming Lei
0 siblings, 2 replies; 7+ messages in thread
From: Ming Lei @ 2020-07-07 8:45 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig; +Cc: linux-block, Ming Lei
Hi,
The 1st patch cleans up __loop_clr_fd a bit.
The 2nd patch fixes one issue which may make ghost partitions even
though after fd is cleared or changed.
Ming Lei (2):
block: loop: share code of reread partitions
block: loop: delete partitions after clearing & changing fd
drivers/block/loop.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
--
2.25.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] block: loop: share code of reread partitions
2020-07-07 8:45 [PATCH 0/2] block: loop: delete partitions after clearing & changing fd Ming Lei
@ 2020-07-07 8:45 ` Ming Lei
2020-07-07 17:49 ` Christoph Hellwig
2020-07-07 8:45 ` [PATCH 2/2] block: loop: delete partitions after clearing & changing fd Ming Lei
1 sibling, 1 reply; 7+ messages in thread
From: Ming Lei @ 2020-07-07 8:45 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig; +Cc: linux-block, Ming Lei
loop_reread_partitions() has been there for rereading partitions, so
replace the open code in __loop_clr_fd() with loop_reread_partitions()
by passing 'locked' parameter.
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
drivers/block/loop.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index a943207705dd..0e08468b9ce0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -650,13 +650,17 @@ static inline void loop_update_dio(struct loop_device *lo)
}
static void loop_reread_partitions(struct loop_device *lo,
- struct block_device *bdev)
+ struct block_device *bdev, bool locked)
{
int rc;
- mutex_lock(&bdev->bd_mutex);
- rc = bdev_disk_changed(bdev, false);
- mutex_unlock(&bdev->bd_mutex);
+ if (locked) {
+ rc = bdev_disk_changed(bdev, false);
+ } else {
+ mutex_lock(&bdev->bd_mutex);
+ rc = bdev_disk_changed(bdev, false);
+ mutex_unlock(&bdev->bd_mutex);
+ }
if (rc)
pr_warn("%s: partition scan of loop%d (%s) failed (rc=%d)\n",
__func__, lo->lo_number, lo->lo_file_name, rc);
@@ -754,7 +758,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
*/
fput(old_file);
if (partscan)
- loop_reread_partitions(lo, bdev);
+ loop_reread_partitions(lo, bdev, false);
return 0;
out_err:
@@ -1179,7 +1183,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
bdgrab(bdev);
mutex_unlock(&loop_ctl_mutex);
if (partscan)
- loop_reread_partitions(lo, bdev);
+ loop_reread_partitions(lo, bdev, false);
if (claimed_bdev)
bd_abort_claiming(bdev, claimed_bdev, loop_configure);
return 0;
@@ -1270,16 +1274,7 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
* must be at least one and it can only become zero when the
* current holder is released.
*/
- if (!release)
- mutex_lock(&bdev->bd_mutex);
- err = bdev_disk_changed(bdev, false);
- if (!release)
- mutex_unlock(&bdev->bd_mutex);
- if (err)
- pr_warn("%s: partition scan of loop%d failed (rc=%d)\n",
- __func__, lo_number, err);
- /* Device is gone, no point in returning error */
- err = 0;
+ loop_reread_partitions(lo, bdev, release);
}
/*
@@ -1420,7 +1415,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
out_unlock:
mutex_unlock(&loop_ctl_mutex);
if (partscan)
- loop_reread_partitions(lo, bdev);
+ loop_reread_partitions(lo, bdev, false);
return err;
}
--
2.25.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] block: loop: delete partitions after clearing & changing fd
2020-07-07 8:45 [PATCH 0/2] block: loop: delete partitions after clearing & changing fd Ming Lei
2020-07-07 8:45 ` [PATCH 1/2] block: loop: share code of reread partitions Ming Lei
@ 2020-07-07 8:45 ` Ming Lei
2020-07-07 17:53 ` Christoph Hellwig
1 sibling, 1 reply; 7+ messages in thread
From: Ming Lei @ 2020-07-07 8:45 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig; +Cc: linux-block, Ming Lei
After clearing fd or changing fd, we have to delete old partitions,
otherwise they may become ghost partitions.
Fix this issue by clearing GENHD_FL_NO_PART_SCAN during calling
bdev_disk_changed() which won't drop old partitions if GENHD_FL_NO_PART_SCAN
isn't set.
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
drivers/block/loop.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 0e08468b9ce0..cf71a1bbcd45 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -650,17 +650,26 @@ static inline void loop_update_dio(struct loop_device *lo)
}
static void loop_reread_partitions(struct loop_device *lo,
- struct block_device *bdev, bool locked)
+ struct block_device *bdev, bool locked,
+ bool force_scan)
{
int rc;
+ bool no_scan;
- if (locked) {
- rc = bdev_disk_changed(bdev, false);
- } else {
+ if (!locked)
mutex_lock(&bdev->bd_mutex);
- rc = bdev_disk_changed(bdev, false);
+
+ no_scan = lo->lo_disk->flags & GENHD_FL_NO_PART_SCAN;
+ if (force_scan && no_scan)
+ lo->lo_disk->flags &= ~GENHD_FL_NO_PART_SCAN;
+
+ rc = bdev_disk_changed(bdev, false);
+
+ if (force_scan && no_scan)
+ lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
+
+ if (!locked)
mutex_unlock(&bdev->bd_mutex);
- }
if (rc)
pr_warn("%s: partition scan of loop%d (%s) failed (rc=%d)\n",
__func__, lo->lo_number, lo->lo_file_name, rc);
@@ -758,7 +767,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
*/
fput(old_file);
if (partscan)
- loop_reread_partitions(lo, bdev, false);
+ loop_reread_partitions(lo, bdev, false, true);
return 0;
out_err:
@@ -1183,7 +1192,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
bdgrab(bdev);
mutex_unlock(&loop_ctl_mutex);
if (partscan)
- loop_reread_partitions(lo, bdev, false);
+ loop_reread_partitions(lo, bdev, false, false);
if (claimed_bdev)
bd_abort_claiming(bdev, claimed_bdev, loop_configure);
return 0;
@@ -1274,7 +1283,7 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
* must be at least one and it can only become zero when the
* current holder is released.
*/
- loop_reread_partitions(lo, bdev, release);
+ loop_reread_partitions(lo, bdev, release, true);
}
/*
@@ -1415,7 +1424,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
out_unlock:
mutex_unlock(&loop_ctl_mutex);
if (partscan)
- loop_reread_partitions(lo, bdev, false);
+ loop_reread_partitions(lo, bdev, false, false);
return err;
}
--
2.25.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] block: loop: share code of reread partitions
2020-07-07 8:45 ` [PATCH 1/2] block: loop: share code of reread partitions Ming Lei
@ 2020-07-07 17:49 ` Christoph Hellwig
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2020-07-07 17:49 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, Christoph Hellwig, linux-block
On Tue, Jul 07, 2020 at 04:45:51PM +0800, Ming Lei wrote:
> loop_reread_partitions() has been there for rereading partitions, so
> replace the open code in __loop_clr_fd() with loop_reread_partitions()
> by passing 'locked' parameter.
>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> drivers/block/loop.c | 29 ++++++++++++-----------------
> 1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index a943207705dd..0e08468b9ce0 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -650,13 +650,17 @@ static inline void loop_update_dio(struct loop_device *lo)
> }
>
> static void loop_reread_partitions(struct loop_device *lo,
> - struct block_device *bdev)
> + struct block_device *bdev, bool locked)
> {
> int rc;
>
> - mutex_lock(&bdev->bd_mutex);
> - rc = bdev_disk_changed(bdev, false);
> - mutex_unlock(&bdev->bd_mutex);
> + if (locked) {
> + rc = bdev_disk_changed(bdev, false);
> + } else {
> + mutex_lock(&bdev->bd_mutex);
> + rc = bdev_disk_changed(bdev, false);
> + mutex_unlock(&bdev->bd_mutex);
> + }
functions with an argument based locking context are a really bad
idea. And there is absolutely no reason to add them just for
a shared printk.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] block: loop: delete partitions after clearing & changing fd
2020-07-07 8:45 ` [PATCH 2/2] block: loop: delete partitions after clearing & changing fd Ming Lei
@ 2020-07-07 17:53 ` Christoph Hellwig
2020-07-08 9:13 ` Ming Lei
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2020-07-07 17:53 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, Christoph Hellwig, linux-block
On Tue, Jul 07, 2020 at 04:45:52PM +0800, Ming Lei wrote:
> After clearing fd or changing fd, we have to delete old partitions,
> otherwise they may become ghost partitions.
>
> Fix this issue by clearing GENHD_FL_NO_PART_SCAN during calling
> bdev_disk_changed() which won't drop old partitions if GENHD_FL_NO_PART_SCAN
> isn't set.
I don't think messing with GENHD_FL_NO_PART_SCAN is a good idea, as
that will also cause an actual partition scan. But except for historic
reasons I can't think of a good idea to even check for
GENHD_FL_NO_PART_SCAN in blk_drop_partitions.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] block: loop: delete partitions after clearing & changing fd
2020-07-07 17:53 ` Christoph Hellwig
@ 2020-07-08 9:13 ` Ming Lei
2020-07-08 9:52 ` Ming Lei
0 siblings, 1 reply; 7+ messages in thread
From: Ming Lei @ 2020-07-08 9:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block
On Tue, Jul 07, 2020 at 07:53:12PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 07, 2020 at 04:45:52PM +0800, Ming Lei wrote:
> > After clearing fd or changing fd, we have to delete old partitions,
> > otherwise they may become ghost partitions.
> >
> > Fix this issue by clearing GENHD_FL_NO_PART_SCAN during calling
> > bdev_disk_changed() which won't drop old partitions if GENHD_FL_NO_PART_SCAN
> > isn't set.
>
> I don't think messing with GENHD_FL_NO_PART_SCAN is a good idea, as
> that will also cause an actual partition scan. But except for historic
> reasons I can't think of a good idea to even check for
> GENHD_FL_NO_PART_SCAN in blk_drop_partitions.
I think it is safe to not check it in blk_drop_partitions(), how about
the following patch?
From a20209464c367c338beee5555f2cb5c8e8ad9f78 Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Wed, 8 Jul 2020 16:07:19 +0800
Subject: [PATCH] block: always remove partitions in blk_drop_partitions()
So far blk_drop_partitions() only removes partitions when
disk_part_scan_enabled() return true. This way can make ghost partition on
loop device after changing/clearing FD in case that PARTSCAN is disabled.
Fix this issue by always removing partitions in blk_drop_partitions(), and
this way is correct because:
1) only loop, mmc and GENHD_FL_HIDDEN disks(nvme multipath) may set
GENHD_FL_NO_PART_SCAN
2) GENHD_FL_HIDDEN disks doesn't expose disk to block device fs, and
bdev_disk_changed()/blk_drop_partitions() won't be called for this kind of
disk
3) for mmc, if GENHD_FL_NO_PART_SCAN is set, no any partitions can be added
for this kind of disk, so blk_drop_partitions() basically does nothing no
matter if GENHD_FL_NO_PART_SCAN is set or not because disk_max_parts(disk) <= 1
4) for loop, bdev_disk_changed() is called in two cases: one is set fd and set
status, when there shouldn't be any partitions; another is clearing/changing fd,
we need to remove old partitions and re-read new partitions if there are and
PART_SCAN is enabled.
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/partitions/core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/block/partitions/core.c b/block/partitions/core.c
index 78951e33b2d7..e62a98a8eeb7 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -619,8 +619,6 @@ int blk_drop_partitions(struct block_device *bdev)
struct disk_part_iter piter;
struct hd_struct *part;
- if (!disk_part_scan_enabled(bdev->bd_disk))
- return 0;
if (bdev->bd_part_count)
return -EBUSY;
--
2.25.2
thanks,
Ming
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] block: loop: delete partitions after clearing & changing fd
2020-07-08 9:13 ` Ming Lei
@ 2020-07-08 9:52 ` Ming Lei
0 siblings, 0 replies; 7+ messages in thread
From: Ming Lei @ 2020-07-08 9:52 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block
On Wed, Jul 08, 2020 at 05:13:18PM +0800, Ming Lei wrote:
> On Tue, Jul 07, 2020 at 07:53:12PM +0200, Christoph Hellwig wrote:
> > On Tue, Jul 07, 2020 at 04:45:52PM +0800, Ming Lei wrote:
> > > After clearing fd or changing fd, we have to delete old partitions,
> > > otherwise they may become ghost partitions.
> > >
> > > Fix this issue by clearing GENHD_FL_NO_PART_SCAN during calling
> > > bdev_disk_changed() which won't drop old partitions if GENHD_FL_NO_PART_SCAN
> > > isn't set.
> >
> > I don't think messing with GENHD_FL_NO_PART_SCAN is a good idea, as
> > that will also cause an actual partition scan. But except for historic
> > reasons I can't think of a good idea to even check for
> > GENHD_FL_NO_PART_SCAN in blk_drop_partitions.
>
> I think it is safe to not check it in blk_drop_partitions(), how about
> the following patch?
>
> From a20209464c367c338beee5555f2cb5c8e8ad9f78 Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@redhat.com>
> Date: Wed, 8 Jul 2020 16:07:19 +0800
> Subject: [PATCH] block: always remove partitions in blk_drop_partitions()
>
> So far blk_drop_partitions() only removes partitions when
> disk_part_scan_enabled() return true. This way can make ghost partition on
> loop device after changing/clearing FD in case that PARTSCAN is disabled.
>
> Fix this issue by always removing partitions in blk_drop_partitions(), and
> this way is correct because:
>
> 1) only loop, mmc and GENHD_FL_HIDDEN disks(nvme multipath) may set
> GENHD_FL_NO_PART_SCAN
>
> 2) GENHD_FL_HIDDEN disks doesn't expose disk to block device fs, and
> bdev_disk_changed()/blk_drop_partitions() won't be called for this kind of
> disk
>
> 3) for mmc, if GENHD_FL_NO_PART_SCAN is set, no any partitions can be added
> for this kind of disk, so blk_drop_partitions() basically does nothing no
> matter if GENHD_FL_NO_PART_SCAN is set or not because disk_max_parts(disk) <= 1
>
> 4) for loop, bdev_disk_changed() is called in two cases: one is set fd and set
> status, when there shouldn't be any partitions; another is clearing/changing fd,
> we need to remove old partitions and re-read new partitions if there are and
> PART_SCAN is enabled.
BTW, the partitions shouldn't have been added, but ioctl(BLKPG, BLKPG_ADD_PARTITION)
doesn't check GENHD_FL_NO_PART_SCAN, so the partitions are added.
However, changing ioctl(BLKPG, BLKPG_ADD_PARTITION) might break some userspace.
Thanks,
Ming
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-08 9:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 8:45 [PATCH 0/2] block: loop: delete partitions after clearing & changing fd Ming Lei
2020-07-07 8:45 ` [PATCH 1/2] block: loop: share code of reread partitions Ming Lei
2020-07-07 17:49 ` Christoph Hellwig
2020-07-07 8:45 ` [PATCH 2/2] block: loop: delete partitions after clearing & changing fd Ming Lei
2020-07-07 17:53 ` Christoph Hellwig
2020-07-08 9:13 ` Ming Lei
2020-07-08 9:52 ` Ming Lei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).