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

[Bart Van Assche <bvanassche@acm.org>]

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.
See also
https://lore.kernel.org/linux-block/69960302-1535-441a-be4e-d652766d65c2@I-love.SAKURA.ne.jp/

Should this patch perhaps be rebased on top of Tetsuo's series?

> @@ -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()?

Thanks,

Bart.