On Wed 01-12-21 23:41:23, Tetsuo Handa wrote:

On 2021/11/30 21:57, Christoph Hellwig wrote:

quoted

On Mon, Nov 29, 2021 at 07:36:27PM +0900, Tetsuo Handa wrote:

quoted

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 [off-list ref]
SUSE Labs, CR