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@xxxxxxxx>
SUSE Labs, CR
