[PATCH 0/8] introduce dynamic device creation/removal

[PATCH 0/8] introduce dynamic device creation/removal

[Sergey Senozhatsky]

Hello,

this patchset introduces dynamic (on demand) zram device add-remove
functionality via /dev/zram-control interface. Two ioctl commands are
defined as of now (accessible in user-space via new zram.h header file):
-- ZRAM_CTL_ADD
	add new device (generates device_id automatically or uses provided
	device_id)
-- ZRAM_CTL_REMOVE
	remove device (by device_id)

util-linux zramctl update will be done later, after we land this patchset.


This also opens a possibility to drop some of sysfs device attrs and FOO_show()
code duplication in the future, and provide device stats/info via ioctl call
instead, providing something like (via zram.h):

	struct zram_info {
		__u64 orig_data_size;
		__u64 mem_used_total;
		__u64 max_comp_streams;

		[..]
	};


fill it under ->init_lock in zram_fill_info() (or any other name) function and
return all device stats at once back to user-space in a single syscall.

This is a long term plan, of course, but I'd like to see sysfs functions go away
in a year or so. What do you think?


Sergey Senozhatsky (8):
  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: add dynamic device add/remove functionality
  zram: return zram device_id value from zram_add()
  zram: allow automatic new zram device_id assignment
  zram: remove max_num_devices limitation
  zram: report every added and removed device

 drivers/block/zram/zram_drv.c | 326 ++++++++++++++++++++++++++++--------------
 drivers/block/zram/zram_drv.h |   6 -
 include/linux/miscdevice.h    |   1 +
 include/uapi/linux/Kbuild     |   1 +
 include/uapi/linux/zram.h     |  17 +++
 5 files changed, 238 insertions(+), 113 deletions(-)
 create mode 100644 include/uapi/linux/zram.h

-- 
2.3.1.167.g7f4ba4b

[PATCH 1/8] zram: cosmetic ZRAM_ATTR_RO code formatting tweak

[Sergey Senozhatsky]

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 8e233ed..56c9e21 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -44,7 +44,7 @@ static const char *default_compressor = "lzo";
 static unsigned int num_devices = 1;
 
 #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.3.1.167.g7f4ba4b

[PATCH 2/8] zram: use idr instead of `zram_devices' array

[Sergey Senozhatsky]

This patch makes some preparations for dynamic device ADD/REMOVE functionality
via /dev/zram-control interface.

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.

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

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 56c9e21..d5e502b 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) */
@@ -1061,18 +1061,28 @@ 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;
 
+	zram = kzalloc(sizeof(struct zram), GFP_KERNEL);
+	if (!zram)
+		return ret;
+
+	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);
 
 	queue = blk_alloc_queue(GFP_KERNEL);
 	if (!queue) {
 		pr_err("Error allocating disk queue for device %d\n",
 			device_id);
-		goto out;
+		goto out_free_idr;
 	}
 
 	blk_queue_make_request(queue, zram_make_request);
@@ -1141,34 +1151,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)
@@ -1187,16 +1205,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;
 	}
 
@@ -1204,13 +1215,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.3.1.167.g7f4ba4b

[PATCH 3/8] zram: factor out device reset from reset_store()

[Sergey Senozhatsky]

Device reset currently consists of 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 (un-init) state.

Dynamic device removal requires the same amount of work and checks.
We can reuse internal cleanup part (b) zram_reset_device(), but step (a)
is done in sysfs ATTR reset_store() handler.

Rename step (b) from zram_reset_device() to zram_reset_device_internal()
and factor out step (a) to zram_reset_device(). Both reset_store() and
dynamic device removal will use zram_reset_device().

For readability let's keep them separated.

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 d5e502b..92087da 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -729,48 +729,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)
 {
@@ -829,16 +787,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;
@@ -850,31 +846,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;
@@ -1167,7 +1166,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.3.1.167.g7f4ba4b

[PATCH 4/8] zram: add dynamic device add/remove functionality

[Sergey Senozhatsky]

Introduce /dev/zram-control interface which lets user-space to add a new zram
device (ZRAM_CTL_ADD ioctl) or remove the existing one (ZRAM_CTL_REMOVE ioctl).

This patch adds only two ioctl operations: add device and remove device. There
is no support for `automatic' device_id generation in this patch (will come up
later), so correct device_id must be provided.

Corresponding user-space visible defines (ZRAM_CTL_ADD/ZRAM_CTL_REMOVE) are
located in a new header file: include/uapi/linux/zram.h. Well, we have `private'
zram_drv.h and `public' zram.h headers from now on. I don't really want to make
zram_drv.h public, that would require moving of all private defines and structs
to zram_drv.c (100+ lines of code) and zram_drv.c is already big and a bit
complicated. On the other hand, zram uses struct block_device_operations that
has a per-device ->ioctl callback (not used by zram as of now). Ideally, we can
drop some of the existing zram_stats sysfs device ATTRs and use ioctl
(f.e. ZRAM_CTL_INFO) to copy out zram->stats to user-space. That will require
zram_info structure being visible to user-space via zram.h, so this new header
file can be of use.

Interface usage example (pretty much like LOOP ctl interface):

	#include <linux/zram.h>

	cfd = open("/dev/zram-control", O_RDWR);

	/* add a new specific zram device */
	err = ioctl(cfd, ZRAM_CTL_ADD, devnr);

	/* remove a specific zram device */
	err = ioctl(cfd, ZRAM_CTL_REMOVE, devnr);

NOTE, zram provides /dev/zram-control alias and zram module will be loaded
automatically when /dev/zram-control is access the first time. However, there
might be users who already depend on the fact that at least zram0 device gets
always created by zram_init(). Thus, due to compatibility reasons, along with
requested device_id (f.e. ZRAM_CTL_ADD 5) zram0 will also be created.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 99 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/miscdevice.h    |  1 +
 include/uapi/linux/Kbuild     |  1 +
 include/uapi/linux/zram.h     | 17 ++++++++
 4 files changed, 117 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/linux/zram.h

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 92087da..4f87d1e 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -33,10 +33,15 @@
 #include <linux/vmalloc.h>
 #include <linux/err.h>
 #include <linux/idr.h>
+#include <linux/miscdevice.h>
+#include <linux/zram.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";
 
@@ -1060,6 +1065,19 @@ static struct attribute_group zram_disk_attr_group = {
 	.attrs = zram_disk_attrs,
 };
 
+/* lookup if there is any device pointer that match to 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);
+}
+
 static int zram_add(int device_id)
 {
 	struct zram *zram;
@@ -1174,6 +1192,76 @@ static void zram_remove(struct zram *zram)
 	kfree(zram);
 }
 
+static long zram_control_ioctl(struct file *file, unsigned int cmd,
+			       unsigned long parm)
+{
+	struct zram *zram;
+	int ret = -ENOSYS, err;
+
+	mutex_lock(&zram_index_mutex);
+	zram = zram_lookup(parm);
+
+	switch (cmd) {
+	case ZRAM_CTL_ADD:
+		if (!IS_ERR(zram)) {
+			ret = -EEXIST;
+			break;
+		}
+		ret = zram_add(parm);
+		break;
+	case ZRAM_CTL_REMOVE:
+		if (IS_ERR(zram)) {
+			ret = PTR_ERR(zram);
+			break;
+		}
+
+		/* First, make ->disksize device attr RO, closing
+		 * ZRAM_CTL_REMOVE vs disksize_store() race window */
+		ret = sysfs_chmod_file(&disk_to_dev(zram->disk)->kobj,
+				&dev_attr_disksize.attr, S_IRUGO);
+		if (ret)
+			break;
+
+		ret = zram_reset_device(zram);
+		if (ret == 0) {
+			/* ->disksize is RO and there are no ->bd_openers */
+			zram_remove(zram);
+			break;
+		}
+
+		/* 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;
+		break;
+	}
+	mutex_unlock(&zram_index_mutex);
+
+	return ret;
+}
+
+static const struct file_operations zram_ctl_fops = {
+	.open		= nonseekable_open,
+	.unlocked_ioctl	= zram_control_ioctl,
+	.compat_ioctl	= zram_control_ioctl,
+	.owner		= THIS_MODULE,
+	.llseek		= noop_llseek,
+};
+
+static struct miscdevice zram_misc = {
+	.minor		= ZRAM_CTRL_MINOR,
+	.name		= "zram-control",
+	.fops		= &zram_ctl_fops,
+};
+
+MODULE_ALIAS_MISCDEV(ZRAM_CTRL_MINOR);
+MODULE_ALIAS("devname:zram-control");
+
 static int zram_exit_cb(int id, void *ptr, void *data)
 {
 	zram_remove(ptr);
@@ -1182,6 +1270,7 @@ static int zram_exit_cb(int id, void *ptr, void *data)
 
 static void destroy_devices(void)
 {
+	misc_deregister(&zram_misc);
 	idr_for_each(&zram_index_idr, &zram_exit_cb, NULL);
 	idr_destroy(&zram_index_idr);
 	unregister_blkdev(zram_major, "zram");
@@ -1198,6 +1287,12 @@ static int __init zram_init(void)
 		return -EINVAL;
 	}
 
+	ret = misc_register(&zram_misc);
+	if (ret < 0) {
+		pr_warn("Unable to register zram-control\n");
+		return ret;
+	}
+
 	zram_major = register_blkdev(0, "zram");
 	if (zram_major <= 0) {
 		pr_warn("Unable to get major number\n");
@@ -1205,7 +1300,9 @@ static int __init zram_init(void)
 	}
 
 	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;
 	}
@@ -1227,7 +1324,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/include/linux/miscdevice.h b/include/linux/miscdevice.h
index ee80dd7..11a14e3 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -49,6 +49,7 @@
 #define LOOP_CTRL_MINOR		237
 #define VHOST_NET_MINOR		238
 #define UHID_MINOR		239
+#define ZRAM_CTRL_MINOR		240
 #define MISC_DYNAMIC_MINOR	255
 
 struct device;
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 68ceb97..9cabde9 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -448,3 +448,4 @@ header-y += xattr.h
 header-y += xfrm.h
 header-y += zorro.h
 header-y += zorro_ids.h
+header-y += zram.h
diff --git a/include/uapi/linux/zram.h b/include/uapi/linux/zram.h
new file mode 100644
index 0000000..7b84532
--- /dev/null
+++ b/include/uapi/linux/zram.h
@@ -0,0 +1,17 @@
+/*
+ * include/linux/zram.h
+ *
+ * Copyright (C) 2015 Sergey Senozhatsky.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _UAPI_LINUX_ZRAM_H
+#define _UAPI_LINUX_ZRAM_H
+
+/* /dev/zram-control interface */
+#define ZRAM_CTL_ADD		0x5C80
+#define ZRAM_CTL_REMOVE		0x5C81
+
+#endif /* _UAPI_LINUX_ZRAM_H */
-- 
2.3.1.167.g7f4ba4b

[PATCH 5/8] zram: return zram device_id value from zram_add()

[Sergey Senozhatsky]

ZRAM_CTL_ADD ioctl 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_CTL_ADD to perform automatic device_id assignment. New
device_id will be returned back to user-space (so user can reference that
device as zram%d device_id).

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 4f87d1e..0154c2d4 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1078,6 +1078,8 @@ static struct zram *zram_lookup(int dev_id)
 	return ERR_PTR(-ENODEV);
 }
 
+/* allocate and initialize new zram device. the function returns >= 0
+ * device_id value upon success, and negative value otherwise. */
 static int zram_add(int device_id)
 {
 	struct zram *zram;
@@ -1161,7 +1163,7 @@ static int zram_add(int device_id)
 	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
 	zram->meta = NULL;
 	zram->max_comp_streams = 1;
-	return 0;
+	return device_id;
 
 out_free_disk:
 	del_gendisk(zram->disk);
@@ -1303,7 +1305,7 @@ static int __init zram_init(void)
 		mutex_lock(&zram_index_mutex);
 		ret = zram_add(dev_id);
 		mutex_unlock(&zram_index_mutex);
-		if (ret != 0)
+		if (ret < 0)
 			goto out_error;
 	}
 
-- 
2.3.1.167.g7f4ba4b

[PATCH 6/8] zram: allow automatic new zram device_id assignment

[Sergey Senozhatsky]

If no particular device_id was requested (passed -1 to zram_add()) during new zram
device creation, generate one automatically and return it back. So, schematically,
device creation can be done as:

	dev_id = ioctl ZRAM_CTL_ADD -1
	# dev_id == 1 or error code
	init device zram1

	dev_id = ioctl ZRAM_CTL_ADD -1
	# dev_id == 2 or error code
	init device zram2
	...

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

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 0154c2d4..5804dda 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1090,8 +1090,15 @@ static int zram_add(int device_id)
 	if (!zram)
 		return ret;
 
-	ret = idr_alloc(&zram_index_idr, zram, device_id,
+	if (device_id < 0) {
+		/* generate new device_id */
+		ret = idr_alloc(&zram_index_idr, zram, 0, 0, GFP_KERNEL);
+		device_id = ret;
+	} else {
+		/* use specific device_id */
+		ret = idr_alloc(&zram_index_idr, zram, device_id,
 			device_id + 1, GFP_KERNEL);
+	}
 	if (ret < 0)
 		goto out_free_dev;
 
-- 
2.3.1.167.g7f4ba4b

[PATCH 7/8] 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>
---
 drivers/block/zram/zram_drv.c | 6 ------
 drivers/block/zram/zram_drv.h | 6 ------
 2 files changed, 12 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 5804dda..1436970 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1290,12 +1290,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;
-	}
-
 	ret = misc_register(&zram_misc);
 	if (ret < 0) {
 		pr_warn("Unable to register zram-control\n");
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 17056e5..60d14bc 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.3.1.167.g7f4ba4b

[PATCH 8/8] zram: report every added and removed device

[Sergey Senozhatsky]

With dynamic device creation/removal printing num_devices in zram_init()
doesn't 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 1436970..91537b1 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1170,6 +1170,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 device_id;
 
 out_free_disk:
@@ -1196,6 +1198,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);
@@ -1283,7 +1287,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)
@@ -1310,7 +1313,6 @@ static int __init zram_init(void)
 			goto out_error;
 	}
 
-	pr_info("Created %u device(s)\n", num_devices);
 	return 0;
 
 out_error:
-- 
2.3.1.167.g7f4ba4b

Re: [PATCH 0/8] introduce dynamic device creation/removal

[Andrew Morton]

On Thu, 26 Feb 2015 23:10:35 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> Hello,
> 
> this patchset introduces dynamic (on demand) zram device add-remove
> functionality via /dev/zram-control interface. Two ioctl commands are
> defined as of now (accessible in user-space via new zram.h header file):
> -- ZRAM_CTL_ADD
> 	add new device (generates device_id automatically or uses provided
> 	device_id)
> -- ZRAM_CTL_REMOVE
> 	remove device (by device_id)
> 
> util-linux zramctl update will be done later, after we land this patchset.
> 
> 
> This also opens a possibility to drop some of sysfs device attrs and FOO_show()
> code duplication in the future, and provide device stats/info via ioctl call
> instead, providing something like (via zram.h):
> 
> 	struct zram_info {
> 		__u64 orig_data_size;
> 		__u64 mem_used_total;
> 		__u64 max_comp_streams;
> 
> 		[..]
> 	};
> 
> 
> fill it under ->init_lock in zram_fill_info() (or any other name) function and
> return all device stats at once back to user-space in a single syscall.
> 
> This is a long term plan, of course, but I'd like to see sysfs functions go away
> in a year or so. What do you think?

hoo boy.  Creating a /dev node and doing ioctls on it is really old
school.  So old school that I've forgotten why we don't do it any more.

Hopefully Alan can recall the thinking?

Re: [PATCH 0/8] introduce dynamic device creation/removal

[Sergey Senozhatsky]

On (02/27/15 14:51), Andrew Morton wrote:
> hoo boy.  Creating a /dev node and doing ioctls on it is really old
> school.  So old school that I've forgotten why we don't do it any more.
> 
> Hopefully Alan can recall the thinking?

oh. I thought this is how loop control works, and ioctl there doesn't look
insane to me.

any quick hint how do you do this in a modern world so I'll redo the patch set?

	-ss

Re: [PATCH 0/8] introduce dynamic device creation/removal

[Sergey Senozhatsky]

On (02/28/15 10:33), Sergey Senozhatsky wrote:
> > hoo boy.  Creating a /dev node and doing ioctls on it is really old
> > school.  So old school that I've forgotten why we don't do it any more.
> > 
> > Hopefully Alan can recall the thinking?
> 
> oh. I thought this is how loop control works, and ioctl there doesn't look
> insane to me.
> 
> any quick hint how do you do this in a modern world so I'll redo the patch set?
> 

I'll change it to sysfs RW control node.

	-ss

Re: [PATCH 0/8] introduce dynamic device creation/removal

[Sergey Senozhatsky]

On (02/27/15 14:51), Andrew Morton wrote:
> hoo boy.  Creating a /dev node and doing ioctls on it is really old
> school.  So old school that I've forgotten why we don't do it any more.
> 
> Hopefully Alan can recall the thinking?
> 

perhaps, something like

 static struct class_attribute zram_class_attrs[] = {
         __ATTR(zram_control, S_IWUSR | S_IRUGO,
                         zram_control_show, zram_control_store),
         __ATTR_NULL,
 };

 struct class zram_class = {
         .name           = "zram-control",
         .class_attrs    = zram_class_attrs,
 };


 class_register(&zram_class);



or (even better) separate control files

 static struct class_attribute zram_class_attrs[] = {
         __ATTR(zram_add, ....),
         __ATTR(zram_remove, ....),
         __ATTR_NULL,
 };


so we can just echo `device_id' to add/remove devices

echo 1 > /sys/class/zram-control/zram_add
echo 1 > /sys/class/zram-control/zram_remove


handling it in FOO_store() functions:

 static ssize_t zram_add_store(struct class *class,
                         struct class_attribute *attr,
                         char *buf)


how about this?


	-ss

Re: [PATCH 0/8] introduce dynamic device creation/removal

[Andrew Morton]

On Sat, 28 Feb 2015 12:34:15 +0900 Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> On (02/27/15 14:51), Andrew Morton wrote:
> > hoo boy.  Creating a /dev node and doing ioctls on it is really old
> > school.  So old school that I've forgotten why we don't do it any more.
> > 
> > Hopefully Alan can recall the thinking?
> > 
> 
> perhaps, something like
> 
>  static struct class_attribute zram_class_attrs[] = {
>          __ATTR(zram_control, S_IWUSR | S_IRUGO,
>                          zram_control_show, zram_control_store),
>          __ATTR_NULL,
>  };
> 
>  struct class zram_class = {
>          .name           = "zram-control",
>          .class_attrs    = zram_class_attrs,
>  };
> 
> 
>  class_register(&zram_class);
> 
> 
> 
> or (even better) separate control files
> 
>  static struct class_attribute zram_class_attrs[] = {
>          __ATTR(zram_add, ....),
>          __ATTR(zram_remove, ....),
>          __ATTR_NULL,
>  };
> 
> 
> so we can just echo `device_id' to add/remove devices
> 
> echo 1 > /sys/class/zram-control/zram_add
> echo 1 > /sys/class/zram-control/zram_remove
> 
> 
> handling it in FOO_store() functions:
> 
>  static ssize_t zram_add_store(struct class *class,
>                          struct class_attribute *attr,
>                          char *buf)
> 
> 
> how about this?

That would be more conventional.  There's no pretty way of doing this
really.  

http://yarchive.net/comp/linux/ioctl.html does a pretty good job of the
thinking on ioctl - mainly the typelessness of it all.  That's very old
and doesn't discuss the 32/64 bit compat issues.

Your patch doesn't really have this problem at this stage, but once the
ioctl interface is added, new controls which are added later should use
it, and we're likely to get into the same issues.

Re: [PATCH 0/8] introduce dynamic device creation/removal

[Alan Cox]

> > This is a long term plan, of course, but I'd like to see sysfs functions go away
> > in a year or so. What do you think?
> 
> hoo boy.  Creating a /dev node and doing ioctls on it is really old
> school.  So old school that I've forgotten why we don't do it any more.
> 
> Hopefully Alan can recall the thinking?

Gee.. pass me the walking stick and the pipe !

The problem we used to have with it was re-use of /dev nodes and
permissions. For example if I create a ZRAM instance and then we crash
and my /dev is persistent then next boot I can potentially be opening
that left over node (or it may even be forgotten), which has old and
stale permissions on it.

With devtmpfs it's a bit easier and saner.

The old abuser of this was the old PCMCIA layer which has more exciting
versions of the problem as it used /tmp for some nodes.

Switching to sysfs probably fits better but the problem is the generic
one of revoking access rights to an object.

Alan