All of lore.kernel.org
 help / color / mirror / Atom feed
From: Roman Pen <roman.penyaev@profitbricks.com>
To: unlisted-recipients:; (no To-header on input)
Cc: Roman Pen <roman.penyaev@profitbricks.com>,
	Gi-Oh Kim <gi-oh.kim@profitbricks.com>,
	Jens Axboe <axboe@kernel.dk>,
	Dan Williams <dan.j.williams@intel.com>,
	Sagi Grimberg <sagig@mellanox.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Ming Lei <tom.leiming@gmail.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH 2/4] block: introduce new call put_gendisk() in genhd.c
Date: Mon,  1 Feb 2016 15:51:53 +0100	[thread overview]
Message-ID: <1454338315-13465-3-git-send-email-roman.penyaev@profitbricks.com> (raw)
In-Reply-To: <1454338315-13465-1-git-send-email-roman.penyaev@profitbricks.com>

The confusion comes from the fact, that if disk was get using

    get_disk()
    get_gendisk()

calls, you also have to put a disk owner module reference, so the sequence
is the following:

    disk = get_gendisk(...);

    /* use disk */

    owner = disk->fops->owner;
    put_disk(disk);
    module_put(owner);

The disk, which was allocated using alloc_disk() requires only the opposite
put_disk() call, without puting the module reference.

That's error prone.

Here in this patch new call is introduced put_gendisk(), which is aimed to
put a disk reference and also a disk owner reference.

The expected usage for the block drivers has not been changed and is the
following:

    disk = alloc_disk(...);

    /* use */

    put_disk(disk);

The expected usage for those who did get_gendisk():

    disk = get_gendisk();

    /* use */

    put_gendisk(disk);

That should help to get rid of modules references leak, which happens if
disk was received by get_gendisk() call, but then was put by put_disk(),
without corresponding module_put().

Also function description is updated.

Signed-off-by: Roman Pen <roman.penyaev@profitbricks.com>
Cc: Gi-Oh Kim <gi-oh.kim@profitbricks.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Ming Lei <tom.leiming@gmail.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: linux-block@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 block/genhd.c         | 59 ++++++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/genhd.h |  1 +
 2 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 0f566a5..66c8278 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -722,7 +722,13 @@ static ssize_t disk_badblocks_store(struct device *dev,
  * @partno: returned partition index
  *
  * This function gets the structure containing partitioning
- * information for the given device @devt.
+ * information for the given device @devt and increases the kobj
+ * and disk owner module references.
+ *
+ * RETURNS:
+ * Resulting gendisk on success, NULL in case of failure. After usage
+ * disk must be put with put_gendisk() call, which also puts the module
+ * reference.
  */
 struct gendisk *get_gendisk(dev_t devt, int *partno)
 {
@@ -1305,6 +1311,17 @@ struct gendisk *alloc_disk(int minors)
 }
 EXPORT_SYMBOL(alloc_disk);
 
+/**
+ * alloc_disk_node - allocates disk for specified minors and node id.
+ *
+ * @minors: how many minors are expected for that disk before switching
+ *          to extended block range
+ * @node_id: memory node from which to allocate
+ *
+ * RETURNS:
+ * Resulting disk on success, NULL in case of failure.
+ * alloc_disk() and alloc_disk_node() are paired with put_disk() call.
+ */
 struct gendisk *alloc_disk_node(int minors, int node_id)
 {
 	struct gendisk *disk;
@@ -1349,6 +1366,16 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
 }
 EXPORT_SYMBOL(alloc_disk_node);
 
+/**
+ * get_disk - increases the kobj and module references.
+ *
+ * @disk: disk for which references should be increased
+ *
+ * RETURNS:
+ * Resulting kobj of the disk on success, NULL in case of failure.
+ * After usage disk must be put with put_gendisk() call, which also
+ * puts the module reference.
+ */
 struct kobject *get_disk(struct gendisk *disk)
 {
 	struct module *owner;
@@ -1367,17 +1394,43 @@ struct kobject *get_disk(struct gendisk *disk)
 	return kobj;
 
 }
-
 EXPORT_SYMBOL(get_disk);
 
+/**
+ * put_disk - puts only kobj reference.
+ *
+ * @disk: disk to put
+ *
+ * This put_disk() is paired with alloc_disk().  Never call this
+ * if you get a disk using get_gendisk() or get_disk() calls.
+ */
 void put_disk(struct gendisk *disk)
 {
 	if (disk)
 		kobject_put(&disk_to_dev(disk)->kobj);
 }
-
 EXPORT_SYMBOL(put_disk);
 
+/**
+ * put_gendisk - puts kobj and disk owner module references.
+ *
+ * @disk: disk to put
+ *
+ * This put_gendisk() is paired with get_gendisk() and get_disk() calls.
+ * Never use this call if you just have allocated a disk using alloc_disk(),
+ * use put_disk() instead.
+ */
+void put_gendisk(struct gendisk *disk)
+{
+	if (disk) {
+		struct module *owner = disk->fops->owner;
+
+		put_disk(disk);
+		module_put(owner);
+	}
+}
+EXPORT_SYMBOL(put_gendisk);
+
 static void set_disk_ro_uevent(struct gendisk *gd, int ro)
 {
 	char event[] = "DISK_RO=1";
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 9e4e547..f84efbf 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -635,6 +635,7 @@ extern struct gendisk *alloc_disk_node(int minors, int node_id);
 extern struct gendisk *alloc_disk(int minors);
 extern struct kobject *get_disk(struct gendisk *disk);
 extern void put_disk(struct gendisk *disk);
+extern void put_gendisk(struct gendisk *disk);
 extern void blk_register_region(dev_t devt, unsigned long range,
 			struct module *module,
 			struct kobject *(*probe)(dev_t, int *, void *),
-- 
2.6.2

  parent reply	other threads:[~2016-02-01 14:53 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-01 14:51 [PATCH 0/4] introduce new put_getdisk() call Roman Pen
2016-02-01 14:51 ` Roman Pen
2016-02-01 14:51 ` [PATCH 1/4] block: fix module reference leak on put_disk() call for cgroups throttle Roman Pen
2016-02-03 10:40   ` Gi-Oh Kim
2016-02-01 14:51 ` Roman Pen [this message]
2016-02-01 14:51 ` [PATCH 3/4] block,fs: switch to a new put_gendisk() call Roman Pen
2016-02-01 14:51 ` [PATCH 4/4] hibernate: fix disk and module leak on successfull resume Roman Pen
2016-02-01 14:51   ` Roman Pen
2016-02-03  1:37   ` Rafael J. Wysocki
2016-02-01 17:16 ` [PATCH 0/4] introduce new put_getdisk() call Tejun Heo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1454338315-13465-3-git-send-email-roman.penyaev@profitbricks.com \
    --to=roman.penyaev@profitbricks.com \
    --cc=axboe@kernel.dk \
    --cc=dan.j.williams@intel.com \
    --cc=gi-oh.kim@profitbricks.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=sagig@mellanox.com \
    --cc=tom.leiming@gmail.com \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.