| Message ID | 20200707084552.3294693-3-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: loop: delete partitions after clearing & changing fd | expand |
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.
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;
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
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; }
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(-)