Re: [PATCH] loop: make autoclear operation asynchronous

[Jan Kara <jack@suse.cz>]

On Wed 01-12-21 23:41:23, Tetsuo Handa wrote:
> On 2021/11/30 21:57, Christoph Hellwig wrote:
> > On Mon, Nov 29, 2021 at 07:36:27PM +0900, Tetsuo Handa wrote:
> >> If the caller just want to call ioctl(LOOP_CTL_GET_FREE) followed by
> >> ioctl(LOOP_CONFIGURE), deferring __loop_clr_fd() would be fine.
> >>
> >> But the caller might want to unmount as soon as fput(filp) from __loop_clr_fd() completes.
> >> I think we need to wait for __loop_clr_fd() from lo_release() to complete.
> > 
> > Anything else could have a reference to this or other files as well.
> > So I can't see how deferring the clear to a different context can be
> > any kind of problem in practice.
> > 
> 
> OK. Here is a patch.
> Is this better than temporarily dropping disk->open_mutex ?

The patch looks good to me. Just one suggestion for improvement:

> +static void loop_schedule_rundown(struct loop_device *lo)
> +{
> +	struct block_device *bdev = lo->lo_device;
> +	struct gendisk *disk = lo->lo_disk;
> +
> +	__module_get(disk->fops->owner);
> +	kobject_get(&bdev->bd_device.kobj);
> +	INIT_WORK(&lo->rundown_work, loop_rundown_workfn);
> +	queue_work(system_long_wq, &lo->rundown_work);
>  }

Why not scheduling this using task_work_add()? It solves the locking
context problems, has generally lower overhead than normal work (no need to
schedule), and avoids possible unexpected side-effects of releasing
loopback device later. Also task work is specifically designed so that one
task work can queue another task work so we should be fine using it.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR