* [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
* 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
* [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 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).