[PATCH 0/4] introduce new put_getdisk() call

[PATCH 0/4] introduce new put_getdisk() call

[Roman Pen]

Hello.

In this patchset in the first patch I fixed module reference leak inside
blk-cgroup.c.  In other patches I switched to a new put_getdisk() call,
which should be used if the disk was received by get_disk() or get_gendisk()
functions, which internally increase module reference.

The idea is to avoid confusion in the future and to have symmetric calls:

	alloc_disk() -> put_disk() [as it is done in all the block drivers]

and

	get_gendisk() -> put_gendisk() [if you need to find a disk by minor,major]

The second sequence internally increases disk owner module reference on
get and decreases it on put.

Roman Pen (4):
  block: fix module reference leak on put_disk() call for cgroups
    throttle
  block: introduce new call put_gendisk() in genhd.c
  block,fs: switch to a new put_gendisk() call
  hibernate: fix disk and module leak on successfull resume

 block/blk-cgroup.c       |  6 ++---
 block/genhd.c            | 59 +++++++++++++++++++++++++++++++++++++++++++++---
 fs/block_dev.c           | 24 ++++++--------------
 include/linux/genhd.h    |  1 +
 kernel/power/hibernate.c |  5 +++-
 5 files changed, 71 insertions(+), 24 deletions(-)

Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Gi-Oh Kim <gi-oh.kim@profitbricks.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Len Brown <len.brown@intel.com>
Cc: Ming Lei <tom.leiming@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: linux-block@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org

-- 
2.6.2

[PATCH 3/4] block,fs: switch to a new put_gendisk() call

[Roman Pen]

In previous patch a new put_gendisk() call was introduced, which puts the
disk and the disk owner module references.

This new call should help not to forget about required module_put() after
receiving a disk pointer using get_disk() or get_gendisk() calls.

In this patch disk_put(),module_put() sequences are replaced with a single
put_gendisk() call.

Signed-off-by: Roman Pen <roman.penyaev@profitbricks.com>
Cc: Gi-Oh Kim <gi-oh.kim@profitbricks.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-block@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
---
 block/blk-cgroup.c | 15 +++------------
 fs/block_dev.c     | 24 +++++++-----------------
 2 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 66e6f1a..2ed9636 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -788,7 +788,6 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 {
 	struct gendisk *disk;
 	struct blkcg_gq *blkg;
-	struct module *owner;
 	unsigned int major, minor;
 	int key_len, part, ret;
 	char *body;
@@ -805,9 +804,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 	if (!disk)
 		return -ENODEV;
 	if (part) {
-		owner = disk->fops->owner;
-		put_disk(disk);
-		module_put(owner);
+		put_gendisk(disk);
 		return -ENODEV;
 	}
 
@@ -823,9 +820,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 		ret = PTR_ERR(blkg);
 		rcu_read_unlock();
 		spin_unlock_irq(disk->queue->queue_lock);
-		owner = disk->fops->owner;
-		put_disk(disk);
-		module_put(owner);
+		put_gendisk(disk);
 		/*
 		 * If queue was bypassing, we should retry.  Do so after a
 		 * short msleep().  It isn't strictly necessary but queue
@@ -856,13 +851,9 @@ EXPORT_SYMBOL_GPL(blkg_conf_prep);
 void blkg_conf_finish(struct blkg_conf_ctx *ctx)
 	__releases(ctx->disk->queue->queue_lock) __releases(rcu)
 {
-	struct module *owner;
-
 	spin_unlock_irq(ctx->disk->queue->queue_lock);
 	rcu_read_unlock();
-	owner = ctx->disk->fops->owner;
-	put_disk(ctx->disk);
-	module_put(owner);
+	put_gendisk(ctx->disk);
 }
 EXPORT_SYMBOL_GPL(blkg_conf_finish);
 
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7b9cd49..dc2ea76 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -15,7 +15,6 @@
 #include <linux/highmem.h>
 #include <linux/blkdev.h>
 #include <linux/backing-dev.h>
-#include <linux/module.h>
 #include <linux/blkpg.h>
 #include <linux/magic.h>
 #include <linux/buffer_head.h>
@@ -870,8 +869,7 @@ static struct block_device *bd_start_claiming(struct block_device *bdev,
 	else
 		whole = bdgrab(bdev);
 
-	module_put(disk->fops->owner);
-	put_disk(disk);
+	put_gendisk(disk);
 	if (!whole)
 		return ERR_PTR(-ENOMEM);
 
@@ -1167,7 +1165,6 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
 static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 {
 	struct gendisk *disk;
-	struct module *owner;
 	int ret;
 	int partno;
 	int perm = 0;
@@ -1193,7 +1190,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 	disk = get_gendisk(bdev->bd_dev, &partno);
 	if (!disk)
 		goto out;
-	owner = disk->fops->owner;
 
 	disk_block_events(disk);
 	mutex_lock_nested(&bdev->bd_mutex, for_part);
@@ -1222,8 +1218,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 					bdev->bd_queue = NULL;
 					mutex_unlock(&bdev->bd_mutex);
 					disk_unblock_events(disk);
-					put_disk(disk);
-					module_put(owner);
+					put_gendisk(disk);
 					goto restart;
 				}
 			}
@@ -1285,9 +1280,8 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 			if (ret)
 				goto out_unlock_bdev;
 		}
-		/* only one opener holds refs to the module and disk */
-		put_disk(disk);
-		module_put(owner);
+		/* only one opener holds refs to the disk */
+		put_gendisk(disk);
 	}
 	bdev->bd_openers++;
 	if (for_part)
@@ -1307,8 +1301,7 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
  out_unlock_bdev:
 	mutex_unlock(&bdev->bd_mutex);
 	disk_unblock_events(disk);
-	put_disk(disk);
-	module_put(owner);
+	put_gendisk(disk);
  out:
 	bdput(bdev);
 
@@ -1525,7 +1518,7 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
 		/*
 		 * Detaching bdev inode from its wb in __destroy_inode()
 		 * is too late: the queue which embeds its bdi (along with
-		 * root wb) can be gone as soon as we put_disk() below.
+		 * root wb) can be gone as soon as we put_gendisk() below.
 		 */
 		inode_detach_wb(bdev->bd_inode);
 	}
@@ -1534,8 +1527,6 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
 			disk->fops->release(disk, mode);
 	}
 	if (!bdev->bd_openers) {
-		struct module *owner = disk->fops->owner;
-
 		disk_put_part(bdev->bd_part);
 		bdev->bd_part = NULL;
 		bdev->bd_disk = NULL;
@@ -1543,8 +1534,7 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
 			victim = bdev->bd_contains;
 		bdev->bd_contains = NULL;
 
-		put_disk(disk);
-		module_put(owner);
+		put_gendisk(disk);
 	}
 	mutex_unlock(&bdev->bd_mutex);
 	bdput(bdev);
-- 
2.6.2

Re: [PATCH 0/4] introduce new put_getdisk() call

[Tejun Heo]

On Mon, Feb 01, 2016 at 03:51:51PM +0100, Roman Pen wrote:
> Hello.
> 
> In this patchset in the first patch I fixed module reference leak inside
> blk-cgroup.c.  In other patches I switched to a new put_getdisk() call,
> which should be used if the disk was received by get_disk() or get_gendisk()
> functions, which internally increase module reference.
> 
> The idea is to avoid confusion in the future and to have symmetric calls:
> 
> 	alloc_disk() -> put_disk() [as it is done in all the block drivers]
> 
> and
> 
> 	get_gendisk() -> put_gendisk() [if you need to find a disk by minor,major]
> 
> The second sequence internally increases disk owner module reference on
> get and decreases it on put.

For the entire series,

 Acked-by: Tejun Heo <tj@kernel.org>

Nice catch, thanks!

-- 
tejun