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

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

[Sergey Senozhatsky]

Hello,

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


    Usage example:
        # add a new specific zram device
        echo 4 > /sys/class/zram-control/zram_add

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


Patch set also does some cleanups and huge code reorganization.

v3:
-- add missing add/remove documentation
-- add patch 0008
-- pair class register with unregister, not class destroy function
-- do not leak class on register_blkdev() error
-- fix kernel version typo (should be 4.1) in documentation

v2:
-- switch to sysfs class, rather than using /dev/zram-control node and
doing IOCTL on it. we lose some features, though. like automatic
device_id generation.


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: reorganize code layout
  zram: add dynamic device add/remove functionality
  zram: remove max_num_devices limitation
  zram: report every added and removed device
  zram: trivial: correct flag operations comment

 Documentation/ABI/testing/sysfs-class-zram |  23 +
 Documentation/blockdev/zram.txt            |  21 +-
 drivers/block/zram/zram_drv.c              | 795 +++++++++++++++++------------
 drivers/block/zram/zram_drv.h              |   6 -
 4 files changed, 499 insertions(+), 346 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-zram

-- 
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 871bd35..8fc2566 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 8fc2566..6707b7b 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

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

[Andrew Morton]

On Tue,  3 Mar 2015 21:49:44 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> 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.
> 
> ...
>
> +static DEFINE_IDR(zram_index_idr);

There is no locking protecting this idr?

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

[Sergey Senozhatsky]

On (03/03/15 14:01), Andrew Morton wrote:
> > 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.
> > 
> > ...
> >
> > +static DEFINE_IDR(zram_index_idr);
> 
> There is no locking protecting this idr?
> 

correct. there is no concurrent idr index modification at this point.
add/remove functionality and concurrent idr modification will be intoriduced
in the patch 0005 zram add dynamic device add remove functionality (along
with `static DEFINE_MUTEX(zram_index_mutex)' idr protection).

	-ss

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

[Minchan Kim]

Hello Sergey,

Sorry for the late review.
I don't grab a time to review in detail still but I saw Andrew merged it
to mmotm today so at least I should ask somethings now and hope review
in detail soon.

On Tue, Mar 03, 2015 at 09:49:44PM +0900, Sergey Senozhatsky wrote:
> 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

So are you claiming using of idr is smaller memory footprint than zram included
list_head?
I hope why you want to use idr for dynamic device management.

> 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 8fc2566..6707b7b 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
> 

-- 
Kind regards,
Minchan Kim

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

[Sergey Senozhatsky]

Hello Minchan,

On (03/04/15 16:06), Minchan Kim wrote:
> > 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
> 
> So are you claiming using of idr is smaller memory footprint than zram included
> list_head?
> I hope why you want to use idr for dynamic device management.

only if we talk about sizeof(struct zram), of course. we pass zram pointers around,
do struct zram loads/stores (accessing disksize, meta, writing stats, etc.) quite a
lot. so keeping it small is a good thing, I think.

	-ss

> > 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 8fc2566..6707b7b 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
> > 
> 
> -- 
> Kind regards,
> Minchan Kim
> 

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

[Sergey Senozhatsky]

On (03/04/15 16:06), Minchan Kim wrote:
> So are you claiming using of idr is smaller memory footprint than zram included
> list_head?
> I hope why you want to use idr for dynamic device management.
> 

and idr handles auto device_id generation for us, all we need to do in zram code
is to pass '0, 0' to idr_alloc(), instead of 'device_id, device_id + 1'.
hoping that automatic id generation will be there one day.

	-ss

> > 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 8fc2566..6707b7b 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
> > 
> 
> -- 
> Kind regards,
> Minchan Kim
> 

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

[Minchan Kim]

On Wed, Mar 04, 2015 at 04:49:13PM +0900, Sergey Senozhatsky wrote:
> On (03/04/15 16:06), Minchan Kim wrote:
> > So are you claiming using of idr is smaller memory footprint than zram included
> > list_head?
> > I hope why you want to use idr for dynamic device management.
> > 
> 
> and idr handles auto device_id generation for us, all we need to do in zram code
> is to pass '0, 0' to idr_alloc(), instead of 'device_id, device_id + 1'.
> hoping that automatic id generation will be there one day.

About memory footprint, sizeof(list_head) might be win if we create zram dyamically
and there is few zram device in the system because IDR uses own metadata and bitmap
and so. But I don't think it's not too much if there is few device, either.
However, automatic id generation appeals more rather than memory footprint so I hope
you add it in description for someone to know why we decided idr in future.

Thanks.

> 
> 	-ss
> 
> > > 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 8fc2566..6707b7b 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
> > > 
> > 
> > -- 
> > Kind regards,
> > Minchan Kim
> > 

-- 
Kind regards,
Minchan Kim

[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 6707b7b..59662dc 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

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

[Minchan Kim]

On Tue, Mar 03, 2015 at 09:49:45PM +0900, Sergey Senozhatsky wrote:
> 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()

Nitpick:
Usually, we have used double underbar for internal functions(ie, __zram_reset_device).


> 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 6707b7b..59662dc 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
> 

-- 
Kind regards,
Minchan Kim

[PATCH 4/8] zram: reorganize code layout

[Sergey Senozhatsky]

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

Our current code layout looks a bit 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 | 593 +++++++++++++++++++++---------------------
 1 file changed, 296 insertions(+), 297 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 59662dc..bab8b20 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -63,6 +63,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)
 {
@@ -246,65 +359,25 @@ 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)
-{
-	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;
-}
+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 int is_partial_io(struct bio_vec *bvec)
+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)
@@ -358,56 +431,6 @@ 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)
-{
-	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
@@ -440,6 +463,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;
@@ -525,21 +549,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)
 {
@@ -667,36 +676,13 @@ out:
 	return ret;
 }
 
-static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
-			int offset, int rw)
-{
-	int ret;
-
-	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);
-	}
-
-	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
- * @offset: byte offset within physical block
- */
-static void zram_bio_discard(struct zram *zram, u32 index,
-			     int offset, struct bio *bio)
+/*
+ * zram_bio_discard - handler on discard request
+ * @index: physical block index in PAGE_SIZE units
+ * @offset: byte offset within physical block
+ */
+static void zram_bio_discard(struct zram *zram, u32 index,
+			     int offset, struct bio *bio)
 {
 	size_t n = bio->bi_iter.bi_size;
 	struct zram_meta *meta = zram->meta;
@@ -729,151 +715,29 @@ 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);
-
-	zram->limit_pages = 0;
+	int ret;
 
-	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;
-
-	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;
@@ -928,9 +792,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;
@@ -1010,6 +872,152 @@ out:
 	return err;
 }
 
+/* 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;
+
+	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;
+
+	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 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 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;
+}
+
+/* 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,
@@ -1026,15 +1034,6 @@ static DEVICE_ATTR_RW(mem_used_max);
 static DEVICE_ATTR_RW(max_comp_streams);
 static DEVICE_ATTR_RW(comp_algorithm);
 
-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 struct attribute *zram_disk_attrs[] = {
 	&dev_attr_disksize.attr,
 	&dev_attr_initstate.attr,
-- 
2.3.1.167.g7f4ba4b

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

[Sergey Senozhatsky]

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

Usage example:
	# add a new specific zram device
	echo 4 > /sys/class/zram-control/zram_add

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

There is no automatic device_id generation, so user is expected to
provide one.

NOTE, 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. 5) zram0
will also be created.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/ABI/testing/sysfs-class-zram |  23 ++++++
 Documentation/blockdev/zram.txt            |  23 +++++-
 drivers/block/zram/zram_drv.c              | 120 +++++++++++++++++++++++++++++
 3 files changed, 162 insertions(+), 4 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..99b2a1e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-zram
@@ -0,0 +1,23 @@
+What:		/sys/class/zram-control/
+Date:		March 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:		March 2015
+KernelVersion:	4.1
+Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+Description:
+		Add a specific /dev/zramX device, where X is a device_id
+		provided by user
+
+		What:		/sys/class/zram-control/zram_add
+Date:		March 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 7fcf9c6..4b140fa 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,
@@ -97,7 +99,20 @@ 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 (where X is a unique device id)
+execute
+	echo X > /sys/class/zram-control/zram_add
+
+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>/
 		disksize
@@ -113,11 +128,11 @@ size of the disk when not in use so a huge zram is wasteful.
 		mem_used_total
 		mem_used_max
 
-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 bab8b20..a457e16 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -33,16 +33,23 @@
 #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";
 
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
 
+#define ZRAM_CTL_ADD		1
+#define ZRAM_CTL_REMOVE		2
+
 #define ZRAM_ATTR_RO(name)						\
 static ssize_t name##_show(struct device *d,				\
 				struct device_attribute *attr, char *b)	\
@@ -1173,6 +1180,109 @@ 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);
+}
+
+/* common zram-control add/remove handler */
+static int zram_control(int cmd, const char *buf)
+{
+	struct zram *zram;
+	int ret = -ENOSYS, err, dev_id;
+
+	/* dev_id is gendisk->first_minor, which is `int' */
+	ret = kstrtoint(buf, 10, &dev_id);
+	if (ret || dev_id < 0)
+		return ret;
+
+	mutex_lock(&zram_index_mutex);
+	zram = zram_lookup(dev_id);
+
+	switch (cmd) {
+	case ZRAM_CTL_ADD:
+		if (!IS_ERR(zram)) {
+			ret = -EEXIST;
+			break;
+		}
+		ret = zram_add(dev_id);
+		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;
+}
+
+/* zram module control sysfs attributes */
+static ssize_t zram_add_store(struct class *class,
+			struct class_attribute *attr,
+			const char *buf,
+			size_t count)
+{
+	int ret = zram_control(ZRAM_CTL_ADD, buf);
+
+	return ret ? ret : count;
+}
+
+static ssize_t zram_remove_store(struct class *class,
+			struct class_attribute *attr,
+			const char *buf,
+			size_t count)
+{
+	int ret = zram_control(ZRAM_CTL_REMOVE, buf);
+
+	return ret ? ret : count;
+}
+
+static struct class_attribute zram_control_class_attrs[] = {
+	__ATTR_WO(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);
@@ -1181,6 +1291,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");
@@ -1197,14 +1308,23 @@ static int __init zram_init(void)
 		return -EINVAL;
 	}
 
+	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.3.1.167.g7f4ba4b

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

[Andrew Morton]

On Tue,  3 Mar 2015 21:49:47 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> Introduce zram-control sysfs class, which has two sysfs attrs:
> - zram_add      -- add a new specific (device_id) zram device
> - zram_remove   -- remove a specific (device_id) zram device
> 
> Usage example:
> 	# add a new specific zram device
> 	echo 4 > /sys/class/zram-control/zram_add
> 
> 	# remove a specific zram device
> 	echo 4 > /sys/class/zram-control/zram_remove
> 
> There is no automatic device_id generation, so user is expected to
> provide one.
> 
> NOTE, 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. 5) zram0
> will also be created.

This doesn't really explain why you think we need the patch.

- What's the current scheme for adding devices?

- What's wrong with it?

- Why is this better?

> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-class-zram |  23 ++++++
>  Documentation/blockdev/zram.txt            |  23 +++++-
>  drivers/block/zram/zram_drv.c              | 120 +++++++++++++++++++++++++++++
>  3 files changed, 162 insertions(+), 4 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..99b2a1e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-zram
> @@ -0,0 +1,23 @@
> +What:		/sys/class/zram-control/
> +Date:		March 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:		March 2015
> +KernelVersion:	4.1
> +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> +Description:
> +		Add a specific /dev/zramX device, where X is a device_id
> +		provided by user
> +
> +		What:		/sys/class/zram-control/zram_add
> +Date:		March 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 7fcf9c6..4b140fa 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,
> @@ -97,7 +99,20 @@ 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 (where X is a unique device id)
> +execute
> +	echo X > /sys/class/zram-control/zram_add
> +
> +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>/
>  		disksize
> @@ -113,11 +128,11 @@ size of the disk when not in use so a huge zram is wasteful.
>  		mem_used_total
>  		mem_used_max
>  
> -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 bab8b20..a457e16 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -33,16 +33,23 @@
>  #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";
>  
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +#define ZRAM_CTL_ADD		1
> +#define ZRAM_CTL_REMOVE		2
> +
>  #define ZRAM_ATTR_RO(name)						\
>  static ssize_t name##_show(struct device *d,				\
>  				struct device_attribute *attr, char *b)	\
> @@ -1173,6 +1180,109 @@ 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);
> +}
> +
> +/* common zram-control add/remove handler */
> +static int zram_control(int cmd, const char *buf)
> +{
> +	struct zram *zram;
> +	int ret = -ENOSYS, err, dev_id;
> +
> +	/* dev_id is gendisk->first_minor, which is `int' */
> +	ret = kstrtoint(buf, 10, &dev_id);
> +	if (ret || dev_id < 0)
> +		return ret;
> +
> +	mutex_lock(&zram_index_mutex);
> +	zram = zram_lookup(dev_id);
> +
> +	switch (cmd) {
> +	case ZRAM_CTL_ADD:
> +		if (!IS_ERR(zram)) {
> +			ret = -EEXIST;
> +			break;
> +		}
> +		ret = zram_add(dev_id);
> +		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;
> +}
> +
> +/* zram module control sysfs attributes */
> +static ssize_t zram_add_store(struct class *class,
> +			struct class_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	int ret = zram_control(ZRAM_CTL_ADD, buf);
> +
> +	return ret ? ret : count;
> +}

The types here are confused.  `ret' is int, `count' is size_t, the
function returns ssize_t and heaven knows what type "ret ?  ret :
count" evaluates to!

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

[Sergey Senozhatsky]

On (03/03/15 14:01), Andrew Morton wrote:
> > Introduce zram-control sysfs class, which has two sysfs attrs:
> > - zram_add      -- add a new specific (device_id) zram device
> > - zram_remove   -- remove a specific (device_id) zram device
> > 
> > Usage example:
> > 	# add a new specific zram device
> > 	echo 4 > /sys/class/zram-control/zram_add
> > 
> > 	# remove a specific zram device
> > 	echo 4 > /sys/class/zram-control/zram_remove
> > 
> > There is no automatic device_id generation, so user is expected to
> > provide one.
> > 
> > NOTE, 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. 5) zram0
> > will also be created.
> 
> This doesn't really explain why you think we need the patch.
> 
> - What's the current scheme for adding devices?
> 
> - What's wrong with it?
> 
> - Why is this better?

oh, yes. you are right.

We currently don't support 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.


	-ss


> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-zram |  23 ++++++
> >  Documentation/blockdev/zram.txt            |  23 +++++-
> >  drivers/block/zram/zram_drv.c              | 120 +++++++++++++++++++++++++++++
> >  3 files changed, 162 insertions(+), 4 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..99b2a1e
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-zram
> > @@ -0,0 +1,23 @@
> > +What:		/sys/class/zram-control/
> > +Date:		March 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:		March 2015
> > +KernelVersion:	4.1
> > +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > +Description:
> > +		Add a specific /dev/zramX device, where X is a device_id
> > +		provided by user
> > +
> > +		What:		/sys/class/zram-control/zram_add
> > +Date:		March 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 7fcf9c6..4b140fa 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,
> > @@ -97,7 +99,20 @@ 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 (where X is a unique device id)
> > +execute
> > +	echo X > /sys/class/zram-control/zram_add
> > +
> > +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>/
> >  		disksize
> > @@ -113,11 +128,11 @@ size of the disk when not in use so a huge zram is wasteful.
> >  		mem_used_total
> >  		mem_used_max
> >  
> > -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 bab8b20..a457e16 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -33,16 +33,23 @@
> >  #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";
> >  
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_CTL_ADD		1
> > +#define ZRAM_CTL_REMOVE		2
> > +
> >  #define ZRAM_ATTR_RO(name)						\
> >  static ssize_t name##_show(struct device *d,				\
> >  				struct device_attribute *attr, char *b)	\
> > @@ -1173,6 +1180,109 @@ 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);
> > +}
> > +
> > +/* common zram-control add/remove handler */
> > +static int zram_control(int cmd, const char *buf)
> > +{
> > +	struct zram *zram;
> > +	int ret = -ENOSYS, err, dev_id;
> > +
> > +	/* dev_id is gendisk->first_minor, which is `int' */
> > +	ret = kstrtoint(buf, 10, &dev_id);
> > +	if (ret || dev_id < 0)
> > +		return ret;
> > +
> > +	mutex_lock(&zram_index_mutex);
> > +	zram = zram_lookup(dev_id);
> > +
> > +	switch (cmd) {
> > +	case ZRAM_CTL_ADD:
> > +		if (!IS_ERR(zram)) {
> > +			ret = -EEXIST;
> > +			break;
> > +		}
> > +		ret = zram_add(dev_id);
> > +		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;
> > +}
> > +
> > +/* zram module control sysfs attributes */
> > +static ssize_t zram_add_store(struct class *class,
> > +			struct class_attribute *attr,
> > +			const char *buf,
> > +			size_t count)
> > +{
> > +	int ret = zram_control(ZRAM_CTL_ADD, buf);
> > +
> > +	return ret ? ret : count;
> > +}
> 
> The types here are confused.  `ret' is int, `count' is size_t, the
> function returns ssize_t and heaven knows what type "ret ?  ret :
> count" evaluates to!
> 
> 

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

[Minchan Kim]

On Tue, Mar 03, 2015 at 09:49:47PM +0900, Sergey Senozhatsky wrote:
> Introduce zram-control sysfs class, which has two sysfs attrs:
> - zram_add      -- add a new specific (device_id) zram device
> - zram_remove   -- remove a specific (device_id) zram device
> 
> Usage example:
> 	# add a new specific zram device
> 	echo 4 > /sys/class/zram-control/zram_add
> 
> 	# remove a specific zram device
> 	echo 4 > /sys/class/zram-control/zram_remove
> 
> There is no automatic device_id generation, so user is expected to
> provide one.

Hmm, so every user want to create zram dynamically should lock
the file to synchronize other user who want create same number
zram device? Otherwise, looping until he is successful?

Why should user specifiy zram-device id to create?

> 
> NOTE, 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. 5) zram0
> will also be created.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-class-zram |  23 ++++++
>  Documentation/blockdev/zram.txt            |  23 +++++-
>  drivers/block/zram/zram_drv.c              | 120 +++++++++++++++++++++++++++++
>  3 files changed, 162 insertions(+), 4 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..99b2a1e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-zram
> @@ -0,0 +1,23 @@
> +What:		/sys/class/zram-control/
> +Date:		March 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:		March 2015
> +KernelVersion:	4.1
> +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> +Description:
> +		Add a specific /dev/zramX device, where X is a device_id
> +		provided by user
> +
> +		What:		/sys/class/zram-control/zram_add
> +Date:		March 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 7fcf9c6..4b140fa 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,
> @@ -97,7 +99,20 @@ 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 (where X is a unique device id)
> +execute
> +	echo X > /sys/class/zram-control/zram_add
> +
> +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>/
>  		disksize
> @@ -113,11 +128,11 @@ size of the disk when not in use so a huge zram is wasteful.
>  		mem_used_total
>  		mem_used_max
>  
> -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 bab8b20..a457e16 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -33,16 +33,23 @@
>  #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";
>  
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +#define ZRAM_CTL_ADD		1
> +#define ZRAM_CTL_REMOVE		2
> +
>  #define ZRAM_ATTR_RO(name)						\
>  static ssize_t name##_show(struct device *d,				\
>  				struct device_attribute *attr, char *b)	\
> @@ -1173,6 +1180,109 @@ 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);
> +}
> +
> +/* common zram-control add/remove handler */
> +static int zram_control(int cmd, const char *buf)
> +{
> +	struct zram *zram;
> +	int ret = -ENOSYS, err, dev_id;
> +
> +	/* dev_id is gendisk->first_minor, which is `int' */
> +	ret = kstrtoint(buf, 10, &dev_id);
> +	if (ret || dev_id < 0)
> +		return ret;
> +
> +	mutex_lock(&zram_index_mutex);
> +	zram = zram_lookup(dev_id);
> +
> +	switch (cmd) {
> +	case ZRAM_CTL_ADD:
> +		if (!IS_ERR(zram)) {
> +			ret = -EEXIST;
> +			break;
> +		}
> +		ret = zram_add(dev_id);
> +		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;
> +}
> +
> +/* zram module control sysfs attributes */
> +static ssize_t zram_add_store(struct class *class,
> +			struct class_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	int ret = zram_control(ZRAM_CTL_ADD, buf);
> +
> +	return ret ? ret : count;
> +}
> +
> +static ssize_t zram_remove_store(struct class *class,
> +			struct class_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	int ret = zram_control(ZRAM_CTL_REMOVE, buf);
> +
> +	return ret ? ret : count;
> +}
> +
> +static struct class_attribute zram_control_class_attrs[] = {
> +	__ATTR_WO(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);
> @@ -1181,6 +1291,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");
> @@ -1197,14 +1308,23 @@ static int __init zram_init(void)
>  		return -EINVAL;
>  	}
>  
> +	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.3.1.167.g7f4ba4b
> 

-- 
Kind regards,
Minchan Kim

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

[Sergey Senozhatsky]

On (03/04/15 16:10), Minchan Kim wrote:
> On Tue, Mar 03, 2015 at 09:49:47PM +0900, Sergey Senozhatsky wrote:
> > Introduce zram-control sysfs class, which has two sysfs attrs:
> > - zram_add      -- add a new specific (device_id) zram device
> > - zram_remove   -- remove a specific (device_id) zram device
> > 
> > Usage example:
> > 	# add a new specific zram device
> > 	echo 4 > /sys/class/zram-control/zram_add
> > 
> > 	# remove a specific zram device
> > 	echo 4 > /sys/class/zram-control/zram_remove
> > 
> > There is no automatic device_id generation, so user is expected to
> > provide one.
> 
> Hmm, so every user want to create zram dynamically should lock
> the file to synchronize other user who want create same number
> zram device? Otherwise, looping until he is successful?
> 
> Why should user specifiy zram-device id to create?
> 

one of them will create a new device, the other one will get -EEXIST.
quite decent.

I'm not sure I know how to return device id back to use space in
response to
	echo -1 > /sys/class/zram-control/zram_add


can be added in follow up patches.

	-ss

> > 
> > NOTE, 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. 5) zram0
> > will also be created.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-zram |  23 ++++++
> >  Documentation/blockdev/zram.txt            |  23 +++++-
> >  drivers/block/zram/zram_drv.c              | 120 +++++++++++++++++++++++++++++
> >  3 files changed, 162 insertions(+), 4 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..99b2a1e
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-zram
> > @@ -0,0 +1,23 @@
> > +What:		/sys/class/zram-control/
> > +Date:		March 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:		March 2015
> > +KernelVersion:	4.1
> > +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > +Description:
> > +		Add a specific /dev/zramX device, where X is a device_id
> > +		provided by user
> > +
> > +		What:		/sys/class/zram-control/zram_add
> > +Date:		March 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 7fcf9c6..4b140fa 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,
> > @@ -97,7 +99,20 @@ 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 (where X is a unique device id)
> > +execute
> > +	echo X > /sys/class/zram-control/zram_add
> > +
> > +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>/
> >  		disksize
> > @@ -113,11 +128,11 @@ size of the disk when not in use so a huge zram is wasteful.
> >  		mem_used_total
> >  		mem_used_max
> >  
> > -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 bab8b20..a457e16 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -33,16 +33,23 @@
> >  #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";
> >  
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_CTL_ADD		1
> > +#define ZRAM_CTL_REMOVE		2
> > +
> >  #define ZRAM_ATTR_RO(name)						\
> >  static ssize_t name##_show(struct device *d,				\
> >  				struct device_attribute *attr, char *b)	\
> > @@ -1173,6 +1180,109 @@ 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);
> > +}
> > +
> > +/* common zram-control add/remove handler */
> > +static int zram_control(int cmd, const char *buf)
> > +{
> > +	struct zram *zram;
> > +	int ret = -ENOSYS, err, dev_id;
> > +
> > +	/* dev_id is gendisk->first_minor, which is `int' */
> > +	ret = kstrtoint(buf, 10, &dev_id);
> > +	if (ret || dev_id < 0)
> > +		return ret;
> > +
> > +	mutex_lock(&zram_index_mutex);
> > +	zram = zram_lookup(dev_id);
> > +
> > +	switch (cmd) {
> > +	case ZRAM_CTL_ADD:
> > +		if (!IS_ERR(zram)) {
> > +			ret = -EEXIST;
> > +			break;
> > +		}
> > +		ret = zram_add(dev_id);
> > +		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;
> > +}
> > +
> > +/* zram module control sysfs attributes */
> > +static ssize_t zram_add_store(struct class *class,
> > +			struct class_attribute *attr,
> > +			const char *buf,
> > +			size_t count)
> > +{
> > +	int ret = zram_control(ZRAM_CTL_ADD, buf);
> > +
> > +	return ret ? ret : count;
> > +}
> > +
> > +static ssize_t zram_remove_store(struct class *class,
> > +			struct class_attribute *attr,
> > +			const char *buf,
> > +			size_t count)
> > +{
> > +	int ret = zram_control(ZRAM_CTL_REMOVE, buf);
> > +
> > +	return ret ? ret : count;
> > +}
> > +
> > +static struct class_attribute zram_control_class_attrs[] = {
> > +	__ATTR_WO(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);
> > @@ -1181,6 +1291,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");
> > @@ -1197,14 +1308,23 @@ static int __init zram_init(void)
> >  		return -EINVAL;
> >  	}
> >  
> > +	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.3.1.167.g7f4ba4b
> > 
> 
> -- 
> Kind regards,
> Minchan Kim
> 

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

[Sergey Senozhatsky]

On (03/04/15 16:29), Sergey Senozhatsky wrote:
> > Why should user specifiy zram-device id to create?
> > 
> 
> one of them will create a new device, the other one will get -EEXIST.
> quite decent.
> 
> I'm not sure I know how to return device id back to use space in
> response to
> 	echo -1 > /sys/class/zram-control/zram_add
> 

hm... that actually doesn't look so hard.

all I need to do is just change zram_add attr to RW (or create a new RO
attribute `zram_auto_add', or whatever) and treat read access to that
zram_add_read() or zram_auto_add_read() attr as (atomic operation):
	-- generate new device_id X
	-- create corresponding zramX device
	-- return that X as char *text to user space (or error). the
	same way we sprintf() stats.

what do you think? I'll play with it.


	-ss


> > > 
> > > NOTE, 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. 5) zram0
> > > will also be created.
> > > 
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-class-zram |  23 ++++++
> > >  Documentation/blockdev/zram.txt            |  23 +++++-
> > >  drivers/block/zram/zram_drv.c              | 120 +++++++++++++++++++++++++++++
> > >  3 files changed, 162 insertions(+), 4 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..99b2a1e
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-zram
> > > @@ -0,0 +1,23 @@
> > > +What:		/sys/class/zram-control/
> > > +Date:		March 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:		March 2015
> > > +KernelVersion:	4.1
> > > +Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > +Description:
> > > +		Add a specific /dev/zramX device, where X is a device_id
> > > +		provided by user
> > > +
> > > +		What:		/sys/class/zram-control/zram_add
> > > +Date:		March 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 7fcf9c6..4b140fa 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,
> > > @@ -97,7 +99,20 @@ 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 (where X is a unique device id)
> > > +execute
> > > +	echo X > /sys/class/zram-control/zram_add
> > > +
> > > +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>/
> > >  		disksize
> > > @@ -113,11 +128,11 @@ size of the disk when not in use so a huge zram is wasteful.
> > >  		mem_used_total
> > >  		mem_used_max
> > >  
> > > -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 bab8b20..a457e16 100644
> > > --- a/drivers/block/zram/zram_drv.c
> > > +++ b/drivers/block/zram/zram_drv.c
> > > @@ -33,16 +33,23 @@
> > >  #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";
> > >  
> > >  /* Module params (documentation at end) */
> > >  static unsigned int num_devices = 1;
> > >  
> > > +#define ZRAM_CTL_ADD		1
> > > +#define ZRAM_CTL_REMOVE		2
> > > +
> > >  #define ZRAM_ATTR_RO(name)						\
> > >  static ssize_t name##_show(struct device *d,				\
> > >  				struct device_attribute *attr, char *b)	\
> > > @@ -1173,6 +1180,109 @@ 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);
> > > +}
> > > +
> > > +/* common zram-control add/remove handler */
> > > +static int zram_control(int cmd, const char *buf)
> > > +{
> > > +	struct zram *zram;
> > > +	int ret = -ENOSYS, err, dev_id;
> > > +
> > > +	/* dev_id is gendisk->first_minor, which is `int' */
> > > +	ret = kstrtoint(buf, 10, &dev_id);
> > > +	if (ret || dev_id < 0)
> > > +		return ret;
> > > +
> > > +	mutex_lock(&zram_index_mutex);
> > > +	zram = zram_lookup(dev_id);
> > > +
> > > +	switch (cmd) {
> > > +	case ZRAM_CTL_ADD:
> > > +		if (!IS_ERR(zram)) {
> > > +			ret = -EEXIST;
> > > +			break;
> > > +		}
> > > +		ret = zram_add(dev_id);
> > > +		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;
> > > +}
> > > +
> > > +/* zram module control sysfs attributes */
> > > +static ssize_t zram_add_store(struct class *class,
> > > +			struct class_attribute *attr,
> > > +			const char *buf,
> > > +			size_t count)
> > > +{
> > > +	int ret = zram_control(ZRAM_CTL_ADD, buf);
> > > +
> > > +	return ret ? ret : count;
> > > +}
> > > +
> > > +static ssize_t zram_remove_store(struct class *class,
> > > +			struct class_attribute *attr,
> > > +			const char *buf,
> > > +			size_t count)
> > > +{
> > > +	int ret = zram_control(ZRAM_CTL_REMOVE, buf);
> > > +
> > > +	return ret ? ret : count;
> > > +}
> > > +
> > > +static struct class_attribute zram_control_class_attrs[] = {
> > > +	__ATTR_WO(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);
> > > @@ -1181,6 +1291,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");
> > > @@ -1197,14 +1308,23 @@ static int __init zram_init(void)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	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.3.1.167.g7f4ba4b
> > > 
> > 
> > -- 
> > Kind regards,
> > Minchan Kim
> > 
> 

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

[Sergey Senozhatsky]

On (03/04/15 17:19), Sergey Senozhatsky wrote:
> hm... that actually doesn't look so hard.
> 
> all I need to do is just change zram_add attr to RW (or create a new RO
> attribute `zram_auto_add', or whatever) and treat read access to that
> zram_add_read() or zram_auto_add_read() attr as (atomic operation):
> 	-- generate new device_id X
> 	-- create corresponding zramX device
> 	-- return that X as char *text to user space (or error). the
> 	same way we sprintf() stats.
> 
> what do you think? I'll play with it.
> 

that will do the trick. testing simple patch:

# cat /sys/class/zram-control/zram_add
1
# cat /sys/class/zram-control/zram_add
2
# cat /sys/class/zram-control/zram_add
3
# cat /sys/class/zram-control/zram_add
4
# cat /sys/class/zram-control/zram_add
5

dmesg:
[14339.063075] zram: Added device: zram1
[14339.972633] zram: Added device: zram2
[14340.626564] zram: Added device: zram3
[14341.242134] zram: Added device: zram4
[14341.850853] zram: Added device: zram5


try to add already existing device_id

# echo 3 > /sys/class/zram-control/zram_add
-bash: echo: write error: File exists


zram_add() error propogation to user space (-ENOMEM in this case)

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


will send out new patch set later today.

	-ss

[PATCH 6/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 | 8 +-------
 drivers/block/zram/zram_drv.h | 6 ------
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a457e16..f1182a7 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1302,12 +1302,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 = class_register(&zram_control_class);
 	if (ret) {
 		pr_warn("Unable to register zram-control class\n");
@@ -1346,7 +1340,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 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 7/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 f1182a7..1662fd1 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1149,6 +1149,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:
@@ -1175,6 +1177,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);
@@ -1295,7 +1299,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)
@@ -1323,7 +1326,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

[PATCH 8/8] 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 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 1662fd1..087b043 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -70,7 +70,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.3.1.167.g7f4ba4b

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

[Andrew Morton]

On Tue,  3 Mar 2015 21:49:42 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

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

This patchset and the "make automatic device_id generation possible"
still appear to have quite a few unresolved issues.  So I'm holding
them out of the 4.1 merge window.

Unfortunately these were the first-arriving zram patches, so the later
ones required quite a bit of mangling.  Hopefully I got it all right.

This was all a bit disruptive.  Please let's not leave major patchsets
floating about in an incomplete/unresolved state for week after week?

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

[Minchan Kim]

On Wed, Apr 15, 2015 at 02:37:17PM -0700, Andrew Morton wrote:
> On Tue,  3 Mar 2015 21:49:42 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:
> 
> > Hello,
> > 
> > This patchset introduces zram-control sysfs class, which has two sysfs
> > attrs:
> >  - zram_add     -- add a new specific (device_id) zram device
> >  - zram_remove  -- remove a specific (device_id) zram device
> 
> This patchset and the "make automatic device_id generation possible"
> still appear to have quite a few unresolved issues.  So I'm holding
> them out of the 4.1 merge window.

There is no unresolved issue to me. Only one thing I suspect was the
feature user enforce new device id for dynamic device addition and
we finally decided to remove the function because there was no useful
usecase at this point. Sergey and other userland people agreed that
so Sergey sent a patch [zram: do not let user enforce new device dev_id]
https://lkml.org/lkml/2015/3/6/427
So, I'm happy with that. Acutally, I wanted to resend whole patchset
for dynamic device creation/remove patchset with corrected version
(ie, remove user enforce new device id) to avoid confusion but didn't
said it to Sergey. It was my bad.

Sergey, Could you resend this patchset without user's enforce device id
function based on new -rc1?

> 
> Unfortunately these were the first-arriving zram patches, so the later
> ones required quite a bit of mangling.  Hopefully I got it all right.
> 
> This was all a bit disruptive.  Please let's not leave major patchsets
> floating about in an incomplete/unresolved state for week after week?

I will keep it in mind.
Thanks.

-- 
Kind regards,
Minchan Kim

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

[Sergey Senozhatsky]

Hello,

On (04/16/15 08:40), Minchan Kim wrote:
> On Wed, Apr 15, 2015 at 02:37:17PM -0700, Andrew Morton wrote:
> > On Tue,  3 Mar 2015 21:49:42 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:
> > 
> > > Hello,
> > > 
> > > This patchset introduces zram-control sysfs class, which has two sysfs
> > > attrs:
> > >  - zram_add     -- add a new specific (device_id) zram device
> > >  - zram_remove  -- remove a specific (device_id) zram device
> > 
> > This patchset and the "make automatic device_id generation possible"
> > still appear to have quite a few unresolved issues.  So I'm holding
> > them out of the 4.1 merge window.
> 
> There is no unresolved issue to me. Only one thing I suspect was the
> feature user enforce new device id for dynamic device addition and
> we finally decided to remove the function because there was no useful
> usecase at this point.

I'm not aware of any unresolved issues. am I missing something?


> Sergey and other userland people agreed that
> so Sergey sent a patch [zram: do not let user enforce new device dev_id]
> https://lkml.org/lkml/2015/3/6/427
> So, I'm happy with that. Acutally, I wanted to resend whole patchset
> for dynamic device creation/remove patchset with corrected version
> (ie, remove user enforce new device id) to avoid confusion but didn't
> said it to Sergey. It was my bad.
> 
> Sergey, Could you resend this patchset without user's enforce device id
> function based on new -rc1?

ok, agree. I'll re-submit later today.


	-ss

> > 
> > Unfortunately these were the first-arriving zram patches, so the later
> > ones required quite a bit of mangling.  Hopefully I got it all right.
> > 
> > This was all a bit disruptive.  Please let's not leave major patchsets
> > floating about in an incomplete/unresolved state for week after week?
> 
> I will keep it in mind.
> Thanks.
> 
> -- 
> Kind regards,
> Minchan Kim
> 

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

[Sergey Senozhatsky]

On (04/15/15 14:37), Andrew Morton wrote:
> Unfortunately these were the first-arriving zram patches, so the later
> ones required quite a bit of mangling.  Hopefully I got it all right.
> 
> This was all a bit disruptive.  Please let's not leave major patchsets
> floating about in an incomplete/unresolved state for week after week?
> 


Andrew, sorry for confusion.

I will fold/cleanup and resend the whole patchset. sorry for

I currently have:

 0001-zram-cosmetic-ZRAM_ATTR_RO-code-formatting-tweak.patch
 0002-zram-use-idr-instead-of-zram_devices-array.patch
 0003-zram-factor-out-device-reset-from-reset_store.patch
 0004-zram-reorganize-code-layout.patch
 0005-zram-add-dynamic-device-add-remove-functionality.patch
 0006-zram-add-dynamic-device-add-remove-functionality-fix.patch
 0007-zram-remove-max_num_devices-limitation.patch
 0008-zram-report-every-added-and-removed-device.patch
 0009-zram-trivial-correct-flag-operations-comment.patch
 0010-zram-return-zram-device_id-value-from-zram_add.patch
 0011-zram-introduce-automatic-device_id-generation.patch
 0012-zram-introduce-automatic-device_id-generation-fix.patch
 0013-zram-do-not-let-user-enforce-new-device-dev_id.patch
 0014-zram-support-compaction.patch
 0015-zram-remove-num_migrated-device-attr.patch
 0016-zram-move-compact_store-to-sysfs-functions-area.patch
 0017-zram-use-generic-start-end-io-accounting.patch
 0018-zram-export-new-io_stat-sysfs-attrs.patch
 0019-zram-export-new-mm_stat-sysfs-attrs.patch
 0020-zram-deprecate-zram-attrs-sysfs-nodes.patch
 0021-zram-fix-error-return-code.patch

in my tree. the resulting patchset will be smaller in size.
I'll send it out later this day.

	-ss

[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 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