Re: [PATCH] loop: defer the queue limits clear to a workqueue

[Tao Cui]

Hi Bart,

Thanks for the review. 

在 2026/9/25 02:01, Bart Van Assche 写道:
> On 9/24/26 3:20 AM, Tao Cui wrote:
>> +    memflags = blk_mq_freeze_queue(lo->lo_queue);
>> +    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;
>> +
>> +        if (mode & FALLOC_FL_PUNCH_HOLE) {
>> +            lim.max_hw_discard_sectors = 0;
>> +            lim.discard_granularity = 0;
>> +        }
>> +    }
> 
> I propose to remove the member variables clear_limits_gen and
> rebind_gen. These member variables complicate verifying correctness
> of the code and IMHO are not necessary. If loop_clear_limits() races
> with loop_clear_limits_workfn(), the work will be rescheduled and
> loop_clear_limits_workfn() will be called another time.
> 

The generation counters were intended to prevent a clear request
queued for an old backing file from being applied after the loop
device had been rebound. However, you're right that resetting
clear_limits_mode in loop_assign_backing_file() and __loop_clr_fd()
under the same mutex already prevents that, so the generation
counters are redundant. I'll remove them.

>>       queue_limits_commit_update(lo->lo_queue, &lim);
>> +    mutex_unlock(&lo->clear_limits_lock);
> 
> Since the mutex_lock(&lo->clear_limits_lock) call occurs after
> queue_limits_start_update(), please move the
> mutex_unlock(&lo->clear_limits_lock) call above the
> queue_limits_commit_update() call. This has no impact on
> correctness nor on the lock nesting order but makes the code
> easier to follow.
> 

On moving mutex_unlock() above queue_limits_commit_update(): with
the generation counters removed, the mutex is what keeps consuming
clear_limits_mode and committing the resulting queue limits atomic
with respect to a rebind. blk_mq_freeze_queue() is
reference-counted, so the workfn and loop_change_fd() can both hold
a freeze concurrently. If the workfn drops the mutex after consuming
a non-zero mode but before queue_limits_commit_update(),
loop_change_fd() could install a new backing file in that window,
and the pending clear would then be committed to the new file. Since
loop_change_fd() does not recompute the queue limits afterward, the
incorrectly cleared discard limits would persist until the next
reconfiguration.

For that reason, I'd prefer to keep the mutex held until after
queue_limits_commit_update(). I can add a comment explaining this if
that would help.

Thanks,
Tao

> Thanks,
> 
> Bart.