| Message ID | 20180104103924.8814-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 04, 2018 at 06:39:24PM +0800, Ming Lei wrote: > When detaching loop disk, neither we remove loop partitions, nor > invalidate them. This way may cause data loss, and often confuse > people. > > This patch fixes the above issue by removing & invalidating loop > partitions in loop_clr_fd(), Please add a comment explaining the call (e.g. the weird loop case of keeping unbound devices around). Also dasd seems to implement this functionality by iterating over all partitions and then calling ioctl_by_bdev(bdev, BLKPG, ..) on it. Please switch it to you new helper assuming there is a good reason to even do this to start with. CCing the maintainers to confirm that. And while you'e at it split the block and loop bits into separate patches. > +void del_gendisk_partitions(struct gendisk *disk) Add a comment explanining the function, please. > struct disk_part_iter piter; > struct hd_struct *part; > > - blk_integrity_del(disk); > - disk_del_events(disk); > - > /* invalidate stuff */ > disk_part_iter_init(&piter, disk, > DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE); > @@ -714,7 +711,17 @@ void del_gendisk(struct gendisk *disk) > disk_part_iter_exit(&piter); > > invalidate_partition(disk, 0); > +} > +EXPORT_SYMBOL(del_gendisk_partitions); EXPORT_SYMBOL_GPL, please.
On Thu, Jan 04, 2018 at 04:38:03AM -0800, Christoph Hellwig wrote: > On Thu, Jan 04, 2018 at 06:39:24PM +0800, Ming Lei wrote: > > When detaching loop disk, neither we remove loop partitions, nor > > invalidate them. This way may cause data loss, and often confuse > > people. > > > > This patch fixes the above issue by removing & invalidating loop > > partitions in loop_clr_fd(), > > Please add a comment explaining the call (e.g. the weird loop > case of keeping unbound devices around). OK. > > Also dasd seems to implement this functionality by iterating over > all partitions and then calling ioctl_by_bdev(bdev, BLKPG, ..) on > it. Please switch it to you new helper assuming there is a good > reason to even do this to start with. CCing the maintainers to > confirm that. This patch doesn't consider to hold bdev lock, and loop's case can be a bit complicated since loop_clr_fd() can be called in release path. I will investigate a bit if it is possible to figure out a general helper to cover both. > > And while you'e at it split the block and loop bits into separate > patches. OK. > > > +void del_gendisk_partitions(struct gendisk *disk) > > Add a comment explanining the function, please. OK. > > > struct disk_part_iter piter; > > struct hd_struct *part; > > > > - blk_integrity_del(disk); > > - disk_del_events(disk); > > - > > /* invalidate stuff */ > > disk_part_iter_init(&piter, disk, > > DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE); > > @@ -714,7 +711,17 @@ void del_gendisk(struct gendisk *disk) > > disk_part_iter_exit(&piter); > > > > invalidate_partition(disk, 0); > > +} > > +EXPORT_SYMBOL(del_gendisk_partitions); > > EXPORT_SYMBOL_GPL, please. OK.
diff --git a/block/genhd.c b/block/genhd.c index 96a66f671720..24c66e940c0c 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -695,14 +695,11 @@ void device_add_disk(struct device *parent, struct gendisk *disk) } EXPORT_SYMBOL(device_add_disk); -void del_gendisk(struct gendisk *disk) +void del_gendisk_partitions(struct gendisk *disk) { struct disk_part_iter piter; struct hd_struct *part; - blk_integrity_del(disk); - disk_del_events(disk); - /* invalidate stuff */ disk_part_iter_init(&piter, disk, DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE); @@ -714,7 +711,17 @@ void del_gendisk(struct gendisk *disk) disk_part_iter_exit(&piter); invalidate_partition(disk, 0); +} +EXPORT_SYMBOL(del_gendisk_partitions); + +void del_gendisk(struct gendisk *disk) +{ + blk_integrity_del(disk); + disk_del_events(disk); + + del_gendisk_partitions(disk); bdev_unhash_inode(disk_devt(disk)); + set_capacity(disk, 0); disk->flags &= ~GENHD_FL_UP; diff --git a/drivers/block/loop.c b/drivers/block/loop.c index bc8e61506968..a84c7befcc94 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1021,6 +1021,8 @@ static int loop_clr_fd(struct loop_device *lo) if (filp == NULL) return -EINVAL; + del_gendisk_partitions(lo->lo_disk); + /* freeze request queue during the transition */ blk_mq_freeze_queue(lo->lo_queue); @@ -1060,8 +1062,6 @@ static int loop_clr_fd(struct loop_device *lo) module_put(THIS_MODULE); blk_mq_unfreeze_queue(lo->lo_queue); - if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev) - loop_reread_partitions(lo, bdev); lo->lo_flags = 0; if (!part_shift) lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 5144ebe046c9..ae41535e705c 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -397,6 +397,7 @@ static inline void add_disk(struct gendisk *disk) } extern void del_gendisk(struct gendisk *gp); +extern void del_gendisk_partitions(struct gendisk *gp); extern struct gendisk *get_gendisk(dev_t dev, int *partno); extern struct block_device *bdget_disk(struct gendisk *disk, int partno);
When detaching loop disk, neither we remove loop partitions, nor invalidate them. This way may cause data loss, and often confuse people. This patch fixes the above issue by removing & invalidating loop partitions in loop_clr_fd(), Reported-by: reddot.rocks@gmail.com Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/genhd.c | 15 +++++++++++---- drivers/block/loop.c | 4 ++-- include/linux/genhd.h | 1 + 3 files changed, 14 insertions(+), 6 deletions(-)