[PATCH v1 0/7] block: reread partitions changes and fix for loop

[PATCH v1 0/7] block: reread partitions changes and fix for loop

[Ming Lei]

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.

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            |   49 +++++++++++++++++++++++++++++----------
 drivers/block/loop.h            |    2 +-
 drivers/block/nbd.c             |    2 +-
 drivers/s390/block/dasd_genhd.c |   20 ++++------------
 include/linux/fs.h              |    3 +++
 6 files changed, 79 insertions(+), 34 deletions(-)


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

Thanks,
Ming Lei

[PATCH v1 1/7] block: export blkdev_reread_part() and __blkdev_reread_part()

[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@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
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 4e858fd..dac6354 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2274,6 +2274,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.7.9.5

[PATCH v1 2/7] block: loop: don't hold lo_ctl_mutex in lo_open

[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.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/block/loop.c |   19 +++++++++++--------
 drivers/block/loop.h |    2 +-
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c4fd1e4..0d9f014 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -881,7 +881,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;
@@ -890,6 +890,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;
@@ -921,6 +924,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;
@@ -1378,9 +1383,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;
@@ -1391,11 +1394,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)
+	if (atomic_dec_return(&lo->lo_refcnt))
 		goto out;
 
+	mutex_lock(&lo->lo_ctl_mutex);
 	if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
 		/*
 		 * In autoclear mode, stop the loop thread
@@ -1648,6 +1650,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;
@@ -1765,7 +1768,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 301c27f..ffb6dd6 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.7.9.5

[PATCH v1 3/7] block: loop: fix another reread part failure

[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().

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 0d9f014..9f17054 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -530,6 +530,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
@@ -578,7 +600,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:
@@ -809,7 +831,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).
@@ -927,7 +949,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;
@@ -1002,7 +1024,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.7.9.5

[PATCH v1 4/7] block: nbd: convert to blkdev_reread_part()

[Ming Lei]

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.7.9.5

[PATCH v1 5/7] block: dasd_genhd: convert to blkdev_reread_part

[Ming Lei]

Also remove the obsolete comment.

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.7.9.5

[PATCH v1 6/7] block: replace trylock with mutex_lock in blkdev_reread_part()

[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.

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.7.9.5

[PATCH v1 7/7] s390/block/dasd: remove obsolete while -EBUSY loop

[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
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/s390/block/dasd_genhd.c |   13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 2af4619..189ea2f 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,8 @@ 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--;
-		DBF_DEV_EVENT(DBF_ERR, block->base,
-			      "scan partitions error, retry %d rc %d",
-			      retry, rc);
-	}
+	DBF_DEV_EVENT(DBF_ERR, block->base,
+		      "scan partitions error, rc %d", rc);
 
 	/*
 	 * Since the matching blkdev_put call to the blkdev_get in
-- 
1.7.9.5

Re: [PATCH v1 0/7] block: reread partitions changes and fix for loop

[Jarod Wilson]

On Wed, Apr 08, 2015 at 11:52:57PM +0800, Ming Lei wrote:
> 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.
> 
> 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)

I tinkered with the atomic replacement and added the lockdep assert on my
end as well, and have been testing. My end results were nearly identical
to this, and testing for my particular use case still looks good.

For the set:

Tested-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Jarod Wilson <jarod@redhat.com>

Thanks much!

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH v1 7/7] s390/block/dasd: remove obsolete while -EBUSY loop

[Sebastian Ott]

On Wed, 8 Apr 2015, Ming Lei wrote:
> 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
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/s390/block/dasd_genhd.c |   13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
> index 2af4619..189ea2f 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,8 @@ 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--;
> -		DBF_DEV_EVENT(DBF_ERR, block->base,
> -			      "scan partitions error, retry %d rc %d",
> -			      retry, rc);
> -	}
> +	DBF_DEV_EVENT(DBF_ERR, block->base,
> +		      "scan partitions error, rc %d", rc);

Could you please change that to only write the debug message in the error
case. Other than that, both dasd patches look good.

Thanks,
Sebastian

> 
>  	/*
>  	 * Since the matching blkdev_put call to the blkdev_get in
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH v1 7/7] s390/block/dasd: remove obsolete while -EBUSY loop

[Jarod Wilson]

On Wed, Apr 08, 2015 at 07:32:24PM +0200, Sebastian Ott wrote:
> On Wed, 8 Apr 2015, Ming Lei wrote:
> > 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
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > ---
> >  drivers/s390/block/dasd_genhd.c |   13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
> > index 2af4619..189ea2f 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,8 @@ 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--;
> > -		DBF_DEV_EVENT(DBF_ERR, block->base,
> > -			      "scan partitions error, retry %d rc %d",
> > -			      retry, rc);
> > -	}
> > +	DBF_DEV_EVENT(DBF_ERR, block->base,
> > +		      "scan partitions error, rc %d", rc);
> 
> Could you please change that to only write the debug message in the error
> case. Other than that, both dasd patches look good.

D'oh, absolutely. Ming, do you want me to send an updated patch 7 along in
reply here, or do you want to handle it and/or repost the entire set?

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH v1 7/7] s390/block/dasd: remove obsolete while -EBUSY loop

[Ming Lei]

On Thu, Apr 9, 2015 at 2:00 AM, Jarod Wilson <jarod@redhat.com> wrote:
> On Wed, Apr 08, 2015 at 07:32:24PM +0200, Sebastian Ott wrote:
>> On Wed, 8 Apr 2015, Ming Lei wrote:
>> > 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
>> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
>> > ---
>> >  drivers/s390/block/dasd_genhd.c |   13 +++----------
>> >  1 file changed, 3 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
>> > index 2af4619..189ea2f 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,8 @@ 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--;
>> > -           DBF_DEV_EVENT(DBF_ERR, block->base,
>> > -                         "scan partitions error, retry %d rc %d",
>> > -                         retry, rc);
>> > -   }
>> > +   DBF_DEV_EVENT(DBF_ERR, block->base,
>> > +                 "scan partitions error, rc %d", rc);
>>
>> Could you please change that to only write the debug message in the error
>> case. Other than that, both dasd patches look good.
>
> D'oh, absolutely. Ming, do you want me to send an updated patch 7 along in
> reply here, or do you want to handle it and/or repost the entire set?

I will handle that, and will add your guys' acked-by and tested-by too,
but need to take a while for looking if there are further comments.

Thanks,
Ming Lei

Re: [PATCH v1 7/7] s390/block/dasd: remove obsolete while -EBUSY loop

[Ming Lei]

On Thu, Apr 9, 2015 at 2:00 AM, Jarod Wilson <jarod@redhat.com> wrote:
> On Wed, Apr 08, 2015 at 07:32:24PM +0200, Sebastian Ott wrote:
>> On Wed, 8 Apr 2015, Ming Lei wrote:
>> > 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
>> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
>> > ---
>> >  drivers/s390/block/dasd_genhd.c |   13 +++----------
>> >  1 file changed, 3 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
>> > index 2af4619..189ea2f 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,8 @@ 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--;
>> > -           DBF_DEV_EVENT(DBF_ERR, block->base,
>> > -                         "scan partitions error, retry %d rc %d",
>> > -                         retry, rc);
>> > -   }
>> > +   DBF_DEV_EVENT(DBF_ERR, block->base,
>> > +                 "scan partitions error, rc %d", rc);
>>
>> Could you please change that to only write the debug message in the error
>> case. Other than that, both dasd patches look good.
>
> D'oh, absolutely. Ming, do you want me to send an updated patch 7 along in
> reply here, or do you want to handle it and/or repost the entire set?

I will handle that, and will add your guys' acked-by and tested-by too,
but need to take a while for looking if there are further comments.

Thanks,
Ming Lei