From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
To: Minchan Kim <minchan@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Ganesh Mahendran <opensource.ganesh@gmail.com>,
Jerome Marchand <jmarchan@redhat.com>,
Nitin Gupta <ngupta@vflare.org>,
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
linux-kernel@vger.kernel.org,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: [PATCH] zram: rework reset and destroy path
Date: Wed, 4 Feb 2015 01:15:06 +0900 [thread overview]
Message-ID: <1422980106-28222-1-git-send-email-sergey.senozhatsky@gmail.com> (raw)
We need to return set_capacity(disk, 0) from reset_store() back
to zram_reset_device(), a catch by Ganesh Mahendran. Potentially,
we can race set_capacity() calls from init and reset paths.
The problem is that zram_reset_device() is also getting called
from zram_exit(), which performs operations in misleading
reversed order -- we first create_device() and then init it,
while zram_exit() perform destroy_device() first and then does
zram_reset_device(). This is done to remove sysfs group before
we reset device, so we can continue with device reset/destruction
not being raced by sysfs attr write (f.e. disksize).
Apart from that, destroy_device() releases zram->disk (but we
still have ->disk pointer), so we cannot acces zram->disk in
later zram_reset_device() call, which may cause additional
errors in the future.
So, this patch rework and cleanup destroy path.
1) remove several unneeded goto labels in zram_init()
2) factor out zram_init() error path and zram_exit() into
destroy_devices() function, which takes the number of devices
to destroy as its argument.
3) remove sysfs group in destroy_devices() first, so we can
reorder operations -- reset device (as expected) goes before
disk destroy and queue cleanup. So we can always access ->disk
in zram_reset_device().
4) and, finally, return set_capacity() back under ->init_lock.
Reported-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/block/zram/zram_drv.c | 71 ++++++++++++++++++-------------------------
1 file changed, 30 insertions(+), 41 deletions(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a32069f..7d2e86f 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -734,8 +734,9 @@ static void zram_reset_device(struct zram *zram)
zram->meta = NULL;
/* Reset stats */
memset(&zram->stats, 0, sizeof(zram->stats));
-
zram->disksize = 0;
+ set_capacity(zram->disk, 0);
+
up_write(&zram->init_lock);
}
@@ -828,7 +829,6 @@ static ssize_t reset_store(struct device *dev,
/* Make sure all pending I/O is finished */
fsync_bdev(bdev);
zram_reset_device(zram);
- set_capacity(zram->disk, 0);
mutex_unlock(&bdev->bd_mutex);
revalidate_disk(zram->disk);
@@ -1114,15 +1114,29 @@ out:
return ret;
}
-static void destroy_device(struct zram *zram)
+static void destroy_devices(unsigned int nr)
{
- sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
- &zram_disk_attr_group);
+ struct zram *zram;
+ unsigned int i;
- del_gendisk(zram->disk);
- put_disk(zram->disk);
+ for (i = 0; i < nr; i++) {
+ zram = &zram_devices[i];
+ /* remove sysfs first, so no one will perform disksize
+ * store while we destroying devices */
+ sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
+ &zram_disk_attr_group);
- blk_cleanup_queue(zram->queue);
+ zram_reset_device(zram);
+
+ del_gendisk(zram->disk);
+ put_disk(zram->disk);
+
+ blk_cleanup_queue(zram->queue);
+ }
+
+ kfree(zram_devices);
+ unregister_blkdev(zram_major, "zram");
+ pr_debug("Destroyed %u device(s)\n", nr);
}
static int __init zram_init(void)
@@ -1132,64 +1146,39 @@ static int __init zram_init(void)
if (num_devices > max_num_devices) {
pr_warn("Invalid value for num_devices: %u\n",
num_devices);
- ret = -EINVAL;
- goto out;
+ return -EINVAL;
}
zram_major = register_blkdev(0, "zram");
if (zram_major <= 0) {
pr_warn("Unable to get major number\n");
- ret = -EBUSY;
- goto out;
+ return -EBUSY;
}
/* Allocate the device array and initialize each one */
zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
if (!zram_devices) {
ret = -ENOMEM;
- goto unregister;
+ goto out_error;
}
for (dev_id = 0; dev_id < num_devices; dev_id++) {
ret = create_device(&zram_devices[dev_id], dev_id);
if (ret)
- goto free_devices;
+ goto out_error;
}
- pr_info("Created %u device(s) ...\n", num_devices);
-
+ pr_info("Created %u device(s)\n", num_devices);
return 0;
-free_devices:
- while (dev_id)
- destroy_device(&zram_devices[--dev_id]);
- kfree(zram_devices);
-unregister:
- unregister_blkdev(zram_major, "zram");
-out:
+out_error:
+ destroy_devices(dev_id);
return ret;
}
static void __exit zram_exit(void)
{
- int i;
- struct zram *zram;
-
- for (i = 0; i < num_devices; i++) {
- zram = &zram_devices[i];
-
- destroy_device(zram);
- /*
- * Shouldn't access zram->disk after destroy_device
- * because destroy_device already released zram->disk.
- */
- zram_reset_device(zram);
- }
-
- unregister_blkdev(zram_major, "zram");
-
- kfree(zram_devices);
- pr_debug("Cleanup done!\n");
+ destroy_devices(num_devices);
}
module_init(zram_init);
--
2.3.0.rc2
next reply other threads:[~2015-02-03 16:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-03 16:15 Sergey Senozhatsky [this message]
2015-02-03 23:42 ` [PATCH] zram: rework reset and destroy path Minchan Kim
2015-02-04 0:24 ` Sergey Senozhatsky
2015-02-04 0:35 ` Minchan Kim
2015-02-04 0:45 ` Sergey Senozhatsky
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=1422980106-28222-1-git-send-email-sergey.senozhatsky@gmail.com \
--to=sergey.senozhatsky@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=jmarchan@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=minchan@kernel.org \
--cc=ngupta@vflare.org \
--cc=opensource.ganesh@gmail.com \
--cc=sergey.senozhatsky.work@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).