All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/7] block: reread partitions changes and fix for loop
@ 2015-05-06  4:26 Ming Lei
  2015-05-06  4:26 ` [PATCH v3 1/7] block: export blkdev_reread_part() and __blkdev_reread_part() Ming Lei
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390

Hi Guys,

Recently there are several reports about loop partition scanning
failure[1][2].

For loop, the root cause is one ABBA and one AA lock dependency
issue, and the two are fixed by patch 2 and patch 3 each.

Another reason is from the trylock in blkdev_reread_part(), which
may cause partition scanning failure too sometimes when another task
is holding the bd_mutex. In the discussion[1], both Tejun and Christoph
suggests to replace the trylock with mutex_lock in blkdev_reread_part(),
also Christoph suggests to export blkdev_reread_part.

Following the discussion, this patchset exports blkdev_reread_part(), and
introduces __blkdev_reread_part() for fixing loop's AA lock issue.
Then ioctl_by_bdev(BLKRRPART) in loop, nbd and dasd is replaced with
blkdev_reread_part(). In the last patch, trylock in blkdev_reread_part()
is replaced with mutex_lock, and some analysis is provided about the conversion.

V3:
	- fix lock unbalance in loop (2/7)
	- add reviewed-by from Christoph
	- run xfstest over loop and it passed
	- enable lockdep for verifying lock changes, and no warning
	with some common tests over loop block 

V2:
	- only print debug message in case of error (7/7)
	- add tested-by, acked-by

V1:
	- introduce __blkdev_reread_part(), and use lockdep_assert_held()(1/7)
	- replace lo_open_mutex with atomic reference count, plus freezing queue(2/7)
	- add comment about detecting release path(3/7)
	- remove dead code in dasd(7/7)

 block/ioctl.c                   | 37 ++++++++++++++++++++++++++++++++-----
 drivers/block/loop.c            | 51 ++++++++++++++++++++++++++++++++++++++-------------
 drivers/block/loop.h            |  2 +-
 drivers/block/nbd.c             |  2 +-
 drivers/s390/block/dasd_genhd.c | 19 +++++--------------
 include/linux/fs.h              |  3 +++
 6 files changed, 80 insertions(+), 34 deletions(-)


[1], https://lkml.org/lkml/2015/1/26/137
[2], https://lkml.org/lkml/2015/3/31/888

Thanks,
Ming Lei



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/7] block: export blkdev_reread_part() and __blkdev_reread_part()
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
@ 2015-05-06  4:26 ` Ming Lei
  2015-05-06  4:26 ` [PATCH v3 2/7] block: loop: don't hold lo_ctl_mutex in lo_open Ming Lei
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390, Christoph Hellwig,
	Stefan Weinhuber, Ming Lei

From: Jarod Wilson <jarod@redhat.com>

This patch exports blkdev_reread_part() for block drivers, also
introduce __blkdev_reread_part().

For some drivers, such as loop, reread of partitions can be run
from the release path, and bd_mutex may already be held prior to
calling ioctl_by_bdev(bdev, BLKRRPART, 0), so introduce
__blkdev_reread_part for use in such cases.

CC: Christoph Hellwig <hch@lst.de>
CC: Jens Axboe <axboe@kernel.dk>
CC: Tejun Heo <tj@kernel.org>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Markus Pargmann <mpa@pengutronix.de>
CC: Stefan Weinhuber <wein@de.ibm.com>
CC: Stefan Haberland <stefan.haberland@de.ibm.com>
CC: Sebastian Ott <sebott@linux.vnet.ibm.com>
CC: Fabian Frederick <fabf@skynet.be>
CC: Ming Lei <ming.lei@canonical.com>
CC: David Herrmann <dh.herrmann@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Peter Zijlstra <peterz@infradead.org>
CC: nbd-general@lists.sourceforge.net
CC: linux-s390@vger.kernel.org
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/ioctl.c      | 28 +++++++++++++++++++++++++---
 include/linux/fs.h |  3 +++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 7d8befd..203cb4a 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -150,21 +150,43 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 	}
 }
 
-static int blkdev_reread_part(struct block_device *bdev)
+/*
+ * This is an exported API for the block driver, and will not
+ * acquire bd_mutex. This API should be used in case that
+ * caller has held bd_mutex already.
+ */
+int __blkdev_reread_part(struct block_device *bdev)
 {
 	struct gendisk *disk = bdev->bd_disk;
-	int res;
 
 	if (!disk_part_scan_enabled(disk) || bdev != bdev->bd_contains)
 		return -EINVAL;
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
+
+	lockdep_assert_held(&bdev->bd_mutex);
+
+	return rescan_partitions(disk, bdev);
+}
+EXPORT_SYMBOL(__blkdev_reread_part);
+
+/*
+ * This is an exported API for the block driver, and will
+ * try to acquire bd_mutex. If bd_mutex has been held already
+ * in current context, please call __blkdev_reread_part().
+ */
+int blkdev_reread_part(struct block_device *bdev)
+{
+	int res;
+
 	if (!mutex_trylock(&bdev->bd_mutex))
 		return -EBUSY;
-	res = rescan_partitions(disk, bdev);
+	res = __blkdev_reread_part(bdev);
 	mutex_unlock(&bdev->bd_mutex);
+
 	return res;
 }
+EXPORT_SYMBOL(blkdev_reread_part);
 
 static int blk_ioctl_discard(struct block_device *bdev, uint64_t start,
 			     uint64_t len, int secure)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 35ec87e..1ef6390 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2279,6 +2279,9 @@ extern struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
 extern struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode,
 					      void *holder);
 extern void blkdev_put(struct block_device *bdev, fmode_t mode);
+extern int __blkdev_reread_part(struct block_device *bdev);
+extern int blkdev_reread_part(struct block_device *bdev);
+
 #ifdef CONFIG_SYSFS
 extern int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk);
 extern void bd_unlink_disk_holder(struct block_device *bdev,
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/7] block: loop: don't hold lo_ctl_mutex in lo_open
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
  2015-05-06  4:26 ` [PATCH v3 1/7] block: export blkdev_reread_part() and __blkdev_reread_part() Ming Lei
@ 2015-05-06  4:26 ` Ming Lei
  2015-05-06  4:26 ` [PATCH v3 3/7] block: loop: fix another reread part failure Ming Lei
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390, Ming Lei

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: Christoph Hellwig <hch@lst.de>
Tested-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/block/loop.c | 21 ++++++++++++---------
 drivers/block/loop.h |  2 +-
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1bee523..b3e294e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -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
 	 * command to fail with EBUSY.
 	 */
-	if (lo->lo_refcnt > 1) {
+	if (atomic_read(&lo->lo_refcnt) > 1) {
 		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
 		mutex_unlock(&lo->lo_ctl_mutex);
 		return 0;
@@ -840,6 +840,9 @@ static int loop_clr_fd(struct loop_device *lo)
 	if (filp == NULL)
 		return -EINVAL;
 
+	/* freeze request queue during the transition */
+	blk_mq_freeze_queue(lo->lo_queue);
+
 	spin_lock_irq(&lo->lo_lock);
 	lo->lo_state = Lo_rundown;
 	lo->lo_backing_file = NULL;
@@ -871,6 +874,8 @@ static int loop_clr_fd(struct loop_device *lo)
 	lo->lo_state = Lo_unbound;
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
+	blk_mq_unfreeze_queue(lo->lo_queue);
+
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev)
 		ioctl_by_bdev(bdev, BLKRRPART, 0);
 	lo->lo_flags = 0;
@@ -1330,9 +1335,7 @@ static int lo_open(struct block_device *bdev, fmode_t mode)
 		goto out;
 	}
 
-	mutex_lock(&lo->lo_ctl_mutex);
-	lo->lo_refcnt++;
-	mutex_unlock(&lo->lo_ctl_mutex);
+	atomic_inc(&lo->lo_refcnt);
 out:
 	mutex_unlock(&loop_index_mutex);
 	return err;
@@ -1343,11 +1346,10 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
 	struct loop_device *lo = disk->private_data;
 	int err;
 
-	mutex_lock(&lo->lo_ctl_mutex);
-
-	if (--lo->lo_refcnt)
-		goto out;
+	if (atomic_dec_return(&lo->lo_refcnt))
+		return;
 
+	mutex_lock(&lo->lo_ctl_mutex);
 	if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
 		/*
 		 * In autoclear mode, stop the loop thread
@@ -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_EXT_DEVT;
 	mutex_init(&lo->lo_ctl_mutex);
+	atomic_set(&lo->lo_refcnt, 0);
 	lo->lo_number		= i;
 	spin_lock_init(&lo->lo_lock);
 	disk->major		= LOOP_MAJOR;
@@ -1718,7 +1721,7 @@ static long loop_control_ioctl(struct file *file, unsigned int cmd,
 			mutex_unlock(&lo->lo_ctl_mutex);
 			break;
 		}
-		if (lo->lo_refcnt > 0) {
+		if (atomic_read(&lo->lo_refcnt) > 0) {
 			ret = -EBUSY;
 			mutex_unlock(&lo->lo_ctl_mutex);
 			break;
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 49564ed..25e8997 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -28,7 +28,7 @@ struct loop_func_table;
 
 struct loop_device {
 	int		lo_number;
-	int		lo_refcnt;
+	atomic_t	lo_refcnt;
 	loff_t		lo_offset;
 	loff_t		lo_sizelimit;
 	int		lo_flags;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 3/7] block: loop: fix another reread part failure
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
  2015-05-06  4:26 ` [PATCH v3 1/7] block: export blkdev_reread_part() and __blkdev_reread_part() Ming Lei
  2015-05-06  4:26 ` [PATCH v3 2/7] block: loop: don't hold lo_ctl_mutex in lo_open Ming Lei
@ 2015-05-06  4:26 ` Ming Lei
  2015-05-06  4:26 ` [PATCH v3 4/7] block: nbd: convert to blkdev_reread_part() Ming Lei
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390, Ming Lei

loop_clr_fd() can be run piggyback with lo_release(), and
under this situation, reread partition may always fail because
bd_mutex has been held already.

This patch detects the situation by the reference count, and
call __blkdev_reread_part() to avoid acquiring the lock again.

In the meantime, this patch switches to new kernel APIs
of blkdev_reread_part() and __blkdev_reread_part().

Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/block/loop.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b3e294e..2b99e34 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -474,6 +474,28 @@ static int loop_flush(struct loop_device *lo)
 	return loop_switch(lo, NULL);
 }
 
+static void loop_reread_partitions(struct loop_device *lo,
+				   struct block_device *bdev)
+{
+	int rc;
+
+	/*
+	 * bd_mutex has been held already in release path, so don't
+	 * acquire it if this function is called in such case.
+	 *
+	 * If the reread partition isn't from release path, lo_refcnt
+	 * must be at least one and it can only become zero when the
+	 * current holder is released.
+	 */
+	if (!atomic_read(&lo->lo_refcnt))
+		rc = __blkdev_reread_part(bdev);
+	else
+		rc = blkdev_reread_part(bdev);
+	if (rc)
+		pr_warn("%s: partition scan of loop%d (%s) failed (rc=%d)\n",
+			__func__, lo->lo_number, lo->lo_file_name, rc);
+}
+
 /*
  * loop_change_fd switched the backing store of a loopback device to
  * a new file. This is useful for operating system installers to free up
@@ -522,7 +544,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
 
 	fput(old_file);
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN)
-		ioctl_by_bdev(bdev, BLKRRPART, 0);
+		loop_reread_partitions(lo, bdev);
 	return 0;
 
  out_putf:
@@ -759,7 +781,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	if (part_shift)
 		lo->lo_flags |= LO_FLAGS_PARTSCAN;
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN)
-		ioctl_by_bdev(bdev, BLKRRPART, 0);
+		loop_reread_partitions(lo, bdev);
 
 	/* Grab the block_device to prevent its destruction after we
 	 * put /dev/loopXX inode. Later in loop_clr_fd() we bdput(bdev).
@@ -877,7 +899,7 @@ static int loop_clr_fd(struct loop_device *lo)
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
 	if (lo->lo_flags & LO_FLAGS_PARTSCAN && bdev)
-		ioctl_by_bdev(bdev, BLKRRPART, 0);
+		loop_reread_partitions(lo, bdev);
 	lo->lo_flags = 0;
 	if (!part_shift)
 		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
@@ -954,7 +976,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	     !(lo->lo_flags & LO_FLAGS_PARTSCAN)) {
 		lo->lo_flags |= LO_FLAGS_PARTSCAN;
 		lo->lo_disk->flags &= ~GENHD_FL_NO_PART_SCAN;
-		ioctl_by_bdev(lo->lo_device, BLKRRPART, 0);
+		loop_reread_partitions(lo, lo->lo_device);
 	}
 
 	lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 4/7] block: nbd: convert to blkdev_reread_part()
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
                   ` (2 preceding siblings ...)
  2015-05-06  4:26 ` [PATCH v3 3/7] block: loop: fix another reread part failure Ming Lei
@ 2015-05-06  4:26 ` Ming Lei
  2015-05-06  4:26 ` [PATCH v3 5/7] block: dasd_genhd: convert to blkdev_reread_part Ming Lei
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390, Ming Lei

Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 39e5f7f..50d7f87 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -713,7 +713,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		bdev->bd_inode->i_size = 0;
 		set_capacity(nbd->disk, 0);
 		if (max_part > 0)
-			ioctl_by_bdev(bdev, BLKRRPART, 0);
+			blkdev_reread_part(bdev);
 		if (nbd->disconnect) /* user requested, ignore socket errors */
 			return 0;
 		return nbd->harderror;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 5/7] block: dasd_genhd: convert to blkdev_reread_part
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
                   ` (3 preceding siblings ...)
  2015-05-06  4:26 ` [PATCH v3 4/7] block: nbd: convert to blkdev_reread_part() Ming Lei
@ 2015-05-06  4:26 ` Ming Lei
  2015-05-06  4:26 ` [PATCH v3 6/7] block: replace trylock with mutex_lock in blkdev_reread_part() Ming Lei
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390, Ming Lei

Also remove the obsolete comment.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/s390/block/dasd_genhd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 90f39f7..2af4619 100644
--- a/drivers/s390/block/dasd_genhd.c
+++ b/drivers/s390/block/dasd_genhd.c
@@ -116,14 +116,11 @@ int dasd_scan_partitions(struct dasd_block *block)
 			      rc);
 		return -ENODEV;
 	}
-	/*
-	 * See fs/partition/check.c:register_disk,rescan_partitions
-	 * Can't call rescan_partitions directly. Use ioctl.
-	 */
-	rc = ioctl_by_bdev(bdev, BLKRRPART, 0);
+
+	rc = blkdev_reread_part(bdev);
 	while (rc == -EBUSY && retry > 0) {
 		schedule();
-		rc = ioctl_by_bdev(bdev, BLKRRPART, 0);
+		rc = blkdev_reread_part(bdev);
 		retry--;
 		DBF_DEV_EVENT(DBF_ERR, block->base,
 			      "scan partitions error, retry %d rc %d",
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 6/7] block: replace trylock with mutex_lock in blkdev_reread_part()
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
                   ` (4 preceding siblings ...)
  2015-05-06  4:26 ` [PATCH v3 5/7] block: dasd_genhd: convert to blkdev_reread_part Ming Lei
@ 2015-05-06  4:26 ` Ming Lei
  2015-05-06  4:26 ` [PATCH v3 7/7] s390/block/dasd: remove obsolete while -EBUSY loop Ming Lei
  2015-05-18 16:11 ` [PATCH v3 0/7] block: reread partitions changes and fix for loop Christoph Hellwig
  7 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390, Ming Lei

The only possible problem of using mutex_lock() instead of trylock
is about deadlock.

If there aren't any locks held before calling blkdev_reread_part(),
deadlock can't be caused by this conversion.

If there are locks held before calling blkdev_reread_part(),
and if these locks arn't required in open, close handler and I/O
path, deadlock shouldn't be caused too.

Both user space's ioctl(BLKRRPART) and md_setup_drive() from
init/do_mounts_md.c belongs to the 1st case, so the conversion is safe
for the two cases.

For loop, the previous patches in this pathset has fixed the ABBA lock
dependency, so the conversion is OK.

For nbd, tx_lock is held when calling the function:

	- both open and release won't hold the lock
	- when blkdev_reread_part() is run, I/O thread has been stopped
	already, so tx_lock won't be acquired in I/O path at that time.
	- so the conversion won't cause deadlock for nbd

For dasd, both dasd_open(), dasd_release() and request function don't
acquire any mutex/semphone, so the conversion should be safe.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/ioctl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 203cb4a..8061eba 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -174,13 +174,18 @@ EXPORT_SYMBOL(__blkdev_reread_part);
  * This is an exported API for the block driver, and will
  * try to acquire bd_mutex. If bd_mutex has been held already
  * in current context, please call __blkdev_reread_part().
+ *
+ * Make sure the held locks in current context aren't required
+ * in open()/close() handler and I/O path for avoiding ABBA deadlock:
+ * - bd_mutex is held before calling block driver's open/close
+ *   handler
+ * - reading partition table may submit I/O to the block device
  */
 int blkdev_reread_part(struct block_device *bdev)
 {
 	int res;
 
-	if (!mutex_trylock(&bdev->bd_mutex))
-		return -EBUSY;
+	mutex_lock(&bdev->bd_mutex);
 	res = __blkdev_reread_part(bdev);
 	mutex_unlock(&bdev->bd_mutex);
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 7/7] s390/block/dasd: remove obsolete while -EBUSY loop
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
                   ` (5 preceding siblings ...)
  2015-05-06  4:26 ` [PATCH v3 6/7] block: replace trylock with mutex_lock in blkdev_reread_part() Ming Lei
@ 2015-05-06  4:26 ` Ming Lei
  2015-05-18 16:11 ` [PATCH v3 0/7] block: reread partitions changes and fix for loop Christoph Hellwig
  7 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-06  4:26 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo
  Cc: Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390, Stefan Weinhuber,
	Ming Lei

From: Jarod Wilson <jarod@redhat.com>

With the mutex_trylock bit gone from blkdev_reread_part(), the retry logic
in dasd_scan_partitions() shouldn't be necessary.

CC: Christoph Hellwig <hch@infradead.org>
CC: Jens Axboe <axboe@kernel.dk>
CC: Tejun Heo <tj@kernel.org>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Markus Pargmann <mpa@pengutronix.de>
CC: Stefan Weinhuber <wein@de.ibm.com>
CC: Stefan Haberland <stefan.haberland@de.ibm.com>
CC: Sebastian Ott <sebott@linux.vnet.ibm.com>
CC: Fabian Frederick <fabf@skynet.be>
CC: Ming Lei <ming.lei@canonical.com>
CC: David Herrmann <dh.herrmann@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Peter Zijlstra <peterz@infradead.org>
CC: nbd-general@lists.sourceforge.net
CC: linux-s390@vger.kernel.org
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/s390/block/dasd_genhd.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 2af4619..ef1d9fb 100644
--- a/drivers/s390/block/dasd_genhd.c
+++ b/drivers/s390/block/dasd_genhd.c
@@ -99,9 +99,8 @@ void dasd_gendisk_free(struct dasd_block *block)
 int dasd_scan_partitions(struct dasd_block *block)
 {
 	struct block_device *bdev;
-	int retry, rc;
+	int rc;
 
-	retry = 5;
 	bdev = bdget_disk(block->gdp, 0);
 	if (!bdev) {
 		DBF_DEV_EVENT(DBF_ERR, block->base, "%s",
@@ -118,14 +117,9 @@ int dasd_scan_partitions(struct dasd_block *block)
 	}
 
 	rc = blkdev_reread_part(bdev);
-	while (rc == -EBUSY && retry > 0) {
-		schedule();
-		rc = blkdev_reread_part(bdev);
-		retry--;
+	if (rc)
 		DBF_DEV_EVENT(DBF_ERR, block->base,
-			      "scan partitions error, retry %d rc %d",
-			      retry, rc);
-	}
+				"scan partitions error, rc %d", rc);
 
 	/*
 	 * Since the matching blkdev_put call to the blkdev_get in
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/7] block: reread partitions changes and fix for loop
  2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
                   ` (6 preceding siblings ...)
  2015-05-06  4:26 ` [PATCH v3 7/7] s390/block/dasd: remove obsolete while -EBUSY loop Ming Lei
@ 2015-05-18 16:11 ` Christoph Hellwig
  2015-05-20  9:17     ` Ming Lei
  7 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2015-05-18 16:11 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-kernel, Christoph Hellwig, Tejun Heo,
	Andrew Morton, Alexander Viro, Jarod Wilson, David Herrmann,
	Markus Pargmann, nbd-general, Stefan Haberland, Sebastian Ott,
	Fabian Frederick, Peter Zijlstra, linux-s390

This series looks good to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/7] block: reread partitions changes and fix for loop
  2015-05-18 16:11 ` [PATCH v3 0/7] block: reread partitions changes and fix for loop Christoph Hellwig
@ 2015-05-20  9:17     ` Ming Lei
  0 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-20  9:17 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, Linux Kernel Mailing List, Tejun Heo, Andrew Morton,
	Alexander Viro, Jarod Wilson, David Herrmann, Markus Pargmann,
	nbd-general, Stefan Haberland, Sebastian Ott, Fabian Frederick,
	Peter Zijlstra, linux-s390

On Tue, May 19, 2015 at 12:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
> This series looks good to me,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Jens, last time you said you are OK with v2, and there is only one line
change in v3, so would you mind merging v3 in block tree?

Thanks,
Ming

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/7] block: reread partitions changes and fix for loop
@ 2015-05-20  9:17     ` Ming Lei
  0 siblings, 0 replies; 13+ messages in thread
From: Ming Lei @ 2015-05-20  9:17 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, Linux Kernel Mailing List, Tejun Heo, Andrew Morton,
	Alexander Viro, Jarod Wilson, David Herrmann, Markus Pargmann,
	nbd-general, Stefan Haberland, Sebastian Ott, Fabian Frederick,
	Peter Zijlstra, linux-s390

On Tue, May 19, 2015 at 12:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
> This series looks good to me,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Jens, last time you said you are OK with v2, and there is only one line
change in v3, so would you mind merging v3 in block tree?

Thanks,
Ming

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/7] block: reread partitions changes and fix for loop
  2015-05-20  9:17     ` Ming Lei
@ 2015-05-20 15:03       ` Jens Axboe
  -1 siblings, 0 replies; 13+ messages in thread
From: Jens Axboe @ 2015-05-20 15:03 UTC (permalink / raw)
  To: Ming Lei, Christoph Hellwig
  Cc: Linux Kernel Mailing List, Tejun Heo, Andrew Morton,
	Alexander Viro, Jarod Wilson, David Herrmann, Markus Pargmann,
	nbd-general, Stefan Haberland, Sebastian Ott, Fabian Frederick,
	Peter Zijlstra, linux-s390

On 05/20/2015 03:17 AM, Ming Lei wrote:
> On Tue, May 19, 2015 at 12:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> This series looks good to me,
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Jens, last time you said you are OK with v2, and there is only one line
> change in v3, so would you mind merging v3 in block tree?

I'll merge it for 4.2, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/7] block: reread partitions changes and fix for loop
@ 2015-05-20 15:03       ` Jens Axboe
  0 siblings, 0 replies; 13+ messages in thread
From: Jens Axboe @ 2015-05-20 15:03 UTC (permalink / raw)
  To: Ming Lei, Christoph Hellwig
  Cc: Linux Kernel Mailing List, Tejun Heo, Andrew Morton,
	Alexander Viro, Jarod Wilson, David Herrmann, Markus Pargmann,
	nbd-general, Stefan Haberland, Sebastian Ott, Fabian Frederick,
	Peter Zijlstra, linux-s390

On 05/20/2015 03:17 AM, Ming Lei wrote:
> On Tue, May 19, 2015 at 12:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> This series looks good to me,
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Jens, last time you said you are OK with v2, and there is only one line
> change in v3, so would you mind merging v3 in block tree?

I'll merge it for 4.2, thanks.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-05-20 15:03 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-06  4:26 [PATCH v3 0/7] block: reread partitions changes and fix for loop Ming Lei
2015-05-06  4:26 ` [PATCH v3 1/7] block: export blkdev_reread_part() and __blkdev_reread_part() Ming Lei
2015-05-06  4:26 ` [PATCH v3 2/7] block: loop: don't hold lo_ctl_mutex in lo_open Ming Lei
2015-05-06  4:26 ` [PATCH v3 3/7] block: loop: fix another reread part failure Ming Lei
2015-05-06  4:26 ` [PATCH v3 4/7] block: nbd: convert to blkdev_reread_part() Ming Lei
2015-05-06  4:26 ` [PATCH v3 5/7] block: dasd_genhd: convert to blkdev_reread_part Ming Lei
2015-05-06  4:26 ` [PATCH v3 6/7] block: replace trylock with mutex_lock in blkdev_reread_part() Ming Lei
2015-05-06  4:26 ` [PATCH v3 7/7] s390/block/dasd: remove obsolete while -EBUSY loop Ming Lei
2015-05-18 16:11 ` [PATCH v3 0/7] block: reread partitions changes and fix for loop Christoph Hellwig
2015-05-20  9:17   ` Ming Lei
2015-05-20  9:17     ` Ming Lei
2015-05-20 15:03     ` Jens Axboe
2015-05-20 15:03       ` Jens Axboe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.