| Message ID | 20250403105414.1334254-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | loop: replace freezing queue with quiesce when changing loop specific setting | expand |
在 2025/4/3 12:54, Ming Lei 写道: > freeze queue should be used for changing block layer generic setting, such > as logical block size, PI, ..., and it is enough to quiesce queue for > changing loop specific setting. > > Remove the queue freeze warning in loop_update_dio(), since it is only > needed in loop_set_block_size(), where queue is froze obviously. > > Fix reported lockdep by syszbot: > > https://lore.kernel.org/linux-block/67ea99e0.050a0220.3c3d88.0042.GAE@google.com/ > > Reported-by: syzbot+9dd7dbb1a4b915dee638@syzkaller.appspotmail.com > Signed-off-by: Ming Lei <ming.lei@redhat.com> I am fine with this commit. Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> Zhu Yanjun > --- > drivers/block/loop.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 674527d770dc..59886556f55c 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -190,8 +190,6 @@ static bool lo_can_use_dio(struct loop_device *lo) > static inline void loop_update_dio(struct loop_device *lo) > { > lockdep_assert_held(&lo->lo_mutex); > - WARN_ON_ONCE(lo->lo_state == Lo_bound && > - lo->lo_queue->mq_freeze_depth == 0); > > if ((lo->lo_flags & LO_FLAGS_DIRECT_IO) && !lo_can_use_dio(lo)) > lo->lo_flags &= ~LO_FLAGS_DIRECT_IO; > @@ -595,7 +593,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, > { > struct file *file = fget(arg); > struct file *old_file; > - unsigned int memflags; > int error; > bool partscan; > bool is_loop; > @@ -640,11 +637,11 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, > > /* and ... switch */ > disk_force_media_change(lo->lo_disk); > - memflags = blk_mq_freeze_queue(lo->lo_queue); > + blk_mq_quiesce_queue(lo->lo_queue); > mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask); > loop_assign_backing_file(lo, file); > loop_update_dio(lo); > - blk_mq_unfreeze_queue(lo->lo_queue, memflags); > + blk_mq_unquiesce_queue(lo->lo_queue); > partscan = lo->lo_flags & LO_FLAGS_PARTSCAN; > loop_global_unlock(lo, is_loop); > > @@ -1270,7 +1267,6 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) > int err; > bool partscan = false; > bool size_changed = false; > - unsigned int memflags; > > err = mutex_lock_killable(&lo->lo_mutex); > if (err) > @@ -1287,8 +1283,8 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) > invalidate_bdev(lo->lo_device); > } > > - /* I/O needs to be drained before changing lo_offset or lo_sizelimit */ > - memflags = blk_mq_freeze_queue(lo->lo_queue); > + /* queue needs to be quiesced before changing lo_offset or lo_sizelimit */ > + blk_mq_quiesce_queue(lo->lo_queue); > > err = loop_set_status_from_info(lo, info); > if (err) > @@ -1310,7 +1306,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) > loop_update_dio(lo); > > out_unfreeze: > - blk_mq_unfreeze_queue(lo->lo_queue, memflags); > + blk_mq_unquiesce_queue(lo->lo_queue); > if (partscan) > clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state); > out_unlock:
On Thu, Apr 03, 2025 at 06:54:14PM +0800, Ming Lei wrote: > freeze queue should be used for changing block layer generic setting, such > as logical block size, PI, ..., and it is enough to quiesce queue for > changing loop specific setting. Why? A queue should generally be frozen for any setting affecting the I/O path. Nothing about generic or internal. This also misses an explanation of what setting this protects and why you think this is safe and the sound fix.
On Fri, Apr 04, 2025 at 11:11:49AM +0200, Christoph Hellwig wrote: > On Thu, Apr 03, 2025 at 06:54:14PM +0800, Ming Lei wrote: > > freeze queue should be used for changing block layer generic setting, such > > as logical block size, PI, ..., and it is enough to quiesce queue for > > changing loop specific setting. > > Why? A queue should generally be frozen for any setting affecting the > I/O path. Nothing about generic or internal. For any driver specific setting, quiesce is enough, because these settings are only visible in driver IO code path, quiesce does provide the required protection exactly. > > This also misses an explanation of what setting this protects and why > you think this is safe and the sound fix. 1) it is typical queue quiesce use case 2) loop specific setting is only visible in loop queue_rq() & workfn, and quiesce does provide the sync for queue_rq() 3) for driver, quiesce is always preferred over freeze, and freeze is easily mis-used by driver, you know we have bad driver uses for freeze. However, for loop, quiesce only isn't enough, we need to flush the workqueue for avoiding workfn to use the changed setting too, will do it in V2. Thanks, Ming
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 674527d770dc..59886556f55c 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -190,8 +190,6 @@ static bool lo_can_use_dio(struct loop_device *lo) static inline void loop_update_dio(struct loop_device *lo) { lockdep_assert_held(&lo->lo_mutex); - WARN_ON_ONCE(lo->lo_state == Lo_bound && - lo->lo_queue->mq_freeze_depth == 0); if ((lo->lo_flags & LO_FLAGS_DIRECT_IO) && !lo_can_use_dio(lo)) lo->lo_flags &= ~LO_FLAGS_DIRECT_IO; @@ -595,7 +593,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, { struct file *file = fget(arg); struct file *old_file; - unsigned int memflags; int error; bool partscan; bool is_loop; @@ -640,11 +637,11 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, /* and ... switch */ disk_force_media_change(lo->lo_disk); - memflags = blk_mq_freeze_queue(lo->lo_queue); + blk_mq_quiesce_queue(lo->lo_queue); mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask); loop_assign_backing_file(lo, file); loop_update_dio(lo); - blk_mq_unfreeze_queue(lo->lo_queue, memflags); + blk_mq_unquiesce_queue(lo->lo_queue); partscan = lo->lo_flags & LO_FLAGS_PARTSCAN; loop_global_unlock(lo, is_loop); @@ -1270,7 +1267,6 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) int err; bool partscan = false; bool size_changed = false; - unsigned int memflags; err = mutex_lock_killable(&lo->lo_mutex); if (err) @@ -1287,8 +1283,8 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) invalidate_bdev(lo->lo_device); } - /* I/O needs to be drained before changing lo_offset or lo_sizelimit */ - memflags = blk_mq_freeze_queue(lo->lo_queue); + /* queue needs to be quiesced before changing lo_offset or lo_sizelimit */ + blk_mq_quiesce_queue(lo->lo_queue); err = loop_set_status_from_info(lo, info); if (err) @@ -1310,7 +1306,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) loop_update_dio(lo); out_unfreeze: - blk_mq_unfreeze_queue(lo->lo_queue, memflags); + blk_mq_unquiesce_queue(lo->lo_queue); if (partscan) clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state); out_unlock:
freeze queue should be used for changing block layer generic setting, such as logical block size, PI, ..., and it is enough to quiesce queue for changing loop specific setting. Remove the queue freeze warning in loop_update_dio(), since it is only needed in loop_set_block_size(), where queue is froze obviously. Fix reported lockdep by syszbot: https://lore.kernel.org/linux-block/67ea99e0.050a0220.3c3d88.0042.GAE@google.com/ Reported-by: syzbot+9dd7dbb1a4b915dee638@syzkaller.appspotmail.com Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/block/loop.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)