[PATCHv2 00/10] cleaned up on-demand device creation

[PATCHv2 00/10] cleaned up on-demand device creation

[Sergey Senozhatsky]

Hello,

resending on-demand device creation patch set. sadly, I managed to create a
mess; so here is my take to clean it up, fold patches and, hopefully, see
them in 4.1.

this mess will not happen again.


Andrew picked up some of the commits lined up for 4.1, which required manual
editing. sorry for that inconvenience.


unfortunately, commit c72c6160d967ed26a0b136dbab337f821d233509
  Author: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
  Date:   Wed Apr 15 16:15:55 2015 -0700

     zram: move compact_store() to sysfs functions area


ended up to be different: from a cosmetic change it has transformed into
a functional change.

I fix it in 0001-zram-enable-compaction-support-in-zram.patch.


the rest is functionally identical to what we had in linux-next and mmotm for
quite some time: in linux-next since Wed Apr 8 09:44:43 2015 +1000
(commit 273b0791dae2f0b).

it would be nice to see it in 4.1, if possible.

no functional change in zram_drv.c file, compared to zram_drv.c from
linux-next-20150415 (yes, actually checked). just a couple of additional
comment tweaks.

like:

-/* allocate and initialize new zram device. the function returns
- * '>= 0' device_id upon success, and negative value otherwise. */
+/*
+ * Allocate and initialize new zram device. the function returns
+ * '>= 0' device_id upon success, and negative value otherwise.
+ */

or

        /*
         * First, make ->disksize device attr RO, closing
-        * ZRAM_CTL_REMOVE vs disksize_store() race window
+        * zram_remove() vs disksize_store() race window
         */


I also picked up the remaining part of Julia Lawall's
<Julia.Lawall@lip6.fr> ("zram: fix error return code") commit.

Documentation is identical to linux-next-20150415 version.


8<-------

We currently don't support zram on-demand device creation.  The only way
to have N zram devices is to specify num_devices module parameter (default
value 1).  That means that if, for some reason, at some point, user wants
to have N + 1 devies he/she must umount all the existing devices, unload
the module, load the module passing num_devices equals to N + 1.  And do
this again, if needed.

This patchset introduces zram-control sysfs class, which has two sysfs
attrs:

 - zram_add     -- add a new zram device
 - zram_remove  -- remove a specific (device_id) zram device

    Usage example:
        # add a new specific zram device
        cat /sys/class/zram-control/zram_add
        1

        # remove a specific zram device
        echo 4 > /sys/class/zram-control/zram_remove

The patchset also does some cleanups and huge code reorganization.


-ss


Sergey Senozhatsky (10):
  zram: enable compaction support in zram
  zram: cosmetic ZRAM_ATTR_RO code formatting tweak
  zram: use idr instead of `zram_devices' array
  zram: factor out device reset from reset_store()
  zram: reorganize code layout
  zram: remove max_num_devices limitation
  zram: report every added and removed device
  zram: trivial: correct flag operations comment
  zram: return zram device_id value from zram_add()
  zram: add dynamic device add/remove functionality

 Documentation/ABI/testing/sysfs-class-zram |  24 +
 Documentation/blockdev/zram.txt            |  31 +-
 drivers/block/zram/zram_drv.c              | 939 +++++++++++++++++------------
 drivers/block/zram/zram_drv.h              |   6 -
 4 files changed, 597 insertions(+), 403 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-zram

-- 
2.4.0.rc1.29.gecc46a1

[PATCHv2 01/10] zram: enable compaction support in zram

[Sergey Senozhatsky]

Commit c72c6160d967 ("zram: move compact_store() to sysfs functions area")
was intended to be a cosmetic change that moved function around w/o any
functional change:
http://lkml.iu.edu/hypermail/linux/kernel/1503.1/01818.html

Unfortunately, on its way from mmotm to Linus's tree it was altered and turned
into "remove compaction support from zram" commit. I've managed to create a
mess of commits and Andrew had to pick some of the commits and hand edit them.

Fix it and learn my lessons.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/blockdev/zram.txt |  2 +-
 drivers/block/zram/zram_drv.c   | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 48a183e..bef4998 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -126,7 +126,7 @@ mem_used_max      RW    the maximum amount memory zram have consumed to
 mem_limit         RW    the maximum amount of memory ZRAM can use to store
                         the compressed data
 num_migrated      RO    the number of objects migrated migrated by compaction
-
+compact           WO    trigger memory compaction
 
 WARNING
 =======
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index c94386a..fa6ea76 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1038,6 +1038,7 @@ static const struct block_device_operations zram_devops = {
 	.owner = THIS_MODULE
 };
 
+static DEVICE_ATTR_WO(compact);
 static DEVICE_ATTR_RW(disksize);
 static DEVICE_ATTR_RO(initstate);
 static DEVICE_ATTR_WO(reset);
@@ -1095,6 +1096,27 @@ static ssize_t mm_stat_show(struct device *dev,
 	return ret;
 }
 
+static ssize_t compact_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	unsigned long nr_migrated;
+	struct zram *zram = dev_to_zram(dev);
+	struct zram_meta *meta;
+
+	down_read(&zram->init_lock);
+	if (!init_done(zram)) {
+		up_read(&zram->init_lock);
+		return -EINVAL;
+	}
+
+	meta = zram->meta;
+	nr_migrated = zs_compact(meta->mem_pool);
+	atomic64_add(nr_migrated, &zram->stats.num_migrated);
+	up_read(&zram->init_lock);
+
+	return len;
+}
+
 static DEVICE_ATTR_RO(io_stat);
 static DEVICE_ATTR_RO(mm_stat);
 ZRAM_ATTR_RO(num_reads);
@@ -1114,6 +1136,7 @@ static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_num_writes.attr,
 	&dev_attr_failed_reads.attr,
 	&dev_attr_failed_writes.attr,
+	&dev_attr_compact.attr,
 	&dev_attr_invalid_io.attr,
 	&dev_attr_notify_free.attr,
 	&dev_attr_zero_pages.attr,
-- 
2.4.0.rc2

[PATCHv2 02/10] zram: cosmetic ZRAM_ATTR_RO code formatting tweak

[Sergey Senozhatsky]

Fix a misplaced backslash.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index fa6ea76..14043b2 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -53,7 +53,7 @@ static inline void deprecated_attr_warn(const char *name)
 }
 
 #define ZRAM_ATTR_RO(name)						\
-static ssize_t name##_show(struct device *d,		\
+static ssize_t name##_show(struct device *d,				\
 				struct device_attribute *attr, char *b)	\
 {									\
 	struct zram *zram = dev_to_zram(d);				\
-- 
2.4.0.rc2

[PATCHv2 03/10] zram: use idr instead of `zram_devices' array

[Sergey Senozhatsky]

This patch makes some preparations for on-demand device add/remove
functionality.

Remove `zram_devices' array and switch to id-to-pointer translation (idr).
idr doesn't bloat zram struct with additional members, f.e. list_head,
yet still provides ability to match the device_id with the device pointer.

No user-space visible changes.

[do not lose -ENOMEM return code when `queue' allocation has failed]
Reported-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 86 ++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 37 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 14043b2..4511830 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -32,12 +32,12 @@
 #include <linux/string.h>
 #include <linux/vmalloc.h>
 #include <linux/err.h>
+#include <linux/idr.h>
 
 #include "zram_drv.h"
 
-/* Globals */
+static DEFINE_IDR(zram_index_idr);
 static int zram_major;
-static struct zram *zram_devices;
 static const char *default_compressor = "lzo";
 
 /* Module params (documentation at end) */
@@ -1156,10 +1156,20 @@ static struct attribute_group zram_disk_attr_group = {
 	.attrs = zram_disk_attrs,
 };
 
-static int create_device(struct zram *zram, int device_id)
+static int zram_add(int device_id)
 {
+	struct zram *zram;
 	struct request_queue *queue;
-	int ret = -ENOMEM;
+	int ret;
+
+	zram = kzalloc(sizeof(struct zram), GFP_KERNEL);
+	if (!zram)
+		return -ENOMEM;
+
+	ret = idr_alloc(&zram_index_idr, zram, device_id,
+			device_id + 1, GFP_KERNEL);
+	if (ret < 0)
+		goto out_free_dev;
 
 	init_rwsem(&zram->init_lock);
 
@@ -1167,12 +1177,13 @@ static int create_device(struct zram *zram, int device_id)
 	if (!queue) {
 		pr_err("Error allocating disk queue for device %d\n",
 			device_id);
-		goto out;
+		ret = -ENOMEM;
+		goto out_free_idr;
 	}
 
 	blk_queue_make_request(queue, zram_make_request);
 
-	 /* gendisk structure */
+	/* gendisk structure */
 	zram->disk = alloc_disk(1);
 	if (!zram->disk) {
 		pr_warn("Error allocating disk structure for device %d\n",
@@ -1237,34 +1248,42 @@ out_free_disk:
 	put_disk(zram->disk);
 out_free_queue:
 	blk_cleanup_queue(queue);
-out:
+out_free_idr:
+	idr_remove(&zram_index_idr, device_id);
+out_free_dev:
+	kfree(zram);
 	return ret;
 }
 
-static void destroy_devices(unsigned int nr)
+static void zram_remove(struct zram *zram)
 {
-	struct zram *zram;
-	unsigned int i;
-
-	for (i = 0; i < nr; i++) {
-		zram = &zram_devices[i];
-		/*
-		 * Remove sysfs first, so no one will perform a disksize
-		 * store while we destroy the devices
-		 */
-		sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
-				&zram_disk_attr_group);
+	/*
+	 * Remove sysfs first, so no one will perform a disksize
+	 * store while we destroy the devices
+	 */
+	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
+			&zram_disk_attr_group);
 
-		zram_reset_device(zram);
+	zram_reset_device(zram);
+	idr_remove(&zram_index_idr, zram->disk->first_minor);
+	blk_cleanup_queue(zram->disk->queue);
+	del_gendisk(zram->disk);
+	put_disk(zram->disk);
+	kfree(zram);
+}
 
-		blk_cleanup_queue(zram->disk->queue);
-		del_gendisk(zram->disk);
-		put_disk(zram->disk);
-	}
+static int zram_exit_cb(int id, void *ptr, void *data)
+{
+	zram_remove(ptr);
+	return 0;
+}
 
-	kfree(zram_devices);
+static void destroy_devices(void)
+{
+	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
+	idr_destroy(&zram_index_idr);
 	unregister_blkdev(zram_major, "zram");
-	pr_info("Destroyed %u device(s)\n", nr);
+	pr_info("Destroyed device(s)\n");
 }
 
 static int __init zram_init(void)
@@ -1283,16 +1302,9 @@ static int __init zram_init(void)
 		return -EBUSY;
 	}
 
-	/* Allocate the device array and initialize each one */
-	zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
-	if (!zram_devices) {
-		unregister_blkdev(zram_major, "zram");
-		return -ENOMEM;
-	}
-
 	for (dev_id = 0; dev_id < num_devices; dev_id++) {
-		ret = create_device(&zram_devices[dev_id], dev_id);
-		if (ret)
+		ret = zram_add(dev_id);
+		if (ret != 0)
 			goto out_error;
 	}
 
@@ -1300,13 +1312,13 @@ static int __init zram_init(void)
 	return 0;
 
 out_error:
-	destroy_devices(dev_id);
+	destroy_devices();
 	return ret;
 }
 
 static void __exit zram_exit(void)
 {
-	destroy_devices(num_devices);
+	destroy_devices();
 }
 
 module_init(zram_init);
-- 
2.4.0.rc2

[PATCHv2 04/10] zram: factor out device reset from reset_store()

[Sergey Senozhatsky]

Device reset currently includes two steps:
a) holding ->bd_mutex we ensure that there are no device users
(bdev->bd_openers)

b) and "internal" part (executed under bdev->bd_mutex and partially
under zram->init_lock) that resets the device - frees allocated
memory and returns the device back to its initial state.

Up until now it worked just fine. But there will be yet another device
reset user soon -- on-demand device removal. We currently can reuse (b),
but step (a) is done in sysfs ATTR reset_store() handler, which makes it
hard to use it in on-demand device reset path.

Rename step (b) from zram_reset_device() to zram_reset_device_internal()
and factor out step (a) to zram_reset_device().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 135 +++++++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 68 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 4511830..14ec8f2 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -750,48 +750,6 @@ static void zram_bio_discard(struct zram *zram, u32 index,
 	}
 }
 
-static void zram_reset_device(struct zram *zram)
-{
-	struct zram_meta *meta;
-	struct zcomp *comp;
-	u64 disksize;
-
-	down_write(&zram->init_lock);
-
-	zram->limit_pages = 0;
-
-	if (!init_done(zram)) {
-		up_write(&zram->init_lock);
-		return;
-	}
-
-	meta = zram->meta;
-	comp = zram->comp;
-	disksize = zram->disksize;
-	/*
-	 * Refcount will go down to 0 eventually and r/w handler
-	 * cannot handle further I/O so it will bail out by
-	 * check zram_meta_get.
-	 */
-	zram_meta_put(zram);
-	/*
-	 * We want to free zram_meta in process context to avoid
-	 * deadlock between reclaim path and any other locks.
-	 */
-	wait_event(zram->io_done, atomic_read(&zram->refcount) == 0);
-
-	/* Reset stats */
-	memset(&zram->stats, 0, sizeof(zram->stats));
-	zram->disksize = 0;
-	zram->max_comp_streams = 1;
-	set_capacity(zram->disk, 0);
-
-	up_write(&zram->init_lock);
-	/* I/O operation under all of CPU are done so let's free */
-	zram_meta_free(meta, disksize);
-	zcomp_destroy(comp);
-}
-
 static ssize_t disksize_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
@@ -850,16 +808,54 @@ out_free_meta:
 	return err;
 }
 
-static ssize_t reset_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t len)
+/* internal device reset part -- cleanup allocated memory and
+ * return back to initial state */
+static void zram_reset_device_internal(struct zram *zram)
 {
-	int ret;
-	unsigned short do_reset;
-	struct zram *zram;
-	struct block_device *bdev;
+	struct zram_meta *meta;
+	struct zcomp *comp;
+	u64 disksize;
 
-	zram = dev_to_zram(dev);
-	bdev = bdget_disk(zram->disk, 0);
+	down_write(&zram->init_lock);
+
+	zram->limit_pages = 0;
+
+	if (!init_done(zram)) {
+		up_write(&zram->init_lock);
+		return;
+	}
+
+	meta = zram->meta;
+	comp = zram->comp;
+	disksize = zram->disksize;
+	/*
+	 * Refcount will go down to 0 eventually and r/w handler
+	 * cannot handle further I/O so it will bail out by
+	 * check zram_meta_get.
+	 */
+	zram_meta_put(zram);
+	/*
+	 * We want to free zram_meta in process context to avoid
+	 * deadlock between reclaim path and any other locks.
+	 */
+	wait_event(zram->io_done, atomic_read(&zram->refcount) == 0);
+
+	/* Reset stats */
+	memset(&zram->stats, 0, sizeof(zram->stats));
+	zram->disksize = 0;
+	zram->max_comp_streams = 1;
+	set_capacity(zram->disk, 0);
+
+	up_write(&zram->init_lock);
+	/* I/O operation under all of CPU are done so let's free */
+	zram_meta_free(meta, disksize);
+	zcomp_destroy(comp);
+}
+
+static int zram_reset_device(struct zram *zram)
+{
+	int ret = 0;
+	struct block_device *bdev = bdget_disk(zram->disk, 0);
 
 	if (!bdev)
 		return -ENOMEM;
@@ -871,31 +867,34 @@ static ssize_t reset_store(struct device *dev,
 		goto out;
 	}
 
-	ret = kstrtou16(buf, 10, &do_reset);
-	if (ret)
-		goto out;
-
-	if (!do_reset) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
-	zram_reset_device(zram);
-
-	mutex_unlock(&bdev->bd_mutex);
-	revalidate_disk(zram->disk);
-	bdput(bdev);
-
-	return len;
-
+	zram_reset_device_internal(zram);
 out:
 	mutex_unlock(&bdev->bd_mutex);
 	bdput(bdev);
 	return ret;
 }
 
+static ssize_t reset_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	int ret;
+	unsigned short do_reset;
+	struct zram *zram;
+
+	zram = dev_to_zram(dev);
+	ret = kstrtou16(buf, 10, &do_reset);
+	if (ret)
+		return ret;
+
+	if (!do_reset)
+		return -EINVAL;
+
+	ret = zram_reset_device(zram);
+	return ret ? ret : len;
+}
+
 static void __zram_make_request(struct zram *zram, struct bio *bio)
 {
 	int offset, rw;
@@ -1264,7 +1263,7 @@ static void zram_remove(struct zram *zram)
 	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
 			&zram_disk_attr_group);
 
-	zram_reset_device(zram);
+	zram_reset_device_internal(zram);
 	idr_remove(&zram_index_idr, zram->disk->first_minor);
 	blk_cleanup_queue(zram->disk->queue);
 	del_gendisk(zram->disk);
-- 
2.4.0.rc2

[PATCHv2 05/10] zram: reorganize code layout

[Sergey Senozhatsky]

This patch looks big, but basically it just moves code blocks.
No functional changes.

Our current code layout looks like a sandwitch.

For example,
a) between read/write handlers, we have update_used_max() helper function:

static int zram_decompress_page
static int zram_bvec_read
static inline void update_used_max
static int zram_bvec_write
static int zram_bvec_rw

b) RW request handlers __zram_make_request/zram_bio_discard are divided by
sysfs attr reset_store() function and corresponding zram_reset_device()
handler:

static void zram_bio_discard
static void zram_reset_device
static ssize_t disksize_store
static ssize_t reset_store
static void __zram_make_request

c) we first a bunch of sysfs read/store functions. then a number of
one-liners, then helper functions, RW functions, sysfs functions, helper
functions again, and so on.

Reorganize layout to be more logically grouped (a brief description,
`cat zram_drv.c | grep static` gives a bigger picture):

-- one-liners: zram_test_flag/etc.

-- helpers: is_partial_io/update_position/etc

-- sysfs attr show/store functions + ZRAM_ATTR_RO() generated stats
show() functions
exception: reset and disksize store functions are required to be after meta()
functions. because we do device create/destroy actions in these sysfs
handlers.

-- "mm" functions: meta get/put, meta alloc/free, page free
static inline bool zram_meta_get
static inline void zram_meta_put
static void zram_meta_free
static struct zram_meta *zram_meta_alloc
static void zram_free_page

-- a block of I/O functions
static int zram_decompress_page
static int zram_bvec_read
static int zram_bvec_write
static void zram_bio_discard
static int zram_bvec_rw
static void __zram_make_request
static void zram_make_request
static void zram_slot_free_notify
static int zram_rw_page

-- device contol: add/remove/init/reset functions (+zram-control class
will sit here)
static void zram_reset_device_internal
static int zram_reset_device
static ssize_t reset_store
static ssize_t disksize_store
static int zram_add
static void zram_remove
static int __init zram_init
static void __exit zram_exit

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 703 +++++++++++++++++++++---------------------
 1 file changed, 351 insertions(+), 352 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 14ec8f2..e526fe1 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -74,6 +74,119 @@ static inline struct zram *dev_to_zram(struct device *dev)
 	return (struct zram *)dev_to_disk(dev)->private_data;
 }
 
+/* flag operations needs meta->tb_lock */
+static int zram_test_flag(struct zram_meta *meta, u32 index,
+			enum zram_pageflags flag)
+{
+	return meta->table[index].value & BIT(flag);
+}
+
+static void zram_set_flag(struct zram_meta *meta, u32 index,
+			enum zram_pageflags flag)
+{
+	meta->table[index].value |= BIT(flag);
+}
+
+static void zram_clear_flag(struct zram_meta *meta, u32 index,
+			enum zram_pageflags flag)
+{
+	meta->table[index].value &= ~BIT(flag);
+}
+
+static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
+{
+	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
+}
+
+static void zram_set_obj_size(struct zram_meta *meta,
+					u32 index, size_t size)
+{
+	unsigned long flags = meta->table[index].value >> ZRAM_FLAG_SHIFT;
+
+	meta->table[index].value = (flags << ZRAM_FLAG_SHIFT) | size;
+}
+
+static inline int is_partial_io(struct bio_vec *bvec)
+{
+	return bvec->bv_len != PAGE_SIZE;
+}
+
+/*
+ * Check if request is within bounds and aligned on zram logical blocks.
+ */
+static inline int valid_io_request(struct zram *zram,
+		sector_t start, unsigned int size)
+{
+	u64 end, bound;
+
+	/* unaligned request */
+	if (unlikely(start & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
+		return 0;
+	if (unlikely(size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
+		return 0;
+
+	end = start + (size >> SECTOR_SHIFT);
+	bound = zram->disksize >> SECTOR_SHIFT;
+	/* out of range range */
+	if (unlikely(start >= bound || end > bound || start > end))
+		return 0;
+
+	/* I/O request is valid */
+	return 1;
+}
+
+static void update_position(u32 *index, int *offset, struct bio_vec *bvec)
+{
+	if (*offset + bvec->bv_len >= PAGE_SIZE)
+		(*index)++;
+	*offset = (*offset + bvec->bv_len) % PAGE_SIZE;
+}
+
+static inline void update_used_max(struct zram *zram,
+					const unsigned long pages)
+{
+	unsigned long old_max, cur_max;
+
+	old_max = atomic_long_read(&zram->stats.max_used_pages);
+
+	do {
+		cur_max = old_max;
+		if (pages > cur_max)
+			old_max = atomic_long_cmpxchg(
+				&zram->stats.max_used_pages, cur_max, pages);
+	} while (old_max != cur_max);
+}
+
+static int page_zero_filled(void *ptr)
+{
+	unsigned int pos;
+	unsigned long *page;
+
+	page = (unsigned long *)ptr;
+
+	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
+		if (page[pos])
+			return 0;
+	}
+
+	return 1;
+}
+
+static void handle_zero_page(struct bio_vec *bvec)
+{
+	struct page *page = bvec->bv_page;
+	void *user_mem;
+
+	user_mem = kmap_atomic(page);
+	if (is_partial_io(bvec))
+		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
+	else
+		clear_page(user_mem);
+	kunmap_atomic(user_mem);
+
+	flush_dcache_page(page);
+}
+
 static ssize_t disksize_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -261,65 +374,95 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	return len;
 }
 
-/* flag operations needs meta->tb_lock */
-static int zram_test_flag(struct zram_meta *meta, u32 index,
-			enum zram_pageflags flag)
+static ssize_t compact_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
 {
-	return meta->table[index].value & BIT(flag);
-}
+	unsigned long nr_migrated;
+	struct zram *zram = dev_to_zram(dev);
+	struct zram_meta *meta;
 
-static void zram_set_flag(struct zram_meta *meta, u32 index,
-			enum zram_pageflags flag)
-{
-	meta->table[index].value |= BIT(flag);
-}
+	down_read(&zram->init_lock);
+	if (!init_done(zram)) {
+		up_read(&zram->init_lock);
+		return -EINVAL;
+	}
 
-static void zram_clear_flag(struct zram_meta *meta, u32 index,
-			enum zram_pageflags flag)
-{
-	meta->table[index].value &= ~BIT(flag);
+	meta = zram->meta;
+	nr_migrated = zs_compact(meta->mem_pool);
+	atomic64_add(nr_migrated, &zram->stats.num_migrated);
+	up_read(&zram->init_lock);
+
+	return len;
 }
 
-static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
+static ssize_t io_stat_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
 {
-	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
+	struct zram *zram = dev_to_zram(dev);
+	ssize_t ret;
+
+	down_read(&zram->init_lock);
+	ret = scnprintf(buf, PAGE_SIZE,
+			"%8llu %8llu %8llu %8llu\n",
+			(u64)atomic64_read(&zram->stats.failed_reads),
+			(u64)atomic64_read(&zram->stats.failed_writes),
+			(u64)atomic64_read(&zram->stats.invalid_io),
+			(u64)atomic64_read(&zram->stats.notify_free));
+	up_read(&zram->init_lock);
+
+	return ret;
 }
 
-static void zram_set_obj_size(struct zram_meta *meta,
-					u32 index, size_t size)
+static ssize_t mm_stat_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
 {
-	unsigned long flags = meta->table[index].value >> ZRAM_FLAG_SHIFT;
+	struct zram *zram = dev_to_zram(dev);
+	u64 orig_size, mem_used = 0;
+	long max_used;
+	ssize_t ret;
 
-	meta->table[index].value = (flags << ZRAM_FLAG_SHIFT) | size;
+	down_read(&zram->init_lock);
+	if (init_done(zram))
+		mem_used = zs_get_total_pages(zram->meta->mem_pool);
+
+	orig_size = atomic64_read(&zram->stats.pages_stored);
+	max_used = atomic_long_read(&zram->stats.max_used_pages);
+
+	ret = scnprintf(buf, PAGE_SIZE,
+			"%8llu %8llu %8llu %8lu %8ld %8llu %8llu\n",
+			orig_size << PAGE_SHIFT,
+			(u64)atomic64_read(&zram->stats.compr_data_size),
+			mem_used << PAGE_SHIFT,
+			zram->limit_pages << PAGE_SHIFT,
+			max_used << PAGE_SHIFT,
+			(u64)atomic64_read(&zram->stats.zero_pages),
+			(u64)atomic64_read(&zram->stats.num_migrated));
+	up_read(&zram->init_lock);
+
+	return ret;
 }
 
-static inline int is_partial_io(struct bio_vec *bvec)
+static DEVICE_ATTR_RO(io_stat);
+static DEVICE_ATTR_RO(mm_stat);
+ZRAM_ATTR_RO(num_reads);
+ZRAM_ATTR_RO(num_writes);
+ZRAM_ATTR_RO(failed_reads);
+ZRAM_ATTR_RO(failed_writes);
+ZRAM_ATTR_RO(invalid_io);
+ZRAM_ATTR_RO(notify_free);
+ZRAM_ATTR_RO(zero_pages);
+ZRAM_ATTR_RO(compr_data_size);
+
+static inline bool zram_meta_get(struct zram *zram)
 {
-	return bvec->bv_len != PAGE_SIZE;
+	if (atomic_inc_not_zero(&zram->refcount))
+		return true;
+	return false;
 }
 
-/*
- * Check if request is within bounds and aligned on zram logical blocks.
- */
-static inline int valid_io_request(struct zram *zram,
-		sector_t start, unsigned int size)
+static inline void zram_meta_put(struct zram *zram)
 {
-	u64 end, bound;
-
-	/* unaligned request */
-	if (unlikely(start & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
-		return 0;
-	if (unlikely(size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
-		return 0;
-
-	end = start + (size >> SECTOR_SHIFT);
-	bound = zram->disksize >> SECTOR_SHIFT;
-	/* out of range range */
-	if (unlikely(start >= bound || end > bound || start > end))
-		return 0;
-
-	/* I/O request is valid */
-	return 1;
+	atomic_dec(&zram->refcount);
 }
 
 static void zram_meta_free(struct zram_meta *meta, u64 disksize)
@@ -373,65 +516,15 @@ out_error:
 	return NULL;
 }
 
-static inline bool zram_meta_get(struct zram *zram)
-{
-	if (atomic_inc_not_zero(&zram->refcount))
-		return true;
-	return false;
-}
-
-static inline void zram_meta_put(struct zram *zram)
-{
-	atomic_dec(&zram->refcount);
-}
-
-static void update_position(u32 *index, int *offset, struct bio_vec *bvec)
+/*
+ * To protect concurrent access to the same index entry,
+ * caller should hold this table index entry's bit_spinlock to
+ * indicate this index entry is accessing.
+ */
+static void zram_free_page(struct zram *zram, size_t index)
 {
-	if (*offset + bvec->bv_len >= PAGE_SIZE)
-		(*index)++;
-	*offset = (*offset + bvec->bv_len) % PAGE_SIZE;
-}
-
-static int page_zero_filled(void *ptr)
-{
-	unsigned int pos;
-	unsigned long *page;
-
-	page = (unsigned long *)ptr;
-
-	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
-		if (page[pos])
-			return 0;
-	}
-
-	return 1;
-}
-
-static void handle_zero_page(struct bio_vec *bvec)
-{
-	struct page *page = bvec->bv_page;
-	void *user_mem;
-
-	user_mem = kmap_atomic(page);
-	if (is_partial_io(bvec))
-		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
-	else
-		clear_page(user_mem);
-	kunmap_atomic(user_mem);
-
-	flush_dcache_page(page);
-}
-
-
-/*
- * To protect concurrent access to the same index entry,
- * caller should hold this table index entry's bit_spinlock to
- * indicate this index entry is accessing.
- */
-static void zram_free_page(struct zram *zram, size_t index)
-{
-	struct zram_meta *meta = zram->meta;
-	unsigned long handle = meta->table[index].handle;
+	struct zram_meta *meta = zram->meta;
+	unsigned long handle = meta->table[index].handle;
 
 	if (unlikely(!handle)) {
 		/*
@@ -455,6 +548,7 @@ static void zram_free_page(struct zram *zram, size_t index)
 	zram_set_obj_size(meta, index, 0);
 }
 
+
 static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 {
 	int ret = 0;
@@ -540,21 +634,6 @@ out_cleanup:
 	return ret;
 }
 
-static inline void update_used_max(struct zram *zram,
-					const unsigned long pages)
-{
-	unsigned long old_max, cur_max;
-
-	old_max = atomic_long_read(&zram->stats.max_used_pages);
-
-	do {
-		cur_max = old_max;
-		if (pages > cur_max)
-			old_max = atomic_long_cmpxchg(
-				&zram->stats.max_used_pages, cur_max, pages);
-	} while (old_max != cur_max);
-}
-
 static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			   int offset)
 {
@@ -682,35 +761,6 @@ out:
 	return ret;
 }
 
-static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
-			int offset, int rw)
-{
-	unsigned long start_time = jiffies;
-	int ret;
-
-	generic_start_io_acct(rw, bvec->bv_len >> SECTOR_SHIFT,
-			&zram->disk->part0);
-
-	if (rw == READ) {
-		atomic64_inc(&zram->stats.num_reads);
-		ret = zram_bvec_read(zram, bvec, index, offset);
-	} else {
-		atomic64_inc(&zram->stats.num_writes);
-		ret = zram_bvec_write(zram, bvec, index, offset);
-	}
-
-	generic_end_io_acct(rw, &zram->disk->part0, start_time);
-
-	if (unlikely(ret)) {
-		if (rw == READ)
-			atomic64_inc(&zram->stats.failed_reads);
-		else
-			atomic64_inc(&zram->stats.failed_writes);
-	}
-
-	return ret;
-}
-
 /*
  * zram_bio_discard - handler on discard request
  * @index: physical block index in PAGE_SIZE units
@@ -750,151 +800,35 @@ static void zram_bio_discard(struct zram *zram, u32 index,
 	}
 }
 
-static ssize_t disksize_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t len)
-{
-	u64 disksize;
-	struct zcomp *comp;
-	struct zram_meta *meta;
-	struct zram *zram = dev_to_zram(dev);
-	int err;
-
-	disksize = memparse(buf, NULL);
-	if (!disksize)
-		return -EINVAL;
-
-	disksize = PAGE_ALIGN(disksize);
-	meta = zram_meta_alloc(zram->disk->first_minor, disksize);
-	if (!meta)
-		return -ENOMEM;
-
-	comp = zcomp_create(zram->compressor, zram->max_comp_streams);
-	if (IS_ERR(comp)) {
-		pr_info("Cannot initialise %s compressing backend\n",
-				zram->compressor);
-		err = PTR_ERR(comp);
-		goto out_free_meta;
-	}
-
-	down_write(&zram->init_lock);
-	if (init_done(zram)) {
-		pr_info("Cannot change disksize for initialized device\n");
-		err = -EBUSY;
-		goto out_destroy_comp;
-	}
-
-	init_waitqueue_head(&zram->io_done);
-	atomic_set(&zram->refcount, 1);
-	zram->meta = meta;
-	zram->comp = comp;
-	zram->disksize = disksize;
-	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
-	up_write(&zram->init_lock);
-
-	/*
-	 * Revalidate disk out of the init_lock to avoid lockdep splat.
-	 * It's okay because disk's capacity is protected by init_lock
-	 * so that revalidate_disk always sees up-to-date capacity.
-	 */
-	revalidate_disk(zram->disk);
-
-	return len;
-
-out_destroy_comp:
-	up_write(&zram->init_lock);
-	zcomp_destroy(comp);
-out_free_meta:
-	zram_meta_free(meta, disksize);
-	return err;
-}
-
-/* internal device reset part -- cleanup allocated memory and
- * return back to initial state */
-static void zram_reset_device_internal(struct zram *zram)
+static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
+			int offset, int rw)
 {
-	struct zram_meta *meta;
-	struct zcomp *comp;
-	u64 disksize;
-
-	down_write(&zram->init_lock);
+	unsigned long start_time = jiffies;
+	int ret;
 
-	zram->limit_pages = 0;
+	generic_start_io_acct(rw, bvec->bv_len >> SECTOR_SHIFT,
+			&zram->disk->part0);
 
-	if (!init_done(zram)) {
-		up_write(&zram->init_lock);
-		return;
+	if (rw == READ) {
+		atomic64_inc(&zram->stats.num_reads);
+		ret = zram_bvec_read(zram, bvec, index, offset);
+	} else {
+		atomic64_inc(&zram->stats.num_writes);
+		ret = zram_bvec_write(zram, bvec, index, offset);
 	}
 
-	meta = zram->meta;
-	comp = zram->comp;
-	disksize = zram->disksize;
-	/*
-	 * Refcount will go down to 0 eventually and r/w handler
-	 * cannot handle further I/O so it will bail out by
-	 * check zram_meta_get.
-	 */
-	zram_meta_put(zram);
-	/*
-	 * We want to free zram_meta in process context to avoid
-	 * deadlock between reclaim path and any other locks.
-	 */
-	wait_event(zram->io_done, atomic_read(&zram->refcount) == 0);
-
-	/* Reset stats */
-	memset(&zram->stats, 0, sizeof(zram->stats));
-	zram->disksize = 0;
-	zram->max_comp_streams = 1;
-	set_capacity(zram->disk, 0);
-
-	up_write(&zram->init_lock);
-	/* I/O operation under all of CPU are done so let's free */
-	zram_meta_free(meta, disksize);
-	zcomp_destroy(comp);
-}
-
-static int zram_reset_device(struct zram *zram)
-{
-	int ret = 0;
-	struct block_device *bdev = bdget_disk(zram->disk, 0);
-
-	if (!bdev)
-		return -ENOMEM;
+	generic_end_io_acct(rw, &zram->disk->part0, start_time);
 
-	mutex_lock(&bdev->bd_mutex);
-	/* Do not reset an active device! */
-	if (bdev->bd_openers) {
-		ret = -EBUSY;
-		goto out;
+	if (unlikely(ret)) {
+		if (rw == READ)
+			atomic64_inc(&zram->stats.failed_reads);
+		else
+			atomic64_inc(&zram->stats.failed_writes);
 	}
 
-	/* Make sure all pending I/O is finished */
-	fsync_bdev(bdev);
-	zram_reset_device_internal(zram);
-out:
-	mutex_unlock(&bdev->bd_mutex);
-	bdput(bdev);
 	return ret;
 }
 
-static ssize_t reset_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t len)
-{
-	int ret;
-	unsigned short do_reset;
-	struct zram *zram;
-
-	zram = dev_to_zram(dev);
-	ret = kstrtou16(buf, 10, &do_reset);
-	if (ret)
-		return ret;
-
-	if (!do_reset)
-		return -EINVAL;
-
-	ret = zram_reset_device(zram);
-	return ret ? ret : len;
-}
-
 static void __zram_make_request(struct zram *zram, struct bio *bio)
 {
 	int offset, rw;
@@ -949,9 +883,7 @@ out:
 	bio_io_error(bio);
 }
 
-/*
- * Handler function for all zram I/O requests.
- */
+/* Handler function for all zram I/O requests */
 static void zram_make_request(struct request_queue *queue, struct bio *bio)
 {
 	struct zram *zram = queue->queuedata;
@@ -1031,101 +963,168 @@ out:
 	return err;
 }
 
-static const struct block_device_operations zram_devops = {
-	.swap_slot_free_notify = zram_slot_free_notify,
-	.rw_page = zram_rw_page,
-	.owner = THIS_MODULE
-};
+/* internal device reset part -- cleanup allocated memory and
+ * return back to initial state */
+static void zram_reset_device_internal(struct zram *zram)
+{
+	struct zram_meta *meta;
+	struct zcomp *comp;
+	u64 disksize;
 
-static DEVICE_ATTR_WO(compact);
-static DEVICE_ATTR_RW(disksize);
-static DEVICE_ATTR_RO(initstate);
-static DEVICE_ATTR_WO(reset);
-static DEVICE_ATTR_RO(orig_data_size);
-static DEVICE_ATTR_RO(mem_used_total);
-static DEVICE_ATTR_RW(mem_limit);
-static DEVICE_ATTR_RW(mem_used_max);
-static DEVICE_ATTR_RW(max_comp_streams);
-static DEVICE_ATTR_RW(comp_algorithm);
+	down_write(&zram->init_lock);
 
-static ssize_t io_stat_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+	zram->limit_pages = 0;
+
+	if (!init_done(zram)) {
+		up_write(&zram->init_lock);
+		return;
+	}
+
+	meta = zram->meta;
+	comp = zram->comp;
+	disksize = zram->disksize;
+	/*
+	 * Refcount will go down to 0 eventually and r/w handler
+	 * cannot handle further I/O so it will bail out by
+	 * check zram_meta_get.
+	 */
+	zram_meta_put(zram);
+	/*
+	 * We want to free zram_meta in process context to avoid
+	 * deadlock between reclaim path and any other locks.
+	 */
+	wait_event(zram->io_done, atomic_read(&zram->refcount) == 0);
+
+	/* Reset stats */
+	memset(&zram->stats, 0, sizeof(zram->stats));
+	zram->disksize = 0;
+	zram->max_comp_streams = 1;
+	set_capacity(zram->disk, 0);
+
+	up_write(&zram->init_lock);
+	/* I/O operation under all of CPU are done so let's free */
+	zram_meta_free(meta, disksize);
+	zcomp_destroy(comp);
+}
+
+static int zram_reset_device(struct zram *zram)
 {
-	struct zram *zram = dev_to_zram(dev);
-	ssize_t ret;
+	int ret = 0;
+	struct block_device *bdev = bdget_disk(zram->disk, 0);
 
-	down_read(&zram->init_lock);
-	ret = scnprintf(buf, PAGE_SIZE,
-			"%8llu %8llu %8llu %8llu\n",
-			(u64)atomic64_read(&zram->stats.failed_reads),
-			(u64)atomic64_read(&zram->stats.failed_writes),
-			(u64)atomic64_read(&zram->stats.invalid_io),
-			(u64)atomic64_read(&zram->stats.notify_free));
-	up_read(&zram->init_lock);
+	if (!bdev)
+		return -ENOMEM;
+
+	mutex_lock(&bdev->bd_mutex);
+	/* Do not reset an active device! */
+	if (bdev->bd_openers) {
+		ret = -EBUSY;
+		goto out;
+	}
 
+	/* Make sure all pending I/O is finished */
+	fsync_bdev(bdev);
+	zram_reset_device_internal(zram);
+out:
+	mutex_unlock(&bdev->bd_mutex);
+	bdput(bdev);
 	return ret;
 }
 
-static ssize_t mm_stat_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
+static ssize_t reset_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
 {
-	struct zram *zram = dev_to_zram(dev);
-	u64 orig_size, mem_used = 0;
-	long max_used;
-	ssize_t ret;
-
-	down_read(&zram->init_lock);
-	if (init_done(zram))
-		mem_used = zs_get_total_pages(zram->meta->mem_pool);
+	int ret;
+	unsigned short do_reset;
+	struct zram *zram;
 
-	orig_size = atomic64_read(&zram->stats.pages_stored);
-	max_used = atomic_long_read(&zram->stats.max_used_pages);
+	zram = dev_to_zram(dev);
+	ret = kstrtou16(buf, 10, &do_reset);
+	if (ret)
+		return ret;
 
-	ret = scnprintf(buf, PAGE_SIZE,
-			"%8llu %8llu %8llu %8lu %8ld %8llu %8llu\n",
-			orig_size << PAGE_SHIFT,
-			(u64)atomic64_read(&zram->stats.compr_data_size),
-			mem_used << PAGE_SHIFT,
-			zram->limit_pages << PAGE_SHIFT,
-			max_used << PAGE_SHIFT,
-			(u64)atomic64_read(&zram->stats.zero_pages),
-			(u64)atomic64_read(&zram->stats.num_migrated));
-	up_read(&zram->init_lock);
+	if (!do_reset)
+		return -EINVAL;
 
-	return ret;
+	ret = zram_reset_device(zram);
+	return ret ? ret : len;
 }
 
-static ssize_t compact_store(struct device *dev,
+static ssize_t disksize_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
-	unsigned long nr_migrated;
-	struct zram *zram = dev_to_zram(dev);
+	u64 disksize;
+	struct zcomp *comp;
 	struct zram_meta *meta;
+	struct zram *zram = dev_to_zram(dev);
+	int err;
 
-	down_read(&zram->init_lock);
-	if (!init_done(zram)) {
-		up_read(&zram->init_lock);
+	disksize = memparse(buf, NULL);
+	if (!disksize)
 		return -EINVAL;
+
+	disksize = PAGE_ALIGN(disksize);
+	meta = zram_meta_alloc(zram->disk->first_minor, disksize);
+	if (!meta)
+		return -ENOMEM;
+
+	comp = zcomp_create(zram->compressor, zram->max_comp_streams);
+	if (IS_ERR(comp)) {
+		pr_info("Cannot initialise %s compressing backend\n",
+				zram->compressor);
+		err = PTR_ERR(comp);
+		goto out_free_meta;
 	}
 
-	meta = zram->meta;
-	nr_migrated = zs_compact(meta->mem_pool);
-	atomic64_add(nr_migrated, &zram->stats.num_migrated);
-	up_read(&zram->init_lock);
+	down_write(&zram->init_lock);
+	if (init_done(zram)) {
+		pr_info("Cannot change disksize for initialized device\n");
+		err = -EBUSY;
+		goto out_destroy_comp;
+	}
+
+	init_waitqueue_head(&zram->io_done);
+	atomic_set(&zram->refcount, 1);
+	zram->meta = meta;
+	zram->comp = comp;
+	zram->disksize = disksize;
+	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
+	up_write(&zram->init_lock);
+
+	/*
+	 * Revalidate disk out of the init_lock to avoid lockdep splat.
+	 * It's okay because disk's capacity is protected by init_lock
+	 * so that revalidate_disk always sees up-to-date capacity.
+	 */
+	revalidate_disk(zram->disk);
 
 	return len;
+
+out_destroy_comp:
+	up_write(&zram->init_lock);
+	zcomp_destroy(comp);
+out_free_meta:
+	zram_meta_free(meta, disksize);
+	return err;
 }
 
-static DEVICE_ATTR_RO(io_stat);
-static DEVICE_ATTR_RO(mm_stat);
-ZRAM_ATTR_RO(num_reads);
-ZRAM_ATTR_RO(num_writes);
-ZRAM_ATTR_RO(failed_reads);
-ZRAM_ATTR_RO(failed_writes);
-ZRAM_ATTR_RO(invalid_io);
-ZRAM_ATTR_RO(notify_free);
-ZRAM_ATTR_RO(zero_pages);
-ZRAM_ATTR_RO(compr_data_size);
+/* per-device block device operations and sysfs attrs */
+static const struct block_device_operations zram_devops = {
+	.swap_slot_free_notify = zram_slot_free_notify,
+	.rw_page = zram_rw_page,
+	.owner = THIS_MODULE
+};
+
+static DEVICE_ATTR_WO(compact);
+static DEVICE_ATTR_RW(disksize);
+static DEVICE_ATTR_RO(initstate);
+static DEVICE_ATTR_WO(reset);
+static DEVICE_ATTR_RO(orig_data_size);
+static DEVICE_ATTR_RO(mem_used_total);
+static DEVICE_ATTR_RW(mem_limit);
+static DEVICE_ATTR_RW(mem_used_max);
+static DEVICE_ATTR_RW(max_comp_streams);
+static DEVICE_ATTR_RW(comp_algorithm);
 
 static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_disksize.attr,
-- 
2.4.0.rc2

[PATCHv2 06/10] zram: remove max_num_devices limitation

[Sergey Senozhatsky]

Limiting the number of zram devices to 32 (default max_num_devices value)
is confusing, let's drop it.  A user with 2TB or 4TB of RAM, for example,
can request as many devices as he can handle.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/blockdev/zram.txt | 8 +++++++-
 drivers/block/zram/zram_drv.c   | 8 +-------
 drivers/block/zram/zram_drv.h   | 6 ------
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index bef4998..2ccc741 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -19,7 +19,9 @@ Following shows a typical sequence of steps for using zram.
 1) Load Module:
 	modprobe zram num_devices=4
 	This creates 4 devices: /dev/zram{0,1,2,3}
-	(num_devices parameter is optional. Default: 1)
+
+num_devices parameter is optional and tells zram how many devices should be
+pre-created. Default: 1.
 
 2) Set max number of compression streams
 	Compression backend may use up to max_comp_streams compression streams,
@@ -128,6 +130,7 @@ mem_limit         RW    the maximum amount of memory ZRAM can use to store
 num_migrated      RO    the number of objects migrated migrated by compaction
 compact           WO    trigger memory compaction
 
+
 WARNING
 =======
 per-stat sysfs attributes are considered to be deprecated.
@@ -149,6 +152,7 @@ File /sys/block/zram<id>/stat
 Represents block layer statistics. Read Documentation/block/stat.txt for
 details.
 
+
 File /sys/block/zram<id>/io_stat
 
 The stat file represents device's I/O statistics not accounted by block
@@ -160,6 +164,7 @@ whitespace:
 	invalid_io
 	notify_free
 
+
 File /sys/block/zram<id>/mm_stat
 
 The stat file represents device's mm statistics. It consists of a single
@@ -172,6 +177,7 @@ line of text and contains the following stats separated by whitespace:
 	zero_pages
 	num_migrated
 
+
 8) Deactivate:
 	swapoff /dev/zram0
 	umount /dev/zram1
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e526fe1..c2d9b76 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1288,12 +1288,6 @@ static int __init zram_init(void)
 {
 	int ret, dev_id;
 
-	if (num_devices > max_num_devices) {
-		pr_warn("Invalid value for num_devices: %u\n",
-				num_devices);
-		return -EINVAL;
-	}
-
 	zram_major = register_blkdev(0, "zram");
 	if (zram_major <= 0) {
 		pr_warn("Unable to get major number\n");
@@ -1323,7 +1317,7 @@ module_init(zram_init);
 module_exit(zram_exit);
 
 module_param(num_devices, uint, 0);
-MODULE_PARM_DESC(num_devices, "Number of zram devices");
+MODULE_PARM_DESC(num_devices, "Number of pre-created zram devices");
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 570c598..042994e 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -20,12 +20,6 @@
 
 #include "zcomp.h"
 
-/*
- * Some arbitrary value. This is just to catch
- * invalid value for num_devices module parameter.
- */
-static const unsigned max_num_devices = 32;
-
 /*-- Configurable parameters */
 
 /*
-- 
2.4.0.rc2

[PATCHv2 07/10] zram: report every added and removed device

[Sergey Senozhatsky]

With dynamic device creation/removal (which will be introduced later in the
series) printing num_devices in zram_init() will not make a lot of sense, as
well as printing the number of destroyed devices in destroy_devices(). Print
per-device action (added/removed) in zram_add() and zram_remove() instead.

Example:

[ 3645.259652] zram: Added device: zram5
[ 3646.152074] zram: Added device: zram6
[ 3650.585012] zram: Removed device: zram5
[ 3655.845584] zram: Added device: zram8
[ 3660.975223] zram: Removed device: zram6

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index c2d9b76..71bedb0 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1239,6 +1239,8 @@ static int zram_add(int device_id)
 	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
 	zram->meta = NULL;
 	zram->max_comp_streams = 1;
+
+	pr_info("Added device: %s\n", zram->disk->disk_name);
 	return 0;
 
 out_free_disk:
@@ -1265,6 +1267,8 @@ static void zram_remove(struct zram *zram)
 	zram_reset_device_internal(zram);
 	idr_remove(&zram_index_idr, zram->disk->first_minor);
 	blk_cleanup_queue(zram->disk->queue);
+
+	pr_info("Removed device: %s\n", zram->disk->disk_name);
 	del_gendisk(zram->disk);
 	put_disk(zram->disk);
 	kfree(zram);
@@ -1281,7 +1285,6 @@ static void destroy_devices(void)
 	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
 	idr_destroy(&zram_index_idr);
 	unregister_blkdev(zram_major, "zram");
-	pr_info("Destroyed device(s)\n");
 }
 
 static int __init zram_init(void)
@@ -1300,7 +1303,6 @@ static int __init zram_init(void)
 			goto out_error;
 	}
 
-	pr_info("Created %u device(s)\n", num_devices);
 	return 0;
 
 out_error:
-- 
2.4.0.rc2

[PATCHv2 08/10] zram: trivial: correct flag operations comment

[Sergey Senozhatsky]

We don't have meta->tb_lock anymore and use meta table entry bit_spin_lock
instead. update corresponding comment.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 71bedb0..f26251a 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -74,7 +74,7 @@ static inline struct zram *dev_to_zram(struct device *dev)
 	return (struct zram *)dev_to_disk(dev)->private_data;
 }
 
-/* flag operations needs meta->tb_lock */
+/* flag operations require table entry bit_spin_lock() being held */
 static int zram_test_flag(struct zram_meta *meta, u32 index,
 			enum zram_pageflags flag)
 {
-- 
2.4.0.rc2

[PATCHv2 09/10] zram: return zram device_id value from zram_add()

[Sergey Senozhatsky]

zram_add requires valid device_id to be provided, that can be a bit
inconvenient.  Change zram_add() to return negative value upon new device
creation failure, and device_id (>= 0) value otherwise.

This prepares zram_add to perform automatic device_id assignment.  New
device_id will be returned back to user-space (so user can reference that
device as /dev/zramX).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index f26251a..2c2e7cc 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1154,6 +1154,10 @@ static struct attribute_group zram_disk_attr_group = {
 	.attrs = zram_disk_attrs,
 };
 
+/*
+ * Allocate and initialize new zram device. the function returns
+ * '>= 0' device_id upon success, and negative value otherwise.
+ */
 static int zram_add(int device_id)
 {
 	struct zram *zram;
@@ -1241,7 +1245,7 @@ static int zram_add(int device_id)
 	zram->max_comp_streams = 1;
 
 	pr_info("Added device: %s\n", zram->disk->disk_name);
-	return 0;
+	return device_id;
 
 out_free_disk:
 	del_gendisk(zram->disk);
@@ -1299,7 +1303,7 @@ static int __init zram_init(void)
 
 	for (dev_id = 0; dev_id < num_devices; dev_id++) {
 		ret = zram_add(dev_id);
-		if (ret != 0)
+		if (ret < 0)
 			goto out_error;
 	}
 
-- 
2.4.0.rc2

[PATCHv2 10/10] zram: add dynamic device add/remove functionality

[Sergey Senozhatsky]

We currently don't support on-demand device creation. The one and only way
to have N zram devices is to specify num_devices module parameter (default
value 1). That means that if, for some reason, at some point, user wants
to have N + 1 devies he/she must umount all the existing devices, unload
the module, load the module passing num_devices equals to N + 1. And do
this again, if needed.

This patch introduces zram control sysfs class, which has two sysfs
attrs:
- zram_add      -- add a new zram device
- zram_remove   -- remove a specific (device_id) zram device

zram_add sysfs attr is read-only and has only automatic device id assignment
mode (as requested by Minchan Kim). read operation performed on this attr
creates a new zram device and returns back its device_id or error status.

Usage example:
	# add a new specific zram device
	cat /sys/class/zram-control/zram_add
	2

	# remove a specific zram device
	echo 4 > /sys/class/zram-control/zram_remove

Returning zram_add() error code back to user (-ENOMEM in this case)

	cat /sys/class/zram-control/zram_add
	cat: /sys/class/zram-control/zram_add: Cannot allocate memory

NOTE, there might be users who already depend on the fact that at
least zram0 device gets always created by zram_init(). Preserve this
behavior.

[Andrew Morton <akpm@linux-foundation.org>: fix comment layout]
Reported-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/ABI/testing/sysfs-class-zram |  24 ++++++
 Documentation/blockdev/zram.txt            |  23 +++++-
 drivers/block/zram/zram_drv.c              | 124 ++++++++++++++++++++++++++++-
 3 files changed, 166 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-zram

diff --git a/Documentation/ABI/testing/sysfs-class-zram b/Documentation/ABI/testing/sysfs-class-zram
new file mode 100644
index 0000000..6f62ef5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-zram
@@ -0,0 +1,24 @@
+What:		/sys/class/zram-control/
+Date:		August 2015
+KernelVersion:	4.1
+Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+Description:
+		The zram-control/ class sub-directory belongs to zram
+		device class
+
+What:		/sys/class/zram-control/zram_add
+Date:		August 2015
+KernelVersion:	4.1
+Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+Description:
+		RO attribute. Read operation will cause zram to add a new
+		device and return its device id back to user (so one can
+		use /dev/zram<id>), or error code.
+
+What:		/sys/class/zram-control/zram_add
+Date:		August 2015
+KernelVersion:	4.1
+Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+Description:
+		Remove a specific /dev/zramX device, where X is a device_id
+		provided by user
diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 2ccc741..44b1a77 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -99,7 +99,24 @@ size of the disk when not in use so a huge zram is wasteful.
 	mkfs.ext4 /dev/zram1
 	mount /dev/zram1 /tmp
 
-7) Stats:
+7) Add/remove zram devices
+
+zram provides a control interface, which enables dynamic (on-demand) device
+addition and removal.
+
+In order to add a new /dev/zramX device, perform read operation on zram_add
+attribute. This will return either new device's device id (meaning that you
+can use /dev/zram<id>) or error code.
+
+Example:
+	cat /sys/class/zram-control/zram_add
+	1
+
+To remove the existing /dev/zramX device (where X is a device id)
+execute
+	echo X > /sys/class/zram-control/zram_remove
+
+8) Stats:
 Per-device statistics are exported as various nodes under /sys/block/zram<id>/
 
 A brief description of exported device attritbutes. For more details please
@@ -178,11 +195,11 @@ line of text and contains the following stats separated by whitespace:
 	num_migrated
 
 
-8) Deactivate:
+9) Deactivate:
 	swapoff /dev/zram0
 	umount /dev/zram1
 
-9) Reset:
+10) Reset:
 	Write any positive value to 'reset' sysfs node
 	echo 1 > /sys/block/zram0/reset
 	echo 1 > /sys/block/zram1/reset
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 2c2e7cc..848222a 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -33,10 +33,14 @@
 #include <linux/vmalloc.h>
 #include <linux/err.h>
 #include <linux/idr.h>
+#include <linux/sysfs.h>
 
 #include "zram_drv.h"
 
 static DEFINE_IDR(zram_index_idr);
+/* idr index must be protected */
+static DEFINE_MUTEX(zram_index_mutex);
+
 static int zram_major;
 static const char *default_compressor = "lzo";
 
@@ -1168,8 +1172,15 @@ static int zram_add(int device_id)
 	if (!zram)
 		return -ENOMEM;
 
-	ret = idr_alloc(&zram_index_idr, zram, device_id,
-			device_id + 1, GFP_KERNEL);
+	if (device_id < 0) {
+		/* generate new device_id */
+		ret = idr_alloc(&zram_index_idr, zram, 0, 0, GFP_KERNEL);
+		device_id = ret;
+	} else {
+		/* use provided device_id */
+		ret = idr_alloc(&zram_index_idr, zram, device_id,
+				device_id + 1, GFP_KERNEL);
+	}
 	if (ret < 0)
 		goto out_free_dev;
 
@@ -1278,6 +1289,105 @@ static void zram_remove(struct zram *zram)
 	kfree(zram);
 }
 
+/*
+ * Lookup if there is any device pointer that match the given device_id.
+ * return device pointer if so, or ERR_PTR() otherwise.
+ */
+static struct zram *zram_lookup(int dev_id)
+{
+	struct zram *zram;
+
+	zram = idr_find(&zram_index_idr, dev_id);
+	if (zram)
+		return zram;
+	return ERR_PTR(-ENODEV);
+}
+
+/* zram module control sysfs attributes */
+static ssize_t zram_add_show(struct class *class,
+			struct class_attribute *attr,
+			char *buf)
+{
+	int ret;
+
+	mutex_lock(&zram_index_mutex);
+	/* pick up available device_id */
+	ret = zram_add(-1);
+	mutex_unlock(&zram_index_mutex);
+
+	if (ret < 0)
+		return ret;
+	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+static ssize_t zram_remove_store(struct class *class,
+			struct class_attribute *attr,
+			const char *buf,
+			size_t count)
+{
+	struct zram *zram;
+	int ret, err, dev_id;
+
+	mutex_lock(&zram_index_mutex);
+
+	/* dev_id is gendisk->first_minor, which is `int' */
+	ret = kstrtoint(buf, 10, &dev_id);
+	if (ret || dev_id < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	zram = zram_lookup(dev_id);
+	if (IS_ERR(zram)) {
+		ret = PTR_ERR(zram);
+		goto out;
+	}
+
+	/*
+	 * First, make ->disksize device attr RO, closing
+	 * zram_remove() vs disksize_store() race window
+	 */
+	ret = sysfs_chmod_file(&disk_to_dev(zram->disk)->kobj,
+			&dev_attr_disksize.attr, S_IRUGO);
+	if (ret)
+		goto out;
+
+	ret = zram_reset_device(zram);
+	if (ret == 0) {
+		/* ->disksize is RO and there are no ->bd_openers */
+		zram_remove(zram);
+		goto out;
+	}
+
+	/*
+	 * If there are still device bd_openers, try to make ->disksize
+	 * RW again and return. even if we fail to make ->disksize RW,
+	 * user still has RW ->reset attr. so it's possible to destroy
+	 * that device.
+	 */
+	err = sysfs_chmod_file(&disk_to_dev(zram->disk)->kobj,
+			&dev_attr_disksize.attr,
+			S_IWUSR | S_IRUGO);
+	if (err)
+		ret = err;
+
+out:
+	mutex_unlock(&zram_index_mutex);
+	return ret ? ret : count;
+}
+
+static struct class_attribute zram_control_class_attrs[] = {
+	__ATTR_RO(zram_add),
+	__ATTR_WO(zram_remove),
+	__ATTR_NULL,
+};
+
+static struct class zram_control_class = {
+	.name		= "zram-control",
+	.owner		= THIS_MODULE,
+	.class_attrs	= zram_control_class_attrs,
+};
+
 static int zram_exit_cb(int id, void *ptr, void *data)
 {
 	zram_remove(ptr);
@@ -1286,6 +1396,7 @@ static int zram_exit_cb(int id, void *ptr, void *data)
 
 static void destroy_devices(void)
 {
+	class_unregister(&zram_control_class);
 	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
 	idr_destroy(&zram_index_idr);
 	unregister_blkdev(zram_major, "zram");
@@ -1295,14 +1406,23 @@ static int __init zram_init(void)
 {
 	int ret, dev_id;
 
+	ret = class_register(&zram_control_class);
+	if (ret) {
+		pr_warn("Unable to register zram-control class\n");
+		return ret;
+	}
+
 	zram_major = register_blkdev(0, "zram");
 	if (zram_major <= 0) {
 		pr_warn("Unable to get major number\n");
+		class_unregister(&zram_control_class);
 		return -EBUSY;
 	}
 
 	for (dev_id = 0; dev_id < num_devices; dev_id++) {
+		mutex_lock(&zram_index_mutex);
 		ret = zram_add(dev_id);
+		mutex_unlock(&zram_index_mutex);
 		if (ret < 0)
 			goto out_error;
 	}
-- 
2.4.0.rc2

Re: [PATCHv2 00/10] cleaned up on-demand device creation

[Minchan Kim]

Hello,

On Thu, Apr 16, 2015 at 08:55:46PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> resending on-demand device creation patch set. sadly, I managed to create a
> mess; so here is my take to clean it up, fold patches and, hopefully, see
> them in 4.1.

Thanks for handling this quickly. I acknowlege dynamic device management part
but I want to review your patchset carefully one more time because you changed
a lot although it's just refactoring. Really sorry for late review. It's totally
my bad. Probably, I will have time to review next week so I feel it's too late
to merge it into 4.1 but I think there is no urgency to merge it.

> 
> this mess will not happen again.
> 
> 
> Andrew picked up some of the commits lined up for 4.1, which required manual
> editing. sorry for that inconvenience.
> 
> 
> unfortunately, commit c72c6160d967ed26a0b136dbab337f821d233509
>   Author: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>   Date:   Wed Apr 15 16:15:55 2015 -0700
> 
>      zram: move compact_store() to sysfs functions area
> 
> 
> ended up to be different: from a cosmetic change it has transformed into
> a functional change.
> 
> I fix it in 0001-zram-enable-compaction-support-in-zram.patch.
> 

So Andrew, could you pick 0001 in this merge window? Without it, zram cannot
use compaction feature of zsmalloc so zsmalloc's compaciton feature will be
void.

> 
> the rest is functionally identical to what we had in linux-next and mmotm for
> quite some time: in linux-next since Wed Apr 8 09:44:43 2015 +1000
> (commit 273b0791dae2f0b).
> 
> it would be nice to see it in 4.1, if possible.
> 
> no functional change in zram_drv.c file, compared to zram_drv.c from
> linux-next-20150415 (yes, actually checked). just a couple of additional
> comment tweaks.
> 
> like:
> 
> -/* allocate and initialize new zram device. the function returns
> - * '>= 0' device_id upon success, and negative value otherwise. */
> +/*
> + * Allocate and initialize new zram device. the function returns
> + * '>= 0' device_id upon success, and negative value otherwise.
> + */
> 
> or
> 
>         /*
>          * First, make ->disksize device attr RO, closing
> -        * ZRAM_CTL_REMOVE vs disksize_store() race window
> +        * zram_remove() vs disksize_store() race window
>          */
> 
> 
> I also picked up the remaining part of Julia Lawall's
> <Julia.Lawall@lip6.fr> ("zram: fix error return code") commit.
> 
> Documentation is identical to linux-next-20150415 version.
> 
> 
> 8<-------
> 
> We currently don't support zram on-demand device creation.  The only way
> to have N zram devices is to specify num_devices module parameter (default
> value 1).  That means that if, for some reason, at some point, user wants
> to have N + 1 devies he/she must umount all the existing devices, unload
> the module, load the module passing num_devices equals to N + 1.  And do
> this again, if needed.
> 
> This patchset introduces zram-control sysfs class, which has two sysfs
> attrs:
> 
>  - zram_add     -- add a new zram device
>  - zram_remove  -- remove a specific (device_id) zram device
> 
>     Usage example:
>         # add a new specific zram device
>         cat /sys/class/zram-control/zram_add
>         1
> 
>         # remove a specific zram device
>         echo 4 > /sys/class/zram-control/zram_remove
> 
> The patchset also does some cleanups and huge code reorganization.
> 
> 
> -ss
> 
> 
> Sergey Senozhatsky (10):
>   zram: enable compaction support in zram
>   zram: cosmetic ZRAM_ATTR_RO code formatting tweak
>   zram: use idr instead of `zram_devices' array
>   zram: factor out device reset from reset_store()
>   zram: reorganize code layout
>   zram: remove max_num_devices limitation
>   zram: report every added and removed device
>   zram: trivial: correct flag operations comment
>   zram: return zram device_id value from zram_add()
>   zram: add dynamic device add/remove functionality
> 
>  Documentation/ABI/testing/sysfs-class-zram |  24 +
>  Documentation/blockdev/zram.txt            |  31 +-
>  drivers/block/zram/zram_drv.c              | 939 +++++++++++++++++------------
>  drivers/block/zram/zram_drv.h              |   6 -
>  4 files changed, 597 insertions(+), 403 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-zram
> 
> -- 
> 2.4.0.rc1.29.gecc46a1
> 

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 00/10] cleaned up on-demand device creation

[Sergey Senozhatsky]

On (04/17/15 08:23), Minchan Kim wrote:
> > resending on-demand device creation patch set. sadly, I managed to create a
> > mess; so here is my take to clean it up, fold patches and, hopefully, see
> > them in 4.1.
> 
> Thanks for handling this quickly. I acknowlege dynamic device management part
> but I want to review your patchset carefully one more time because you changed
> a lot although it's just refactoring. Really sorry for late review. It's totally
> my bad. Probably, I will have time to review next week so I feel it's too late
> to merge it into 4.1 but I think there is no urgency to merge it.

ok. 4.2 then... we ended up missing 4.1 merge window.


Andrew, please ignore 0002 - 0010. 0001 is important, please pick it up.

	-ss

Re: [PATCHv2 00/10] cleaned up on-demand device creation

[Sergey Senozhatsky]

On (04/17/15 08:23), Minchan Kim wrote:
> Probably, I will have time to review next week so I feel it's too late
> to merge it into 4.1 but I think there is no urgency to merge it.
> 

Minchan,

I will resubmit the whole patch set. it has things like

[..]
  What:           /sys/class/zram-control/zram_add
  Date:           August 2015
  KernelVersion:  4.1
[..]

will have to do it anyway. so no rush, take your time.

	-ss

Re: [PATCHv2 00/10] cleaned up on-demand device creation

[Sergey Senozhatsky]

On (04/16/15 20:55), Sergey Senozhatsky wrote:
> unfortunately, commit c72c6160d967ed26a0b136dbab337f821d233509
>   Author: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>   Date:   Wed Apr 15 16:15:55 2015 -0700
> 
>      zram: move compact_store() to sysfs functions area
>

Andrew, please ignore the whole series.


I will send a separate patch to address the compaction issue shortly.


	-ss

Re: [PATCHv2 00/10] cleaned up on-demand device creation

[Sergey Senozhatsky]

On (04/17/15 10:00), Sergey Senozhatsky wrote:
> Date: Fri, 17 Apr 2015 10:00:24 +0900
> From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> To: Andrew Morton <akpm@linux-foundation.org>, Minchan Kim
>  <minchan@kernel.org>
> Cc: Nitin Gupta <ngupta@vflare.org>, linux-kernel@vger.kernel.org, Sergey
>  Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Subject: Re: [PATCHv2 00/10] cleaned up on-demand device creation
> User-Agent: Mutt/1.5.23 (2014-03-12)
> 
> On (04/16/15 20:55), Sergey Senozhatsky wrote:
> > unfortunately, commit c72c6160d967ed26a0b136dbab337f821d233509
> >   Author: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >   Date:   Wed Apr 15 16:15:55 2015 -0700
> > 
> >      zram: move compact_store() to sysfs functions area
> >
> 
> Andrew, please ignore the whole series.
> 
> 
> I will send a separate patch to address the compaction issue shortly.
> 

or simply revert c72c6160d967ed26a0b136dbab337f821d233509, which is
probably even better. there is no sense in moving compact_store to
a sysfs handlers logical code block anyway. there is simply no such
block there yet (logical code reorganization happens in a separate patch,
which was not merged into 4.1).

so I think reverting it is the best option for now.

$ git revert c72c6160d967ed26a0b136dbab337f821d233509
[master 211073e] Revert "zram: move compact_store() to sysfs functions area"

	-ss

Re: [PATCHv2 02/10] zram: cosmetic ZRAM_ATTR_RO code formatting tweak

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:48PM +0900, Sergey Senozhatsky wrote:
> Fix a misplaced backslash.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 03/10] zram: use idr instead of `zram_devices' array

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:49PM +0900, Sergey Senozhatsky wrote:
> This patch makes some preparations for on-demand device add/remove
> functionality.
> 
> Remove `zram_devices' array and switch to id-to-pointer translation (idr).
> idr doesn't bloat zram struct with additional members, f.e. list_head,
> yet still provides ability to match the device_id with the device pointer.
> 
> No user-space visible changes.
> 
> [do not lose -ENOMEM return code when `queue' allocation has failed]
> Reported-by: Julia Lawall <Julia.Lawall@lip6.fr>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 86 ++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 14043b2..4511830 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -32,12 +32,12 @@
>  #include <linux/string.h>
>  #include <linux/vmalloc.h>
>  #include <linux/err.h>
> +#include <linux/idr.h>
>  
>  #include "zram_drv.h"
>  
> -/* Globals */
> +static DEFINE_IDR(zram_index_idr);
>  static int zram_major;
> -static struct zram *zram_devices;
>  static const char *default_compressor = "lzo";
>  
>  /* Module params (documentation at end) */
> @@ -1156,10 +1156,20 @@ static struct attribute_group zram_disk_attr_group = {
>  	.attrs = zram_disk_attrs,
>  };
>  
> -static int create_device(struct zram *zram, int device_id)
> +static int zram_add(int device_id)
>  {
> +	struct zram *zram;
>  	struct request_queue *queue;
> -	int ret = -ENOMEM;
> +	int ret;
> +
> +	zram = kzalloc(sizeof(struct zram), GFP_KERNEL);
> +	if (!zram)
> +		return -ENOMEM;
> +
> +	ret = idr_alloc(&zram_index_idr, zram, device_id,
> +			device_id + 1, GFP_KERNEL);
> +	if (ret < 0)
> +		goto out_free_dev;
>  
>  	init_rwsem(&zram->init_lock);
>  
> @@ -1167,12 +1177,13 @@ static int create_device(struct zram *zram, int device_id)
>  	if (!queue) {
>  		pr_err("Error allocating disk queue for device %d\n",
>  			device_id);
> -		goto out;
> +		ret = -ENOMEM;
> +		goto out_free_idr;
>  	}
>  
>  	blk_queue_make_request(queue, zram_make_request);
>  
> -	 /* gendisk structure */
> +	/* gendisk structure */
>  	zram->disk = alloc_disk(1);
>  	if (!zram->disk) {
>  		pr_warn("Error allocating disk structure for device %d\n",
> @@ -1237,34 +1248,42 @@ out_free_disk:
>  	put_disk(zram->disk);
>  out_free_queue:
>  	blk_cleanup_queue(queue);
> -out:
> +out_free_idr:
> +	idr_remove(&zram_index_idr, device_id);
> +out_free_dev:
> +	kfree(zram);
>  	return ret;
>  }
>  
> -static void destroy_devices(unsigned int nr)
> +static void zram_remove(struct zram *zram)
>  {
> -	struct zram *zram;
> -	unsigned int i;
> -
> -	for (i = 0; i < nr; i++) {
> -		zram = &zram_devices[i];
> -		/*
> -		 * Remove sysfs first, so no one will perform a disksize
> -		 * store while we destroy the devices
> -		 */
> -		sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
> -				&zram_disk_attr_group);
> +	/*
> +	 * Remove sysfs first, so no one will perform a disksize
> +	 * store while we destroy the devices
> +	 */
> +	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
> +			&zram_disk_attr_group);
>  
> -		zram_reset_device(zram);
> +	zram_reset_device(zram);
> +	idr_remove(&zram_index_idr, zram->disk->first_minor);
> +	blk_cleanup_queue(zram->disk->queue);
> +	del_gendisk(zram->disk);
> +	put_disk(zram->disk);
> +	kfree(zram);
> +}
>  
> -		blk_cleanup_queue(zram->disk->queue);
> -		del_gendisk(zram->disk);
> -		put_disk(zram->disk);
> -	}
> +static int zram_exit_cb(int id, void *ptr, void *data)

trivial: I prefer remove to exit.

> +{
> +	zram_remove(ptr);
> +	return 0;
> +}
>  
> -	kfree(zram_devices);
> +static void destroy_devices(void)
> +{
> +	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
> +	idr_destroy(&zram_index_idr);
>  	unregister_blkdev(zram_major, "zram");
> -	pr_info("Destroyed %u device(s)\n", nr);
> +	pr_info("Destroyed device(s)\n");
>  }
>  
>  static int __init zram_init(void)
> @@ -1283,16 +1302,9 @@ static int __init zram_init(void)
>  		return -EBUSY;
>  	}
>  
> -	/* Allocate the device array and initialize each one */
> -	zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
> -	if (!zram_devices) {
> -		unregister_blkdev(zram_major, "zram");
> -		return -ENOMEM;
> -	}
> -
>  	for (dev_id = 0; dev_id < num_devices; dev_id++) {
> -		ret = create_device(&zram_devices[dev_id], dev_id);
> -		if (ret)
> +		ret = zram_add(dev_id);
> +		if (ret != 0)

It's better to check ret < 0 rather than ret != 0.

Otherwise,
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 04/10] zram: factor out device reset from reset_store()

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:50PM +0900, Sergey Senozhatsky wrote:
> Device reset currently includes two steps:
> a) holding ->bd_mutex we ensure that there are no device users
> (bdev->bd_openers)
> 
> b) and "internal" part (executed under bdev->bd_mutex and partially
> under zram->init_lock) that resets the device - frees allocated
> memory and returns the device back to its initial state.
> 
> Up until now it worked just fine. But there will be yet another device
> reset user soon -- on-demand device removal. We currently can reuse (b),
> but step (a) is done in sysfs ATTR reset_store() handler, which makes it
> hard to use it in on-demand device reset path.
> 
> Rename step (b) from zram_reset_device() to zram_reset_device_internal()

Usually, we use double underbar to show interal function so I prefer
__zram_reset_device to xxx_internal.

> and factor out step (a) to zram_reset_device().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 05/10] zram: reorganize code layout

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:51PM +0900, Sergey Senozhatsky wrote:
> This patch looks big, but basically it just moves code blocks.
> No functional changes.
> 
> Our current code layout looks like a sandwitch.
> 
> For example,
> a) between read/write handlers, we have update_used_max() helper function:
> 
> static int zram_decompress_page
> static int zram_bvec_read
> static inline void update_used_max
> static int zram_bvec_write
> static int zram_bvec_rw
> 
> b) RW request handlers __zram_make_request/zram_bio_discard are divided by
> sysfs attr reset_store() function and corresponding zram_reset_device()
> handler:
> 
> static void zram_bio_discard
> static void zram_reset_device
> static ssize_t disksize_store
> static ssize_t reset_store
> static void __zram_make_request
> 
> c) we first a bunch of sysfs read/store functions. then a number of
> one-liners, then helper functions, RW functions, sysfs functions, helper
> functions again, and so on.
> 
> Reorganize layout to be more logically grouped (a brief description,
> `cat zram_drv.c | grep static` gives a bigger picture):
> 
> -- one-liners: zram_test_flag/etc.
> 
> -- helpers: is_partial_io/update_position/etc
> 
> -- sysfs attr show/store functions + ZRAM_ATTR_RO() generated stats
> show() functions
> exception: reset and disksize store functions are required to be after meta()
> functions. because we do device create/destroy actions in these sysfs
> handlers.
> 
> -- "mm" functions: meta get/put, meta alloc/free, page free
> static inline bool zram_meta_get
> static inline void zram_meta_put
> static void zram_meta_free
> static struct zram_meta *zram_meta_alloc
> static void zram_free_page
> 
> -- a block of I/O functions
> static int zram_decompress_page
> static int zram_bvec_read
> static int zram_bvec_write
> static void zram_bio_discard
> static int zram_bvec_rw
> static void __zram_make_request
> static void zram_make_request
> static void zram_slot_free_notify
> static int zram_rw_page
> 
> -- device contol: add/remove/init/reset functions (+zram-control class
> will sit here)
> static void zram_reset_device_internal
> static int zram_reset_device
> static ssize_t reset_store
> static ssize_t disksize_store
> static int zram_add
> static void zram_remove
> static int __init zram_init
> static void __exit zram_exit
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

Thanks for the clean up!

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 06/10] zram: remove max_num_devices limitation

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:52PM +0900, Sergey Senozhatsky wrote:
> Limiting the number of zram devices to 32 (default max_num_devices value)
> is confusing, let's drop it.  A user with 2TB or 4TB of RAM, for example,
> can request as many devices as he can handle.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

I don't know you intentionally add new empty lines to make it clear
below but it's not related to this patch but I'm okay although you
did it to make to look better. Othewise, please fix when you respin.

> ---
>  Documentation/blockdev/zram.txt | 8 +++++++-
>  drivers/block/zram/zram_drv.c   | 8 +-------
>  drivers/block/zram/zram_drv.h   | 6 ------
>  3 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index bef4998..2ccc741 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -19,7 +19,9 @@ Following shows a typical sequence of steps for using zram.
>  1) Load Module:
>  	modprobe zram num_devices=4
>  	This creates 4 devices: /dev/zram{0,1,2,3}
> -	(num_devices parameter is optional. Default: 1)
> +
> +num_devices parameter is optional and tells zram how many devices should be
> +pre-created. Default: 1.
>  
>  2) Set max number of compression streams
>  	Compression backend may use up to max_comp_streams compression streams,
> @@ -128,6 +130,7 @@ mem_limit         RW    the maximum amount of memory ZRAM can use to store
>  num_migrated      RO    the number of objects migrated migrated by compaction
>  compact           WO    trigger memory compaction
>  
> +

?

>  WARNING
>  =======
>  per-stat sysfs attributes are considered to be deprecated.
> @@ -149,6 +152,7 @@ File /sys/block/zram<id>/stat
>  Represents block layer statistics. Read Documentation/block/stat.txt for
>  details.
>  
> +

?

>  File /sys/block/zram<id>/io_stat
>  
>  The stat file represents device's I/O statistics not accounted by block
> @@ -160,6 +164,7 @@ whitespace:
>  	invalid_io
>  	notify_free
>  
> +

?

>  File /sys/block/zram<id>/mm_stat
>  
>  The stat file represents device's mm statistics. It consists of a single
> @@ -172,6 +177,7 @@ line of text and contains the following stats separated by whitespace:
>  	zero_pages
>  	num_migrated
>  
> +

?

>  8) Deactivate:
>  	swapoff /dev/zram0
>  	umount /dev/zram1
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index e526fe1..c2d9b76 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1288,12 +1288,6 @@ static int __init zram_init(void)
>  {
>  	int ret, dev_id;
>  
> -	if (num_devices > max_num_devices) {
> -		pr_warn("Invalid value for num_devices: %u\n",
> -				num_devices);
> -		return -EINVAL;
> -	}
> -
>  	zram_major = register_blkdev(0, "zram");
>  	if (zram_major <= 0) {
>  		pr_warn("Unable to get major number\n");
> @@ -1323,7 +1317,7 @@ module_init(zram_init);
>  module_exit(zram_exit);
>  
>  module_param(num_devices, uint, 0);
> -MODULE_PARM_DESC(num_devices, "Number of zram devices");
> +MODULE_PARM_DESC(num_devices, "Number of pre-created zram devices");
>  
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 570c598..042994e 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -20,12 +20,6 @@
>  
>  #include "zcomp.h"
>  
> -/*
> - * Some arbitrary value. This is just to catch
> - * invalid value for num_devices module parameter.
> - */
> -static const unsigned max_num_devices = 32;
> -
>  /*-- Configurable parameters */
>  
>  /*
> -- 
> 2.4.0.rc2
> 

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 07/10] zram: report every added and removed device

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:53PM +0900, Sergey Senozhatsky wrote:
> With dynamic device creation/removal (which will be introduced later in the
> series) printing num_devices in zram_init() will not make a lot of sense, as
> well as printing the number of destroyed devices in destroy_devices(). Print
> per-device action (added/removed) in zram_add() and zram_remove() instead.
> 
> Example:
> 
> [ 3645.259652] zram: Added device: zram5
> [ 3646.152074] zram: Added device: zram6
> [ 3650.585012] zram: Removed device: zram5
> [ 3655.845584] zram: Added device: zram8
> [ 3660.975223] zram: Removed device: zram6
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index c2d9b76..71bedb0 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1239,6 +1239,8 @@ static int zram_add(int device_id)
>  	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
>  	zram->meta = NULL;
>  	zram->max_comp_streams = 1;
> +
> +	pr_info("Added device: %s\n", zram->disk->disk_name);
>  	return 0;
>  
>  out_free_disk:
> @@ -1265,6 +1267,8 @@ static void zram_remove(struct zram *zram)
>  	zram_reset_device_internal(zram);
>  	idr_remove(&zram_index_idr, zram->disk->first_minor);
>  	blk_cleanup_queue(zram->disk->queue);
> +
> +	pr_info("Removed device: %s\n", zram->disk->disk_name);

trivial:

why did you add the info in the middle of logic functions?
Without special reason, let's add it top of the zram_remove.

Acked-by: Minchan Kim <minchan@kernel.org>

>  	del_gendisk(zram->disk);
>  	put_disk(zram->disk);
>  	kfree(zram);
> @@ -1281,7 +1285,6 @@ static void destroy_devices(void)
>  	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
>  	idr_destroy(&zram_index_idr);
>  	unregister_blkdev(zram_major, "zram");
> -	pr_info("Destroyed device(s)\n");
>  }
>  
>  static int __init zram_init(void)
> @@ -1300,7 +1303,6 @@ static int __init zram_init(void)
>  			goto out_error;
>  	}
>  
> -	pr_info("Created %u device(s)\n", num_devices);
>  	return 0;
>  
>  out_error:
> -- 
> 2.4.0.rc2
> 

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 08/10] zram: trivial: correct flag operations comment

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:54PM +0900, Sergey Senozhatsky wrote:
> We don't have meta->tb_lock anymore and use meta table entry bit_spin_lock
> instead. update corresponding comment.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 09/10] zram: return zram device_id value from zram_add()

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:55PM +0900, Sergey Senozhatsky wrote:
> zram_add requires valid device_id to be provided, that can be a bit
> inconvenient.  Change zram_add() to return negative value upon new device
> creation failure, and device_id (>= 0) value otherwise.
> 
> This prepares zram_add to perform automatic device_id assignment.  New
> device_id will be returned back to user-space (so user can reference that
> device as /dev/zramX).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality

[Minchan Kim]

On Thu, Apr 16, 2015 at 08:55:56PM +0900, Sergey Senozhatsky wrote:
> We currently don't support on-demand device creation. The one and only way
> to have N zram devices is to specify num_devices module parameter (default
> value 1). That means that if, for some reason, at some point, user wants
> to have N + 1 devies he/she must umount all the existing devices, unload
> the module, load the module passing num_devices equals to N + 1. And do
> this again, if needed.
> 
> This patch introduces zram control sysfs class, which has two sysfs
> attrs:
> - zram_add      -- add a new zram device
> - zram_remove   -- remove a specific (device_id) zram device
> 
> zram_add sysfs attr is read-only and has only automatic device id assignment
> mode (as requested by Minchan Kim). read operation performed on this attr
> creates a new zram device and returns back its device_id or error status.
> 
> Usage example:
> 	# add a new specific zram device
> 	cat /sys/class/zram-control/zram_add
> 	2
> 
> 	# remove a specific zram device
> 	echo 4 > /sys/class/zram-control/zram_remove
> 
> Returning zram_add() error code back to user (-ENOMEM in this case)
> 
> 	cat /sys/class/zram-control/zram_add
> 	cat: /sys/class/zram-control/zram_add: Cannot allocate memory
> 
> NOTE, there might be users who already depend on the fact that at
> least zram0 device gets always created by zram_init(). Preserve this
> behavior.
> 
> [Andrew Morton <akpm@linux-foundation.org>: fix comment layout]
> Reported-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-class-zram |  24 ++++++
>  Documentation/blockdev/zram.txt            |  23 +++++-
>  drivers/block/zram/zram_drv.c              | 124 ++++++++++++++++++++++++++++-
>  3 files changed, 166 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-zram
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-zram b/Documentation/ABI/testing/sysfs-class-zram
> new file mode 100644
> index 0000000..6f62ef5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-zram
> @@ -0,0 +1,24 @@
> +What:		/sys/class/zram-control/
> +Date:		August 2015
> +KernelVersion:	4.1
> +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> +Description:
> +		The zram-control/ class sub-directory belongs to zram
> +		device class
> +
> +What:		/sys/class/zram-control/zram_add
> +Date:		August 2015
> +KernelVersion:	4.1
> +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> +Description:
> +		RO attribute. Read operation will cause zram to add a new
> +		device and return its device id back to user (so one can
> +		use /dev/zram<id>), or error code.
> +
> +What:		/sys/class/zram-control/zram_add
> +Date:		August 2015
> +KernelVersion:	4.1
> +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> +Description:
> +		Remove a specific /dev/zramX device, where X is a device_id
> +		provided by user
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index 2ccc741..44b1a77 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -99,7 +99,24 @@ size of the disk when not in use so a huge zram is wasteful.
>  	mkfs.ext4 /dev/zram1
>  	mount /dev/zram1 /tmp
>  
> -7) Stats:
> +7) Add/remove zram devices
> +
> +zram provides a control interface, which enables dynamic (on-demand) device
> +addition and removal.
> +
> +In order to add a new /dev/zramX device, perform read operation on zram_add
> +attribute. This will return either new device's device id (meaning that you
> +can use /dev/zram<id>) or error code.
> +
> +Example:
> +	cat /sys/class/zram-control/zram_add

Why do we put zram-contol there rather than /sys/block/zram
> +	1
> +
> +To remove the existing /dev/zramX device (where X is a device id)
> +execute
> +	echo X > /sys/class/zram-control/zram_remove
> +
> +8) Stats:
>  Per-device statistics are exported as various nodes under /sys/block/zram<id>/
>  
>  A brief description of exported device attritbutes. For more details please
> @@ -178,11 +195,11 @@ line of text and contains the following stats separated by whitespace:
>  	num_migrated
>  
>  
> -8) Deactivate:
> +9) Deactivate:
>  	swapoff /dev/zram0
>  	umount /dev/zram1
>  
> -9) Reset:
> +10) Reset:
>  	Write any positive value to 'reset' sysfs node
>  	echo 1 > /sys/block/zram0/reset
>  	echo 1 > /sys/block/zram1/reset
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 2c2e7cc..848222a 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -33,10 +33,14 @@
>  #include <linux/vmalloc.h>
>  #include <linux/err.h>
>  #include <linux/idr.h>
> +#include <linux/sysfs.h>
>  
>  #include "zram_drv.h"
>  
>  static DEFINE_IDR(zram_index_idr);
> +/* idr index must be protected */
> +static DEFINE_MUTEX(zram_index_mutex);
> +
>  static int zram_major;
>  static const char *default_compressor = "lzo";
>  
> @@ -1168,8 +1172,15 @@ static int zram_add(int device_id)

Why do zram_add need device_id?
We decided to remove option user pass device_id.

>  	if (!zram)
>  		return -ENOMEM;
>  
> -	ret = idr_alloc(&zram_index_idr, zram, device_id,
> -			device_id + 1, GFP_KERNEL);
> +	if (device_id < 0) {
> +		/* generate new device_id */
> +		ret = idr_alloc(&zram_index_idr, zram, 0, 0, GFP_KERNEL);
> +		device_id = ret;
> +	} else {
> +		/* use provided device_id */
> +		ret = idr_alloc(&zram_index_idr, zram, device_id,
> +				device_id + 1, GFP_KERNEL);
> +	}
>  	if (ret < 0)
>  		goto out_free_dev;
>  
> @@ -1278,6 +1289,105 @@ static void zram_remove(struct zram *zram)
>  	kfree(zram);
>  }
>  
> +/*
> + * Lookup if there is any device pointer that match the given device_id.
> + * return device pointer if so, or ERR_PTR() otherwise.
> + */
> +static struct zram *zram_lookup(int dev_id)
> +{
> +	struct zram *zram;
> +
> +	zram = idr_find(&zram_index_idr, dev_id);
> +	if (zram)
> +		return zram;
> +	return ERR_PTR(-ENODEV);

Just return NULL which is more simple and readable.

> +}
> +
> +/* zram module control sysfs attributes */
> +static ssize_t zram_add_show(struct class *class,
> +			struct class_attribute *attr,
> +			char *buf)
> +{
> +	int ret;
> +
> +	mutex_lock(&zram_index_mutex);
> +	/* pick up available device_id */
> +	ret = zram_add(-1);
> +	mutex_unlock(&zram_index_mutex);
> +
> +	if (ret < 0)
> +		return ret;
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
> +}
> +
> +static ssize_t zram_remove_store(struct class *class,
> +			struct class_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	struct zram *zram;
> +	int ret, err, dev_id;
> +
> +	mutex_lock(&zram_index_mutex);
> +
> +	/* dev_id is gendisk->first_minor, which is `int' */
> +	ret = kstrtoint(buf, 10, &dev_id);
> +	if (ret || dev_id < 0) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	zram = zram_lookup(dev_id);
> +	if (IS_ERR(zram)) {
> +		ret = PTR_ERR(zram);
> +		goto out;
> +	}
> +
> +	/*
> +	 * First, make ->disksize device attr RO, closing
> +	 * zram_remove() vs disksize_store() race window

Why don't you use zram->init_lock to protect the race?

> +	 */
> +	ret = sysfs_chmod_file(&disk_to_dev(zram->disk)->kobj,
> +			&dev_attr_disksize.attr, S_IRUGO);
> +	if (ret)
> +		goto out;
> +
> +	ret = zram_reset_device(zram);
> +	if (ret == 0) {
> +		/* ->disksize is RO and there are no ->bd_openers */
> +		zram_remove(zram);
> +		goto out;
> +	}
> +
> +	/*
> +	 * If there are still device bd_openers, try to make ->disksize
> +	 * RW again and return. even if we fail to make ->disksize RW,
> +	 * user still has RW ->reset attr. so it's possible to destroy
> +	 * that device.
> +	 */
> +	err = sysfs_chmod_file(&disk_to_dev(zram->disk)->kobj,
> +			&dev_attr_disksize.attr,
> +			S_IWUSR | S_IRUGO);
> +	if (err)
> +		ret = err;
> +
> +out:
> +	mutex_unlock(&zram_index_mutex);
> +	return ret ? ret : count;
> +}
> +
> +static struct class_attribute zram_control_class_attrs[] = {
> +	__ATTR_RO(zram_add),
> +	__ATTR_WO(zram_remove),
> +	__ATTR_NULL,
> +};
> +
> +static struct class zram_control_class = {
> +	.name		= "zram-control",
> +	.owner		= THIS_MODULE,
> +	.class_attrs	= zram_control_class_attrs,
> +};
> +
>  static int zram_exit_cb(int id, void *ptr, void *data)
>  {
>  	zram_remove(ptr);
> @@ -1286,6 +1396,7 @@ static int zram_exit_cb(int id, void *ptr, void *data)
>  
>  static void destroy_devices(void)
>  {
> +	class_unregister(&zram_control_class);
>  	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
>  	idr_destroy(&zram_index_idr);
>  	unregister_blkdev(zram_major, "zram");
> @@ -1295,14 +1406,23 @@ static int __init zram_init(void)
>  {
>  	int ret, dev_id;
>  
> +	ret = class_register(&zram_control_class);
> +	if (ret) {
> +		pr_warn("Unable to register zram-control class\n");
> +		return ret;
> +	}
> +
>  	zram_major = register_blkdev(0, "zram");
>  	if (zram_major <= 0) {
>  		pr_warn("Unable to get major number\n");
> +		class_unregister(&zram_control_class);
>  		return -EBUSY;
>  	}
>  
>  	for (dev_id = 0; dev_id < num_devices; dev_id++) {
> +		mutex_lock(&zram_index_mutex);
>  		ret = zram_add(dev_id);
> +		mutex_unlock(&zram_index_mutex);
>  		if (ret < 0)
>  			goto out_error;
>  	}
> -- 
> 2.4.0.rc2
> 

-- 
Kind regards,
Minchan Kim

Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality

[Minchan Kim]

On Thu, Apr 23, 2015 at 12:06 PM, Minchan Kim <minchan@kernel.org> wrote:
> On Thu, Apr 16, 2015 at 08:55:56PM +0900, Sergey Senozhatsky wrote:
>> We currently don't support on-demand device creation. The one and only way
>> to have N zram devices is to specify num_devices module parameter (default
>> value 1). That means that if, for some reason, at some point, user wants
>> to have N + 1 devies he/she must umount all the existing devices, unload
>> the module, load the module passing num_devices equals to N + 1. And do
>> this again, if needed.
>>
>> This patch introduces zram control sysfs class, which has two sysfs
>> attrs:
>> - zram_add      -- add a new zram device
>> - zram_remove   -- remove a specific (device_id) zram device
>>
>> zram_add sysfs attr is read-only and has only automatic device id assignment
>> mode (as requested by Minchan Kim). read operation performed on this attr
>> creates a new zram device and returns back its device_id or error status.
>>
>> Usage example:
>>       # add a new specific zram device
>>       cat /sys/class/zram-control/zram_add
>>       2
>>
>>       # remove a specific zram device
>>       echo 4 > /sys/class/zram-control/zram_remove
>>
>> Returning zram_add() error code back to user (-ENOMEM in this case)
>>
>>       cat /sys/class/zram-control/zram_add
>>       cat: /sys/class/zram-control/zram_add: Cannot allocate memory
>>
>> NOTE, there might be users who already depend on the fact that at
>> least zram0 device gets always created by zram_init(). Preserve this
>> behavior.
>>
>> [Andrew Morton <akpm@linux-foundation.org>: fix comment layout]
>> Reported-by: Minchan Kim <minchan@kernel.org>
>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> ---
>>  Documentation/ABI/testing/sysfs-class-zram |  24 ++++++
>>  Documentation/blockdev/zram.txt            |  23 +++++-
>>  drivers/block/zram/zram_drv.c              | 124 ++++++++++++++++++++++++++++-
>>  3 files changed, 166 insertions(+), 5 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-zram
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-zram b/Documentation/ABI/testing/sysfs-class-zram
>> new file mode 100644
>> index 0000000..6f62ef5
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-zram
>> @@ -0,0 +1,24 @@
>> +What:                /sys/class/zram-control/
>> +Date:                August 2015
>> +KernelVersion:       4.1
>> +Contact:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> +Description:
>> +             The zram-control/ class sub-directory belongs to zram
>> +             device class
>> +
>> +What:                /sys/class/zram-control/zram_add
>> +Date:                August 2015
>> +KernelVersion:       4.1
>> +Contact:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> +Description:
>> +             RO attribute. Read operation will cause zram to add a new
>> +             device and return its device id back to user (so one can
>> +             use /dev/zram<id>), or error code.
>> +
>> +What:                /sys/class/zram-control/zram_add
>> +Date:                August 2015
>> +KernelVersion:       4.1
>> +Contact:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> +Description:
>> +             Remove a specific /dev/zramX device, where X is a device_id
>> +             provided by user
>> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
>> index 2ccc741..44b1a77 100644
>> --- a/Documentation/blockdev/zram.txt
>> +++ b/Documentation/blockdev/zram.txt
>> @@ -99,7 +99,24 @@ size of the disk when not in use so a huge zram is wasteful.
>>       mkfs.ext4 /dev/zram1
>>       mount /dev/zram1 /tmp
>>
>> -7) Stats:
>> +7) Add/remove zram devices
>> +
>> +zram provides a control interface, which enables dynamic (on-demand) device
>> +addition and removal.
>> +
>> +In order to add a new /dev/zramX device, perform read operation on zram_add
>> +attribute. This will return either new device's device id (meaning that you
>> +can use /dev/zram<id>) or error code.
>> +
>> +Example:
>> +     cat /sys/class/zram-control/zram_add
>
> Why do we put zram-contol there rather than /sys/block/zram

Forget this. I tried to remove the comment but sent by mistake.

Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality

[Sergey Senozhatsky]

On (04/23/15 12:06), Minchan Kim wrote:
> > +Example:
> > +	cat /sys/class/zram-control/zram_add
> 
> Why do we put zram-contol there rather than /sys/block/zram

that's what clsss_register() does.

[..]

> > @@ -1168,8 +1172,15 @@ static int zram_add(int device_id)
> 
> Why do zram_add need device_id?
> We decided to remove option user pass device_id.

will cleanup. it was simpler at that time to support both devices
created by sysfs request and devices pre-crated for num_devices
module param.


> > +static struct zram *zram_lookup(int dev_id)
> > +{
> > +	struct zram *zram;
> > +
> > +	zram = idr_find(&zram_index_idr, dev_id);
> > +	if (zram)
> > +		return zram;
> > +	return ERR_PTR(-ENODEV);
> 
> Just return NULL which is more simple and readable.
> 

ok.


[..]
> > +	/*
> > +	 * First, make ->disksize device attr RO, closing
> > +	 * zram_remove() vs disksize_store() race window
> 
> Why don't you use zram->init_lock to protect the race?

zram_reset_device() takes this lock internally. but, it
unlocks the device upon the return from zram_reset_device():

lock idr_lock
 zram_reset_device()
  lock bd_mutex
    __zam_reset_device()
        lock init_lock
         reset
        unlock init_lock     ---\
  unlock bd_mutex                |
                                 |<----- disksize_store() race window
 zram_remove()               ---/
unlock idr_lock


until we call zram_remove() (which does sysfs_remove_group()) device has
sysfs attrs and, thus, disksize_store() can arrive in the middle. the
simplest things I came up with was that RO bit on sysfs disksize attrs.
I can factor out another set of __foo function to handle it differently,
not sure if this worth it.

I'll revisit it.

	-ss

Re: [PATCHv2 07/10] zram: report every added and removed device

[Sergey Senozhatsky]

On (04/23/15 11:38), Minchan Kim wrote:
> > +
> > +	pr_info("Removed device: %s\n", zram->disk->disk_name);
> 
> trivial:
> 
> why did you add the info in the middle of logic functions?
> Without special reason, let's add it top of the zram_remove.
> 

ok.

	-ss

> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> >  	del_gendisk(zram->disk);
> >  	put_disk(zram->disk);
> >  	kfree(zram);
> > @@ -1281,7 +1285,6 @@ static void destroy_devices(void)
> >  	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
> >  	idr_destroy(&zram_index_idr);
> >  	unregister_blkdev(zram_major, "zram");
> > -	pr_info("Destroyed device(s)\n");
> >  }
> >  
> >  static int __init zram_init(void)
> > @@ -1300,7 +1303,6 @@ static int __init zram_init(void)
> >  			goto out_error;
> >  	}
> >  
> > -	pr_info("Created %u device(s)\n", num_devices);
> >  	return 0;
> >  
> >  out_error:
> > -- 
> > 2.4.0.rc2
> > 
> 
> -- 
> Kind regards,
> Minchan Kim
> 

Re: [PATCHv2 06/10] zram: remove max_num_devices limitation

[Sergey Senozhatsky]

On (04/23/15 11:36), Minchan Kim wrote:
> Date: Thu, 23 Apr 2015 11:36:36 +0900
> From: Minchan Kim <minchan@kernel.org>
> To: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>, Nitin Gupta
>  <ngupta@vflare.org>, linux-kernel@vger.kernel.org, Sergey Senozhatsky
>  <sergey.senozhatsky.work@gmail.com>
> Subject: Re: [PATCHv2 06/10] zram: remove max_num_devices limitation
> User-Agent: Mutt/1.5.23 (2014-03-12)
> 
> On Thu, Apr 16, 2015 at 08:55:52PM +0900, Sergey Senozhatsky wrote:
> > Limiting the number of zram devices to 32 (default max_num_devices value)
> > is confusing, let's drop it.  A user with 2TB or 4TB of RAM, for example,
> > can request as many devices as he can handle.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> I don't know you intentionally add new empty lines to make it clear
> below but it's not related to this patch but I'm okay although you
> did it to make to look better. Othewise, please fix when you respin.
> 

no reason. was a bit in a hurry trying to redo the set for 4.1 merge window
and not to miss anything. will clean up.

	-ss

> > ---
> >  Documentation/blockdev/zram.txt | 8 +++++++-
> >  drivers/block/zram/zram_drv.c   | 8 +-------
> >  drivers/block/zram/zram_drv.h   | 6 ------
> >  3 files changed, 8 insertions(+), 14 deletions(-)
> > 
> > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> > index bef4998..2ccc741 100644
> > --- a/Documentation/blockdev/zram.txt
> > +++ b/Documentation/blockdev/zram.txt
> > @@ -19,7 +19,9 @@ Following shows a typical sequence of steps for using zram.
> >  1) Load Module:
> >  	modprobe zram num_devices=4
> >  	This creates 4 devices: /dev/zram{0,1,2,3}
> > -	(num_devices parameter is optional. Default: 1)
> > +
> > +num_devices parameter is optional and tells zram how many devices should be
> > +pre-created. Default: 1.
> >  
> >  2) Set max number of compression streams
> >  	Compression backend may use up to max_comp_streams compression streams,
> > @@ -128,6 +130,7 @@ mem_limit         RW    the maximum amount of memory ZRAM can use to store
> >  num_migrated      RO    the number of objects migrated migrated by compaction
> >  compact           WO    trigger memory compaction
> >  
> > +
> 
> ?
> 
> >  WARNING
> >  =======
> >  per-stat sysfs attributes are considered to be deprecated.
> > @@ -149,6 +152,7 @@ File /sys/block/zram<id>/stat
> >  Represents block layer statistics. Read Documentation/block/stat.txt for
> >  details.
> >  
> > +
> 
> ?
> 
> >  File /sys/block/zram<id>/io_stat
> >  
> >  The stat file represents device's I/O statistics not accounted by block
> > @@ -160,6 +164,7 @@ whitespace:
> >  	invalid_io
> >  	notify_free
> >  
> > +
> 
> ?
> 
> >  File /sys/block/zram<id>/mm_stat
> >  
> >  The stat file represents device's mm statistics. It consists of a single
> > @@ -172,6 +177,7 @@ line of text and contains the following stats separated by whitespace:
> >  	zero_pages
> >  	num_migrated
> >  
> > +
> 
> ?
> 
> >  8) Deactivate:
> >  	swapoff /dev/zram0
> >  	umount /dev/zram1
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index e526fe1..c2d9b76 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -1288,12 +1288,6 @@ static int __init zram_init(void)
> >  {
> >  	int ret, dev_id;
> >  
> > -	if (num_devices > max_num_devices) {
> > -		pr_warn("Invalid value for num_devices: %u\n",
> > -				num_devices);
> > -		return -EINVAL;
> > -	}
> > -
> >  	zram_major = register_blkdev(0, "zram");
> >  	if (zram_major <= 0) {
> >  		pr_warn("Unable to get major number\n");
> > @@ -1323,7 +1317,7 @@ module_init(zram_init);
> >  module_exit(zram_exit);
> >  
> >  module_param(num_devices, uint, 0);
> > -MODULE_PARM_DESC(num_devices, "Number of zram devices");
> > +MODULE_PARM_DESC(num_devices, "Number of pre-created zram devices");
> >  
> >  MODULE_LICENSE("Dual BSD/GPL");
> >  MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 570c598..042994e 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -20,12 +20,6 @@
> >  
> >  #include "zcomp.h"
> >  
> > -/*
> > - * Some arbitrary value. This is just to catch
> > - * invalid value for num_devices module parameter.
> > - */
> > -static const unsigned max_num_devices = 32;
> > -
> >  /*-- Configurable parameters */
> >  
> >  /*
> > -- 
> > 2.4.0.rc2
> > 
> 
> -- 
> Kind regards,
> Minchan Kim
> 

Re: [PATCHv2 04/10] zram: factor out device reset from reset_store()

[Sergey Senozhatsky]

On (04/23/15 11:29), Minchan Kim wrote:
> On Thu, Apr 16, 2015 at 08:55:50PM +0900, Sergey Senozhatsky wrote:
> > Device reset currently includes two steps:
> > a) holding ->bd_mutex we ensure that there are no device users
> > (bdev->bd_openers)
> > 
> > b) and "internal" part (executed under bdev->bd_mutex and partially
> > under zram->init_lock) that resets the device - frees allocated
> > memory and returns the device back to its initial state.
> > 
> > Up until now it worked just fine. But there will be yet another device
> > reset user soon -- on-demand device removal. We currently can reuse (b),
> > but step (a) is done in sysfs ATTR reset_store() handler, which makes it
> > hard to use it in on-demand device reset path.
> > 
> > Rename step (b) from zram_reset_device() to zram_reset_device_internal()
> 
> Usually, we use double underbar to show interal function so I prefer
> __zram_reset_device to xxx_internal.
> 


yes. I do remember your comment regarding the function name from the first
submission. it's already in the new patchset. thanks.

/* along with two additional comment styles clean ups. */

	-ss

> > and factor out step (a) to zram_reset_device().
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> -- 
> Kind regards,
> Minchan Kim
> 

Re: [PATCHv2 03/10] zram: use idr instead of `zram_devices' array

[Sergey Senozhatsky]

On (04/23/15 11:23), Minchan Kim wrote:
[..]

> > +static int zram_exit_cb(int id, void *ptr, void *data)
> 
> trivial: I prefer remove to exit.
> 

ok.

> > +{
> > +	zram_remove(ptr);
> > +	return 0;
> > +}
> >  
> > -	kfree(zram_devices);
> > +static void destroy_devices(void)
> > +{
> > +	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
> > +	idr_destroy(&zram_index_idr);
> >  	unregister_blkdev(zram_major, "zram");
> > -	pr_info("Destroyed %u device(s)\n", nr);
> > +	pr_info("Destroyed device(s)\n");
> >  }
> >  
> >  static int __init zram_init(void)
> > @@ -1283,16 +1302,9 @@ static int __init zram_init(void)
> >  		return -EBUSY;
> >  	}
> >  
> > -	/* Allocate the device array and initialize each one */
> > -	zram_devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
> > -	if (!zram_devices) {
> > -		unregister_blkdev(zram_major, "zram");
> > -		return -ENOMEM;
> > -	}
> > -
> >  	for (dev_id = 0; dev_id < num_devices; dev_id++) {
> > -		ret = create_device(&zram_devices[dev_id], dev_id);
> > -		if (ret)
> > +		ret = zram_add(dev_id);
> > +		if (ret != 0)
> 
> It's better to check ret < 0 rather than ret != 0.
> 

yes, it's

        for (dev_id = 0; dev_id < num_devices; dev_id++) {
                mutex_lock(&zram_index_mutex);
                ret = zram_add(dev_id);
                mutex_unlock(&zram_index_mutex);
                if (ret < 0)
                        goto out_error;
        }


at the end of the patch set.


will clean up everything and resubmit after the merge window, next week.


thanks!

	-ss

> Otherwise,
> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> -- 
> Kind regards,
> Minchan Kim
> 

Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality

[Minchan Kim]

On Thu, Apr 23, 2015 at 01:23:00PM +0900, Sergey Senozhatsky wrote:
> On (04/23/15 12:06), Minchan Kim wrote:
> > > +Example:
> > > +	cat /sys/class/zram-control/zram_add
> > 
> > Why do we put zram-contol there rather than /sys/block/zram
> 
> that's what clsss_register() does.
> 
> [..]
> 
> > > @@ -1168,8 +1172,15 @@ static int zram_add(int device_id)
> > 
> > Why do zram_add need device_id?
> > We decided to remove option user pass device_id.
> 
> will cleanup. it was simpler at that time to support both devices
> created by sysfs request and devices pre-crated for num_devices
> module param.
> 
> 
> > > +static struct zram *zram_lookup(int dev_id)
> > > +{
> > > +	struct zram *zram;
> > > +
> > > +	zram = idr_find(&zram_index_idr, dev_id);
> > > +	if (zram)
> > > +		return zram;
> > > +	return ERR_PTR(-ENODEV);
> > 
> > Just return NULL which is more simple and readable.
> > 
> 
> ok.
> 
> 
> [..]
> > > +	/*
> > > +	 * First, make ->disksize device attr RO, closing
> > > +	 * zram_remove() vs disksize_store() race window
> > 
> > Why don't you use zram->init_lock to protect the race?
> 
> zram_reset_device() takes this lock internally. but, it
> unlocks the device upon the return from zram_reset_device():
> 
> lock idr_lock
>  zram_reset_device()
>   lock bd_mutex
>     __zam_reset_device()
>         lock init_lock
>          reset
>         unlock init_lock     ---\
>   unlock bd_mutex                |
>                                  |<----- disksize_store() race window
>  zram_remove()               ---/
> unlock idr_lock
> 
> 
> until we call zram_remove() (which does sysfs_remove_group()) device has
> sysfs attrs and, thus, disksize_store() can arrive in the middle. the
> simplest things I came up with was that RO bit on sysfs disksize attrs.
> I can factor out another set of __foo function to handle it differently,
> not sure if this worth it.

I believe we should handle it with init_lock more elegantly rather than
RO trick. The init_lock was born to protect race between init and exit
although we didn't need it until now since we don't have dynamic device
feature. So, with refactoring, we should handle it with init_lock.

> 
> I'll revisit it.

Thanks. I believe you will find.

-- 
Kind regards,
Minchan Kim