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

[Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>]

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