* [PATCH v2] loop: defer the queue limits clear to a workqueue
@ 2026-09-02 6:34 Tao Cui
2026-09-02 6:36 ` Tao Cui
0 siblings, 1 reply; 3+ messages in thread
From: Tao Cui @ 2026-09-02 6:34 UTC (permalink / raw)
To: Bart Van Assche, axboe, hch, Tetsuo Handa
Cc: cui.tao, linux-block, linux-kernel, Tao Cui
From: Tao Cui <cuitao@kylinos.cn>
loop_clear_limits() calls queue_limits_commit_update() directly from
the loop workqueue that processes the request. That does a
non-atomic struct assignment to q->limits without freezing the queue,
which races with lockless readers of q->limits on other CPUs - bio
splitting reads max_hw_sectors, the discard path reads
max_hw_discard_sectors - and can let them observe torn values. The
trigger is a discard or write-zeroes request on a loop device whose
backing file does not support the corresponding fallocate operation.
The code already has an XXX comment saying this should move to a
workqueue. Do that: schedule a work item on the system workqueue,
where it is safe to freeze the queue and update the limits using
queue_limits_commit_update_frozen(). Accumulate pending modes in
lo->clear_limits_mode so that failures between scheduling and
execution of the work item are not lost, reset the accumulated
modes when a new backing file is assigned, and cancel the work
item before the device is freed.
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
Changes since v1:
- Make clear_limits_mode atomic_t: loop workers can hit the |=
concurrently.
- Reset clear_limits_mode when assigning a new backing file, so
stale modes do not clear limits of the new file.
- Cancel the work item from loop_remove() before del_gendisk():
the queue can already be in RCU-delayed freeing when
lo_free_disk() cancels it.
Tested on x86-64 (qemu, vfat-backed loop device): 30s discard and
reconfigure loop exercises the clear 577 times, no torn sysfs reads,
no difference against the unpatched kernel.
Link: https://lore.kernel.org/r/20260828072004.273519-1-cui.tao@linux.dev/
---
drivers/block/loop.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 6f12976035b0..368ea857895c 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -67,6 +67,8 @@ struct loop_device {
struct list_head rootcg_cmd_list;
struct list_head idle_worker_list;
struct rb_root worker_tree;
+ struct work_struct clear_limits_work;
+ atomic_t clear_limits_mode;
struct timer_list timer;
bool sysfs_inited;
@@ -222,9 +224,12 @@ static void loop_set_size(struct loop_device *lo, loff_t size)
kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
}
-static void loop_clear_limits(struct loop_device *lo, int mode)
+static void loop_clear_limits_workfn(struct work_struct *work)
{
+ struct loop_device *lo =
+ container_of(work, struct loop_device, clear_limits_work);
struct queue_limits lim = queue_limits_start_update(lo->lo_queue);
+ int mode = atomic_xchg(&lo->clear_limits_mode, 0);
if (mode & FALLOC_FL_ZERO_RANGE)
lim.max_write_zeroes_sectors = 0;
@@ -234,14 +239,13 @@ static void loop_clear_limits(struct loop_device *lo, int mode)
lim.discard_granularity = 0;
}
- /*
- * XXX: this updates the queue limits without freezing the queue, which
- * is against the locking protocol and dangerous. But we can't just
- * freeze the queue as we're inside the ->queue_rq method here. So this
- * should move out into a workqueue unless we get the file operations to
- * advertise if they support specific fallocate operations.
- */
- queue_limits_commit_update(lo->lo_queue, &lim);
+ queue_limits_commit_update_frozen(lo->lo_queue, &lim);
+}
+
+static void loop_clear_limits(struct loop_device *lo, int mode)
+{
+ atomic_or(mode, &lo->clear_limits_mode);
+ schedule_work(&lo->clear_limits_work);
}
static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
@@ -516,6 +520,7 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
{
lo->lo_backing_file = file;
+ atomic_set(&lo->clear_limits_mode, 0);
lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
mapping_set_gfp_mask(file->f_mapping,
lo->old_gfp_mask & ~(__GFP_IO | __GFP_FS));
@@ -1781,6 +1786,7 @@ static void lo_free_disk(struct gendisk *disk)
destroy_workqueue(lo->workqueue);
loop_free_idle_workers(lo, true);
timer_shutdown_sync(&lo->timer);
+ cancel_work_sync(&lo->clear_limits_work);
mutex_destroy(&lo->lo_mutex);
kfree(lo);
}
@@ -2100,6 +2106,7 @@ static int loop_add(int i)
spin_lock_init(&lo->lo_lock);
spin_lock_init(&lo->lo_work_lock);
INIT_WORK(&lo->rootcg_work, loop_rootcg_workfn);
+ INIT_WORK(&lo->clear_limits_work, loop_clear_limits_workfn);
INIT_LIST_HEAD(&lo->rootcg_cmd_list);
disk->major = LOOP_MAJOR;
disk->first_minor = i << part_shift;
@@ -2138,6 +2145,10 @@ static int loop_add(int i)
static void loop_remove(struct loop_device *lo)
{
+ /* Cancel early: the queue may be in RCU-delayed freeing
+ * by the time lo_free_disk() runs. */
+ cancel_work_sync(&lo->clear_limits_work);
+
/* Make this loop device unreachable from pathname. */
del_gendisk(lo->lo_disk);
blk_mq_free_tag_set(&lo->tag_set);
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] loop: defer the queue limits clear to a workqueue
2026-09-02 6:34 [PATCH v2] loop: defer the queue limits clear to a workqueue Tao Cui
@ 2026-09-02 6:36 ` Tao Cui
2026-09-02 9:26 ` Tao Cui
0 siblings, 1 reply; 3+ messages in thread
From: Tao Cui @ 2026-09-02 6:36 UTC (permalink / raw)
To: Bart Van Assche, axboe, hch, Tetsuo Handa
Cc: cui.tao, linux-block, linux-kernel, Tao Cui
Hi. Bart,
在 2026/9/2 14:34, Tao Cui 写道:
> From: Tao Cui <cuitao@kylinos.cn>
>
> loop_clear_limits() calls queue_limits_commit_update() directly from
> the loop workqueue that processes the request. That does a
> non-atomic struct assignment to q->limits without freezing the queue,
> which races with lockless readers of q->limits on other CPUs - bio
> splitting reads max_hw_sectors, the discard path reads
> max_hw_discard_sectors - and can let them observe torn values. The
> trigger is a discard or write-zeroes request on a loop device whose
> backing file does not support the corresponding fallocate operation.
>
> The code already has an XXX comment saying this should move to a
> workqueue. Do that: schedule a work item on the system workqueue,
> where it is safe to freeze the queue and update the limits using
> queue_limits_commit_update_frozen(). Accumulate pending modes in
> lo->clear_limits_mode so that failures between scheduling and
> execution of the work item are not lost, reset the accumulated
> modes when a new backing file is assigned, and cancel the work
> item before the device is freed.
>
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>
> ---
> Changes since v1:
>
> - Make clear_limits_mode atomic_t: loop workers can hit the |=
> concurrently.
>
> - Reset clear_limits_mode when assigning a new backing file, so
> stale modes do not clear limits of the new file.
>
> - Cancel the work item from loop_remove() before del_gendisk():
> the queue can already be in RCU-delayed freeing when
> lo_free_disk() cancels it.
>
> Tested on x86-64 (qemu, vfat-backed loop device): 30s discard and
> reconfigure loop exercises the clear 577 times, no torn sysfs reads,
> no difference against the unpatched kernel.
>
thanks for the Reviewed-by on v1.
v2 makes clear_limits_mode atomic, resets it on rebind, and
cancels the work item from loop_remove(). The code changed, so
could you take another look when you have time?
Thanks,
Tao
> Link: https://lore.kernel.org/r/20260828072004.273519-1-cui.tao@linux.dev/
> ---
> drivers/block/loop.c | 29 ++++++++++++++++++++---------
> 1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 6f12976035b0..368ea857895c 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -67,6 +67,8 @@ struct loop_device {
> struct list_head rootcg_cmd_list;
> struct list_head idle_worker_list;
> struct rb_root worker_tree;
> + struct work_struct clear_limits_work;
> + atomic_t clear_limits_mode;
> struct timer_list timer;
> bool sysfs_inited;
>
> @@ -222,9 +224,12 @@ static void loop_set_size(struct loop_device *lo, loff_t size)
> kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
> }
>
> -static void loop_clear_limits(struct loop_device *lo, int mode)
> +static void loop_clear_limits_workfn(struct work_struct *work)
> {
> + struct loop_device *lo =
> + container_of(work, struct loop_device, clear_limits_work);
> struct queue_limits lim = queue_limits_start_update(lo->lo_queue);
> + int mode = atomic_xchg(&lo->clear_limits_mode, 0);
>
> if (mode & FALLOC_FL_ZERO_RANGE)
> lim.max_write_zeroes_sectors = 0;
> @@ -234,14 +239,13 @@ static void loop_clear_limits(struct loop_device *lo, int mode)
> lim.discard_granularity = 0;
> }
>
> - /*
> - * XXX: this updates the queue limits without freezing the queue, which
> - * is against the locking protocol and dangerous. But we can't just
> - * freeze the queue as we're inside the ->queue_rq method here. So this
> - * should move out into a workqueue unless we get the file operations to
> - * advertise if they support specific fallocate operations.
> - */
> - queue_limits_commit_update(lo->lo_queue, &lim);
> + queue_limits_commit_update_frozen(lo->lo_queue, &lim);
> +}
> +
> +static void loop_clear_limits(struct loop_device *lo, int mode)
> +{
> + atomic_or(mode, &lo->clear_limits_mode);
> + schedule_work(&lo->clear_limits_work);
> }
>
> static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
> @@ -516,6 +520,7 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
> static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
> {
> lo->lo_backing_file = file;
> + atomic_set(&lo->clear_limits_mode, 0);
> lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
> mapping_set_gfp_mask(file->f_mapping,
> lo->old_gfp_mask & ~(__GFP_IO | __GFP_FS));
> @@ -1781,6 +1786,7 @@ static void lo_free_disk(struct gendisk *disk)
> destroy_workqueue(lo->workqueue);
> loop_free_idle_workers(lo, true);
> timer_shutdown_sync(&lo->timer);
> + cancel_work_sync(&lo->clear_limits_work);
> mutex_destroy(&lo->lo_mutex);
> kfree(lo);
> }
> @@ -2100,6 +2106,7 @@ static int loop_add(int i)
> spin_lock_init(&lo->lo_lock);
> spin_lock_init(&lo->lo_work_lock);
> INIT_WORK(&lo->rootcg_work, loop_rootcg_workfn);
> + INIT_WORK(&lo->clear_limits_work, loop_clear_limits_workfn);
> INIT_LIST_HEAD(&lo->rootcg_cmd_list);
> disk->major = LOOP_MAJOR;
> disk->first_minor = i << part_shift;
> @@ -2138,6 +2145,10 @@ static int loop_add(int i)
>
> static void loop_remove(struct loop_device *lo)
> {
> + /* Cancel early: the queue may be in RCU-delayed freeing
> + * by the time lo_free_disk() runs. */
> + cancel_work_sync(&lo->clear_limits_work);
> +
> /* Make this loop device unreachable from pathname. */
> del_gendisk(lo->lo_disk);
> blk_mq_free_tag_set(&lo->tag_set);
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] loop: defer the queue limits clear to a workqueue
2026-09-02 6:36 ` Tao Cui
@ 2026-09-02 9:26 ` Tao Cui
0 siblings, 0 replies; 3+ messages in thread
From: Tao Cui @ 2026-09-02 9:26 UTC (permalink / raw)
To: Bart Van Assche, axboe, hch, Tetsuo Handa
Cc: cui.tao, linux-block, linux-kernel, Tao Cui
在 2026/9/2 14:36, Tao Cui 写道:
> Hi. Bart,
>
> 在 2026/9/2 14:34, Tao Cui 写道:
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> loop_clear_limits() calls queue_limits_commit_update() directly from
>> the loop workqueue that processes the request. That does a
>> non-atomic struct assignment to q->limits without freezing the queue,
>> which races with lockless readers of q->limits on other CPUs - bio
>> splitting reads max_hw_sectors, the discard path reads
>> max_hw_discard_sectors - and can let them observe torn values. The
>> trigger is a discard or write-zeroes request on a loop device whose
>> backing file does not support the corresponding fallocate operation.
>>
>> The code already has an XXX comment saying this should move to a
>> workqueue. Do that: schedule a work item on the system workqueue,
>> where it is safe to freeze the queue and update the limits using
>> queue_limits_commit_update_frozen(). Accumulate pending modes in
>> lo->clear_limits_mode so that failures between scheduling and
>> execution of the work item are not lost, reset the accumulated
>> modes when a new backing file is assigned, and cancel the work
>> item before the device is freed.
>>
>> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>>
>> ---
>> Changes since v1:
>>
>> - Make clear_limits_mode atomic_t: loop workers can hit the |=
>> concurrently.
>>
>> - Reset clear_limits_mode when assigning a new backing file, so
>> stale modes do not clear limits of the new file.
>>
>> - Cancel the work item from loop_remove() before del_gendisk():
>> the queue can already be in RCU-delayed freeing when
>> lo_free_disk() cancels it.
>>
>> Tested on x86-64 (qemu, vfat-backed loop device): 30s discard and
>> reconfigure loop exercises the clear 577 times, no torn sysfs reads,
>> no difference against the unpatched kernel.
>>
>
> thanks for the Reviewed-by on v1.
>
> v2 makes clear_limits_mode atomic, resets it on rebind, and
> cancels the work item from loop_remove(). The code changed, so
> could you take another look when you have time?
>
I went through the latest sashiko report on v2; its four inline
comments are two issues.
The cancellation issue (marked on loop_remove() and
lo_free_disk()) is not reachable: loop_control_remove() only
accepts a device that is Lo_unbound with zero openers (loop.c,
-EBUSY otherwise), and a bound device also holds a module
reference from loop_configure(), so loop_remove() never sees
in-flight I/O that could reschedule the work item during
del_gendisk(). The cancel in lo_free_disk() therefore never
waits on a running instance.
The mode-capture issue (marked on the atomic_xchg() and the
reset) is real. It is the window the v1 commit message already
described: the work item takes the modes with atomic_xchg()
before it enters the freeze inside
queue_limits_commit_update_frozen(), so a LOOP_CHANGE_FD that
completes in between gets the old modes applied to the new
backing file, and discard stays off until the next reconfigure.
v2 only fixed the persistent half of it. Cancelling from
loop_assign_backing_file() is not an option, as the work item may
be blocked on the very freeze that loop_change_fd() holds, and
cancel_work_sync() would deadlock.
The fix is a rebind generation counter: capture it when
scheduling, skip the clear in the work item when it changed. I'll
send a v3 with that.
---
Tao
> Thanks,
> Tao
>
>> Link: https://lore.kernel.org/r/20260828072004.273519-1-cui.tao@linux.dev/
>> ---
>> drivers/block/loop.c | 29 ++++++++++++++++++++---------
>> 1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index 6f12976035b0..368ea857895c 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -67,6 +67,8 @@ struct loop_device {
>> struct list_head rootcg_cmd_list;
>> struct list_head idle_worker_list;
>> struct rb_root worker_tree;
>> + struct work_struct clear_limits_work;
>> + atomic_t clear_limits_mode;
>> struct timer_list timer;
>> bool sysfs_inited;
>>
>> @@ -222,9 +224,12 @@ static void loop_set_size(struct loop_device *lo, loff_t size)
>> kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>> }
>>
>> -static void loop_clear_limits(struct loop_device *lo, int mode)
>> +static void loop_clear_limits_workfn(struct work_struct *work)
>> {
>> + struct loop_device *lo =
>> + container_of(work, struct loop_device, clear_limits_work);
>> struct queue_limits lim = queue_limits_start_update(lo->lo_queue);
>> + int mode = atomic_xchg(&lo->clear_limits_mode, 0);
>>
>> if (mode & FALLOC_FL_ZERO_RANGE)
>> lim.max_write_zeroes_sectors = 0;
>> @@ -234,14 +239,13 @@ static void loop_clear_limits(struct loop_device *lo, int mode)
>> lim.discard_granularity = 0;
>> }
>>
>> - /*
>> - * XXX: this updates the queue limits without freezing the queue, which
>> - * is against the locking protocol and dangerous. But we can't just
>> - * freeze the queue as we're inside the ->queue_rq method here. So this
>> - * should move out into a workqueue unless we get the file operations to
>> - * advertise if they support specific fallocate operations.
>> - */
>> - queue_limits_commit_update(lo->lo_queue, &lim);
>> + queue_limits_commit_update_frozen(lo->lo_queue, &lim);
>> +}
>> +
>> +static void loop_clear_limits(struct loop_device *lo, int mode)
>> +{
>> + atomic_or(mode, &lo->clear_limits_mode);
>> + schedule_work(&lo->clear_limits_work);
>> }
>>
>> static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
>> @@ -516,6 +520,7 @@ static int loop_validate_file(struct file *file, struct block_device *bdev)
>> static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
>> {
>> lo->lo_backing_file = file;
>> + atomic_set(&lo->clear_limits_mode, 0);
>> lo->old_gfp_mask = mapping_gfp_mask(file->f_mapping);
>> mapping_set_gfp_mask(file->f_mapping,
>> lo->old_gfp_mask & ~(__GFP_IO | __GFP_FS));
>> @@ -1781,6 +1786,7 @@ static void lo_free_disk(struct gendisk *disk)
>> destroy_workqueue(lo->workqueue);
>> loop_free_idle_workers(lo, true);
>> timer_shutdown_sync(&lo->timer);
>> + cancel_work_sync(&lo->clear_limits_work);
>> mutex_destroy(&lo->lo_mutex);
>> kfree(lo);
>> }
>> @@ -2100,6 +2106,7 @@ static int loop_add(int i)
>> spin_lock_init(&lo->lo_lock);
>> spin_lock_init(&lo->lo_work_lock);
>> INIT_WORK(&lo->rootcg_work, loop_rootcg_workfn);
>> + INIT_WORK(&lo->clear_limits_work, loop_clear_limits_workfn);
>> INIT_LIST_HEAD(&lo->rootcg_cmd_list);
>> disk->major = LOOP_MAJOR;
>> disk->first_minor = i << part_shift;
>> @@ -2138,6 +2145,10 @@ static int loop_add(int i)
>>
>> static void loop_remove(struct loop_device *lo)
>> {
>> + /* Cancel early: the queue may be in RCU-delayed freeing
>> + * by the time lo_free_disk() runs. */
>> + cancel_work_sync(&lo->clear_limits_work);
>> +
>> /* Make this loop device unreachable from pathname. */
>> del_gendisk(lo->lo_disk);
>> blk_mq_free_tag_set(&lo->tag_set);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-02 9:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-02 6:34 [PATCH v2] loop: defer the queue limits clear to a workqueue Tao Cui
2026-09-02 6:36 ` Tao Cui
2026-09-02 9:26 ` Tao Cui
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®