loop: defer the queue limits clear to a workqueue

PATCH Superseded Tao Cui 2 participants 2 messages Sep 19 – Sep 21 Block layer

Share & tools

Tao Cui

author

2026-09-19 10:47 UTC #0 lore raw

PATCH v4 1/1 loop: defer the queue limits clear to a workqueue

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 around the limits update. The pending modes and the rebind generation live under a new mutex, lo->clear_limits_lock. The work item takes the mutex with the queue frozen, so a rebind cannot slip in between the generation check and the limits commit. Rebinding the device drops the accumulated modes and invalidates an already scheduled work item, so a stale clear cannot hit the new backing file. The work item is cancelled before the device is freed.

Signed-off-by Tao Cui <[email protected]>

---
Changes since v3:

- Replace the three atomic variables (clear_limits_mode, rebind_gen, clear_limits_gen) with plain variables protected by a new clear_limits_lock mutex, as suggested by Bart. The mutex is taken after the queue freeze in the work item, which keeps the same freeze -> mutex ordering as loop_change_fd(), the only rebinding path that freezes the queue.

Changes since v2:

- Skip the clear when the device was rebound since the work item was scheduled: the modes used to be captured before the freeze, so a LOOP_CHANGE_FD completing in between could apply the old modes to the new backing file. The rebind generation is now checked with the queue frozen, which excludes loop_change_fd() because it assigns the new backing file under the same freeze, so a rebind cannot slip in between the check and the commit. Cancelling from loop_assign_backing_file() would instead deadlock on the freeze held by loop_change_fd().

- Also bump the rebind generation from __loop_clr_fd(): unbinding does not go through loop_assign_backing_file(), so a work item scheduled before the last close could otherwise commit a stale clear to the queue limits of the unbound device.

Changes since v1:

- 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): a 30s discard and reconfigure loop exercises the clear path 260 times, no torn sysfs reads, no difference against the unpatched kernel.

Link https://lore.kernel.org/r/[email protected]/ open here

---
 drivers/block/loop.c | 45 +++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

drivers/block/loop.c +30 ~~-15~~

index 9f6e518e60d28..d6460f359c0a8 100644
@@ -68,9 +68,10 @@ struct loop_device {
 	struct list_head        idle_worker_list;
 	struct rb_root          worker_tree;
 	struct work_struct      clear_limits_work;
-	atomic_t		clear_limits_mode;
-	atomic_t		rebind_gen;
-	atomic_t		clear_limits_gen;
+	struct mutex		clear_limits_lock;
+	unsigned int		clear_limits_mode;
+	unsigned int		rebind_gen;
+	unsigned int		clear_limits_gen;
 	struct timer_list       timer;
 	bool			sysfs_inited;

@@ -241,11 +242,13 @@ static void loop_clear_limits_workfn(struct work_struct *work)
 	 * under the freeze; the other rebinding paths, loop_configure()
 	 * and __loop_clr_fd(), do not freeze the queue, but they bump
 	 * rebind_gen and reset clear_limits_mode themselves, so the
-	 * generation check and the atomic_xchg() below cover them.
+	 * generation check and the reset below cover them.
 	 */
 	memflags = blk_mq_freeze_queue(lo->lo_queue);
-	if (atomic_read(&lo->clear_limits_gen) == atomic_read(&lo->rebind_gen)) {
-		mode = atomic_xchg(&lo->clear_limits_mode, 0);
+	mutex_lock(&lo->clear_limits_lock);
+	if (lo->clear_limits_gen == lo->rebind_gen) {
+		mode = lo->clear_limits_mode;
+		lo->clear_limits_mode = 0;
 
 		if (mode & FALLOC_FL_ZERO_RANGE)
 			lim.max_write_zeroes_sectors = 0;

@@ -255,14 +258,17 @@ static void loop_clear_limits_workfn(struct work_struct *work)
 			lim.discard_granularity = 0;
 		}
 	}
+	mutex_unlock(&lo->clear_limits_lock);
 	queue_limits_commit_update(lo->lo_queue, &lim);
 	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
 }
 
 static void loop_clear_limits(struct loop_device *lo, int mode)
 {
-	atomic_set(&lo->clear_limits_gen, atomic_read(&lo->rebind_gen));
-	atomic_or(mode, &lo->clear_limits_mode);
+	mutex_lock(&lo->clear_limits_lock);
+	lo->clear_limits_gen = lo->rebind_gen;
+	lo->clear_limits_mode |= mode;
+	mutex_unlock(&lo->clear_limits_lock);
 	schedule_work(&lo->clear_limits_work);
 }

@@ -538,8 +544,10 @@ 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_inc(&lo->rebind_gen);
-	atomic_set(&lo->clear_limits_mode, 0);
+	mutex_lock(&lo->clear_limits_lock);
+	lo->rebind_gen++;
+	lo->clear_limits_mode = 0;
+	mutex_unlock(&lo->clear_limits_lock);
 	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));

@@ -1168,13 +1176,16 @@ static void __loop_clr_fd(struct loop_device *lo)
 	spin_lock_irq(&lo->lo_lock);
 	filp = lo->lo_backing_file;
 	lo->lo_backing_file = NULL;
+	spin_unlock_irq(&lo->lo_lock);
+
 	/*
 	 * Invalidate any pending clear that was scheduled against the old
 	 * backing file, like loop_assign_backing_file() does on rebind.
 	 */
-	atomic_inc(&lo->rebind_gen);
-	atomic_set(&lo->clear_limits_mode, 0);
-	spin_unlock_irq(&lo->lo_lock);
+	mutex_lock(&lo->clear_limits_lock);
+	lo->rebind_gen++;
+	lo->clear_limits_mode = 0;
+	mutex_unlock(&lo->clear_limits_lock);
 
 	lo->lo_device = NULL;
 	lo->lo_offset = 0;

@@ -1813,6 +1824,7 @@ static void lo_free_disk(struct gendisk *disk)
 	timer_shutdown_sync(&lo->timer);
 	cancel_work_sync(&lo->clear_limits_work);
 	mutex_destroy(&lo->lo_mutex);
+	mutex_destroy(&lo->clear_limits_lock);
 	kfree(lo);
 }

@@ -2132,6 +2144,7 @@ static int loop_add(int i)
 	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);
+	mutex_init(&lo->clear_limits_lock);
 	INIT_LIST_HEAD(&lo->rootcg_cmd_list);
 	disk->major		= LOOP_MAJOR;
 	disk->first_minor	= i << part_shift;

@@ -2170,8 +2183,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 early: the queue may already be in RCU-delayed freeing
+	 * by the time lo_free_disk() cancels the work item.
+	 */
 	cancel_work_sync(&lo->clear_limits_work);
 
 	/* Make this loop device unreachable from pathname. */

Signature · 1 line

2.43.0

Bart Van Assche

2026-09-21 17:40 UTC #1 lore raw

Quoted 5 lines · Tao Cui

drivers/block/loop.c +0 -0
index 9f6e518e60d28..d6460f359c0a8 100644

drivers/block/loop.c struct loop_device { · line 74

Quoted 10 lines

drivers/block/loop.c +4 -3

@@ -68,9 +68,10 @@ struct loop_device {
  	struct list_head        idle_worker_list;
  	struct rb_root          worker_tree;
  	struct work_struct      clear_limits_work;
-	atomic_t		clear_limits_mode;
-	atomic_t		rebind_gen;
-	atomic_t		clear_limits_gen;
+	struct mutex		clear_limits_lock;
+	unsigned int		clear_limits_mode;
+	unsigned int		rebind_gen;
+	unsigned int		clear_limits_gen;

This patch has been generated as a diff compared to v3. Please make sure that your patch applies cleanly to Jens' for-next branch.

Thanks,

Bart.