| 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
On Fri, Apr 04, 2025 at 08:06:05PM +0800, Ming Lei wrote: > 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. What are "internal settings" please? If you change the loop backing file outstanding I/O is relevant. If you change NVMe ANA or retry policies are relevant as they are checked in the I/O completion handler. > > 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 What is the typical queue quiesce use case? Why is it "typical" and why is it safe. > > 2) loop specific setting is only visible in loop queue_rq() & workfn, and > quiesce does provide the sync for queue_rq() _What_ loop specific setting. > 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. I am actually much more worried about quiesce. It is much less well defined.
On Mon, Apr 07, 2025 at 08:48:06AM +0200, Christoph Hellwig wrote: > On Fri, Apr 04, 2025 at 08:06:05PM +0800, Ming Lei wrote: > > 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. > > What are "internal settings" please? If you change the loop backing > file outstanding I/O is relevant. If you change NVMe ANA or retry > policies are relevant as they are checked in the I/O completion handler. internal setting means the driver internal setting, which is only visible for driver. Here this setting(lo_offset, backing file, dio, ...) won't be used in completion handler, so it is fine to use quiesce here for updating these loop specific setting. > > > > 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 > > What is the typical queue quiesce use case? Why is it "typical" and > why is it safe. typical quiesce provides sync with driver's ->queue_rq(), and it is typical that these driver settings are only used in driver submission code path. > > > > > 2) loop specific setting is only visible in loop queue_rq() & workfn, and > > quiesce does provide the sync for queue_rq() > > _What_ loop specific setting. Any setting is only visible in loop driver, such lo_offset, backing file, dio, ... > > > 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. > > I am actually much more worried about quiesce. It is much less well > defined. It is widely used, and please see document of blk_mq_quiesce_queue(). Thanks, Ming
On Mon, Apr 07, 2025 at 10:54:01PM +0800, Ming Lei wrote: > > What are "internal settings" please? If you change the loop backing > > file outstanding I/O is relevant. If you change NVMe ANA or retry > > policies are relevant as they are checked in the I/O completion handler. > > internal setting means the driver internal setting, which is only visible > for driver. > > Here this setting(lo_offset, backing file, dio, ...) won't be used in > completion handler, so it is fine to use quiesce here for updating > these loop specific setting. The backing file is used during I/O. So you certainly can't change it while I/O is in flight. But the important point is that there is nothing inherent about "internal" attributes needing different synchronization. Maybe some field don't need a full I/O quiesce, but you need to explain that for each and every single one of them. > > > > > > > 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 > > > > What is the typical queue quiesce use case? Why is it "typical" and > > why is it safe. > > typical quiesce provides sync with driver's ->queue_rq(), and it is typical > that these driver settings are only used in driver submission code path. "typical" does not matter. The required synchronization needs to be provided even for non-typical use cases. > > > 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. > > > > I am actually much more worried about quiesce. It is much less well > > defined. > > It is widely used, and please see document of blk_mq_quiesce_queue(). I did not say it isn't widely used. Which is part of the problem.
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(-)