Commit f8933667 authored by Ming Lei's avatar Ming Lei Committed by Jens Axboe

block: loop: don't hold lo_ctl_mutex in lo_open

The lo_ctl_mutex is held for running all ioctl handlers, and
in some ioctl handlers, ioctl_by_bdev(BLKRRPART) is called for
rereading partitions, which requires bd_mutex.

So it is easy to cause failure because trylock(bd_mutex) may
fail inside blkdev_reread_part(), and follows the lock context:

blkid or other application:
	->open()
		->mutex_lock(bd_mutex)
		->lo_open()
			->mutex_lock(lo_ctl_mutex)

losetup(set fd ioctl):
	->mutex_lock(lo_ctl_mutex)
	->ioctl_by_bdev(BLKRRPART)
		->trylock(bd_mutex)

This patch trys to eliminate the ABBA lock dependency by removing
lo_ctl_mutext in lo_open() with the following approach:

1) make lo_refcnt as atomic_t and avoid acquiring lo_ctl_mutex in lo_open():
	- for open vs. add/del loop, no any problem because of loop_index_mutex
	- freeze request queue during clr_fd, so I/O can't come until
	  clearing fd is completed, like the effect of holding lo_ctl_mutex
	  in lo_open
	- both open() and release() have been serialized by bd_mutex already

2) don't hold lo_ctl_mutex for decreasing/checking lo_refcnt in
lo_release(), then lo_ctl_mutex is only required for the last release.
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Tested-by: default avatarJarod Wilson <jarod@redhat.com>
Acked-by: default avatarJarod Wilson <jarod@redhat.com>
Signed-off-by: default avatarMing Lei <ming.lei@canonical.com>
Signed-off-by: default avatarJens Axboe <axboe@fb.com>
parent b816d45f
...@@ -831,7 +831,7 @@ static int loop_clr_fd(struct loop_device *lo) ...@@ -831,7 +831,7 @@ static int loop_clr_fd(struct loop_device *lo)
* <dev>/do something like mkfs/losetup -d <dev> causing the losetup -d * <dev>/do something like mkfs/losetup -d <dev> causing the losetup -d
* command to fail with EBUSY. * command to fail with EBUSY.
*/ */
if (lo->lo_refcnt > 1) { if (atomic_read(&lo->lo_refcnt) > 1) {
lo->lo_flags |= LO_FLAGS_AUTOCLEAR; lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
mutex_unlock(&lo->lo_ctl_mutex); mutex_unlock(&lo->lo_ctl_mutex);
return 0; return 0;
...@@ -840,6 +840,9 @@ static int loop_clr_fd(struct loop_device *lo) ...@@ -840,6 +840,9 @@ static int loop_clr_fd(struct loop_device *lo)
if (filp == NULL) if (filp == NULL)
return -EINVAL; return -EINVAL;
/* freeze request queue during the transition */
blk_mq_freeze_queue(lo->lo_queue);
spin_lock_irq(&lo->lo_lock); spin_lock_irq(&lo->lo_lock);
lo->lo_state = Lo_rundown; lo->lo_state = Lo_rundown;
lo->lo_backing_file = NULL; lo->lo_backing_file = NULL;
...@@ -871,6 +874,8 @@ static int loop_clr_fd(struct loop_device *lo) ...@@ -871,6 +874,8 @@ static int loop_clr_fd(struct loop_device *lo)
lo->lo_state = Lo_unbound; lo->lo_state = Lo_unbound;
/* This is safe: open() is still holding a reference. */ /* This is safe: open() is still holding a reference. */
module_put(THIS_MODULE); module_put(THIS_MODULE);
blk_mq_unfreeze_queue(lo->lo_queue);
if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev) if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev)
ioctl_by_bdev(bdev, BLKRRPART, 0); ioctl_by_bdev(bdev, BLKRRPART, 0);
lo->lo_flags = 0; lo->lo_flags = 0;
...@@ -1330,9 +1335,7 @@ static int lo_open(struct block_device *bdev, fmode_t mode) ...@@ -1330,9 +1335,7 @@ static int lo_open(struct block_device *bdev, fmode_t mode)
goto out; goto out;
} }
mutex_lock(&lo->lo_ctl_mutex); atomic_inc(&lo->lo_refcnt);
lo->lo_refcnt++;
mutex_unlock(&lo->lo_ctl_mutex);
out: out:
mutex_unlock(&loop_index_mutex); mutex_unlock(&loop_index_mutex);
return err; return err;
...@@ -1343,11 +1346,10 @@ static void lo_release(struct gendisk *disk, fmode_t mode) ...@@ -1343,11 +1346,10 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
struct loop_device *lo = disk->private_data; struct loop_device *lo = disk->private_data;
int err; int err;
mutex_lock(&lo->lo_ctl_mutex); if (atomic_dec_return(&lo->lo_refcnt))
return;
if (--lo->lo_refcnt)
goto out;
mutex_lock(&lo->lo_ctl_mutex);
if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) { if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
/* /*
* In autoclear mode, stop the loop thread * In autoclear mode, stop the loop thread
...@@ -1601,6 +1603,7 @@ static int loop_add(struct loop_device **l, int i) ...@@ -1601,6 +1603,7 @@ static int loop_add(struct loop_device **l, int i)
disk->flags |= GENHD_FL_NO_PART_SCAN; disk->flags |= GENHD_FL_NO_PART_SCAN;
disk->flags |= GENHD_FL_EXT_DEVT; disk->flags |= GENHD_FL_EXT_DEVT;
mutex_init(&lo->lo_ctl_mutex); mutex_init(&lo->lo_ctl_mutex);
atomic_set(&lo->lo_refcnt, 0);
lo->lo_number = i; lo->lo_number = i;
spin_lock_init(&lo->lo_lock); spin_lock_init(&lo->lo_lock);
disk->major = LOOP_MAJOR; disk->major = LOOP_MAJOR;
...@@ -1718,7 +1721,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd, ...@@ -1718,7 +1721,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
mutex_unlock(&lo->lo_ctl_mutex); mutex_unlock(&lo->lo_ctl_mutex);
break; break;
} }
if (lo->lo_refcnt > 0) { if (atomic_read(&lo->lo_refcnt) > 0) {
ret = -EBUSY; ret = -EBUSY;
mutex_unlock(&lo->lo_ctl_mutex); mutex_unlock(&lo->lo_ctl_mutex);
break; break;
......
...@@ -28,7 +28,7 @@ struct loop_func_table; ...@@ -28,7 +28,7 @@ struct loop_func_table;
struct loop_device { struct loop_device {
int lo_number; int lo_number;
int lo_refcnt; atomic_t lo_refcnt;
loff_t lo_offset; loff_t lo_offset;
loff_t lo_sizelimit; loff_t lo_sizelimit;
int lo_flags; int lo_flags;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment