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

[Tao Cui]

Hi Bart,

在 2026/9/29 01:26, Bart Van Assche 写道:
> On 9/28/26 2:35 AM, Tao Cui wrote:
>> @@ -1148,6 +1182,16 @@ static void __loop_clr_fd(struct loop_device *lo)
>>       lo->lo_backing_file = NULL;
>>       spin_unlock_irq(&lo->lo_lock);
>>   +    /*
>> +     * Invalidate any clear that was scheduled against the old backing
>> +     * file.  Unbinding cannot race with in-flight I/O, so cancelling
>> +     * here leaves no work item behind.
>> +     */
> 
> This comment is wrong - asynchronous I/O can still be in progress here.

You are right, asynchronous I/O can still be in flight in
__loop_clr_fd(). I'll fix the comment.
> See also
> https://lore.kernel.org/linux-block/69960302-1535-441a-be4e-d652766d65c2@xxxxxxxxxxxxxxxxxxx/
> 
> Should this patch perhaps be rebased on top of Tetsuo's series?
> 
On rebasing on top of Tetsuo's series: since you recommend it, I
can rebase this patch on top of it - the cancel inside
__loop_clr_fd() just moves along with the new structure, and the
I/O flush there makes that cancel airtight. 

>> @@ -1783,7 +1827,9 @@ 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);
>> +    mutex_destroy(&lo->clear_limits_lock);
>>       kfree(lo);
>>   }
> 
> Is the above new cancel_work_sync() call really necessary?
> 
>> @@ -2140,6 +2188,12 @@ static int loop_add(int i)
>>     static void loop_remove(struct loop_device *lo)
>>   {
>> +    /*
>> +     * 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. */
>>       del_gendisk(lo->lo_disk);
>>       blk_mq_free_tag_set(&lo->tag_set);
> 
> Is the above new cancel_work_sync() call necessary since there is
> already an identical call in __loop_clr_fd()?
> 
On the cancel in loop_remove(): i think this one is needed because
asynchronous I/O may still complete after __loop_clr_fd() has run
and queue the work item. Since the queue becomes eligible for
RCU-delayed freeing before lo_free_disk() is reached,
cancel_work_sync() before del_gendisk() ensures no pending
clear_limits_work remains against that queue.

I'll send v7 with the comment fixed and the redundant cancel removed once Tetsuo's series has landed.

Thanks,
Tao.
> Thanks,
> 
> Bart.