From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758660AbbDWDGg (ORCPT ); Wed, 22 Apr 2015 23:06:36 -0400 Received: from mail-pa0-f52.google.com ([209.85.220.52]:36154 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756412AbbDWDGd (ORCPT ); Wed, 22 Apr 2015 23:06:33 -0400 Date: Thu, 23 Apr 2015 12:06:26 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Andrew Morton , Nitin Gupta , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality Message-ID: <20150423030626.GI24928@blaptop> References: <1429185356-11096-1-git-send-email-sergey.senozhatsky@gmail.com> <1429185356-11096-11-git-send-email-sergey.senozhatsky@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429185356-11096-11-git-send-email-sergey.senozhatsky@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 16, 2015 at 08:55:56PM +0900, Sergey Senozhatsky wrote: > We currently don't support on-demand device creation. The one and only way > to have N zram devices is to specify num_devices module parameter (default > value 1). That means that if, for some reason, at some point, user wants > to have N + 1 devies he/she must umount all the existing devices, unload > the module, load the module passing num_devices equals to N + 1. And do > this again, if needed. > > This patch introduces zram control sysfs class, which has two sysfs > attrs: > - zram_add -- add a new zram device > - zram_remove -- remove a specific (device_id) zram device > > zram_add sysfs attr is read-only and has only automatic device id assignment > mode (as requested by Minchan Kim). read operation performed on this attr > creates a new zram device and returns back its device_id or error status. > > Usage example: > # add a new specific zram device > cat /sys/class/zram-control/zram_add > 2 > > # remove a specific zram device > echo 4 > /sys/class/zram-control/zram_remove > > Returning zram_add() error code back to user (-ENOMEM in this case) > > cat /sys/class/zram-control/zram_add > cat: /sys/class/zram-control/zram_add: Cannot allocate memory > > NOTE, there might be users who already depend on the fact that at > least zram0 device gets always created by zram_init(). Preserve this > behavior. > > [Andrew Morton : fix comment layout] > Reported-by: Minchan Kim > Signed-off-by: Sergey Senozhatsky > --- > Documentation/ABI/testing/sysfs-class-zram | 24 ++++++ > Documentation/blockdev/zram.txt | 23 +++++- > drivers/block/zram/zram_drv.c | 124 ++++++++++++++++++++++++++++- > 3 files changed, 166 insertions(+), 5 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-class-zram > > diff --git a/Documentation/ABI/testing/sysfs-class-zram b/Documentation/ABI/testing/sysfs-class-zram > new file mode 100644 > index 0000000..6f62ef5 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-zram > @@ -0,0 +1,24 @@ > +What: /sys/class/zram-control/ > +Date: August 2015 > +KernelVersion: 4.1 > +Contact: Sergey Senozhatsky > +Description: > + The zram-control/ class sub-directory belongs to zram > + device class > + > +What: /sys/class/zram-control/zram_add > +Date: August 2015 > +KernelVersion: 4.1 > +Contact: Sergey Senozhatsky > +Description: > + RO attribute. Read operation will cause zram to add a new > + device and return its device id back to user (so one can > + use /dev/zram), or error code. > + > +What: /sys/class/zram-control/zram_add > +Date: August 2015 > +KernelVersion: 4.1 > +Contact: Sergey Senozhatsky > +Description: > + Remove a specific /dev/zramX device, where X is a device_id > + provided by user > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt > index 2ccc741..44b1a77 100644 > --- a/Documentation/blockdev/zram.txt > +++ b/Documentation/blockdev/zram.txt > @@ -99,7 +99,24 @@ size of the disk when not in use so a huge zram is wasteful. > mkfs.ext4 /dev/zram1 > mount /dev/zram1 /tmp > > -7) Stats: > +7) Add/remove zram devices > + > +zram provides a control interface, which enables dynamic (on-demand) device > +addition and removal. > + > +In order to add a new /dev/zramX device, perform read operation on zram_add > +attribute. This will return either new device's device id (meaning that you > +can use /dev/zram) or error code. > + > +Example: > + cat /sys/class/zram-control/zram_add Why do we put zram-contol there rather than /sys/block/zram > + 1 > + > +To remove the existing /dev/zramX device (where X is a device id) > +execute > + echo X > /sys/class/zram-control/zram_remove > + > +8) Stats: > Per-device statistics are exported as various nodes under /sys/block/zram/ > > A brief description of exported device attritbutes. For more details please > @@ -178,11 +195,11 @@ line of text and contains the following stats separated by whitespace: > num_migrated > > > -8) Deactivate: > +9) Deactivate: > swapoff /dev/zram0 > umount /dev/zram1 > > -9) Reset: > +10) Reset: > Write any positive value to 'reset' sysfs node > echo 1 > /sys/block/zram0/reset > echo 1 > /sys/block/zram1/reset > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 2c2e7cc..848222a 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -33,10 +33,14 @@ > #include > #include > #include > +#include > > #include "zram_drv.h" > > static DEFINE_IDR(zram_index_idr); > +/* idr index must be protected */ > +static DEFINE_MUTEX(zram_index_mutex); > + > static int zram_major; > static const char *default_compressor = "lzo"; > > @@ -1168,8 +1172,15 @@ static int zram_add(int device_id) Why do zram_add need device_id? We decided to remove option user pass device_id. > if (!zram) > return -ENOMEM; > > - ret = idr_alloc(&zram_index_idr, zram, device_id, > - device_id + 1, GFP_KERNEL); > + if (device_id < 0) { > + /* generate new device_id */ > + ret = idr_alloc(&zram_index_idr, zram, 0, 0, GFP_KERNEL); > + device_id = ret; > + } else { > + /* use provided device_id */ > + ret = idr_alloc(&zram_index_idr, zram, device_id, > + device_id + 1, GFP_KERNEL); > + } > if (ret < 0) > goto out_free_dev; > > @@ -1278,6 +1289,105 @@ static void zram_remove(struct zram *zram) > kfree(zram); > } > > +/* > + * Lookup if there is any device pointer that match the given device_id. > + * return device pointer if so, or ERR_PTR() otherwise. > + */ > +static struct zram *zram_lookup(int dev_id) > +{ > + struct zram *zram; > + > + zram = idr_find(&zram_index_idr, dev_id); > + if (zram) > + return zram; > + return ERR_PTR(-ENODEV); Just return NULL which is more simple and readable. > +} > + > +/* zram module control sysfs attributes */ > +static ssize_t zram_add_show(struct class *class, > + struct class_attribute *attr, > + char *buf) > +{ > + int ret; > + > + mutex_lock(&zram_index_mutex); > + /* pick up available device_id */ > + ret = zram_add(-1); > + mutex_unlock(&zram_index_mutex); > + > + if (ret < 0) > + return ret; > + return scnprintf(buf, PAGE_SIZE, "%d\n", ret); > +} > + > +static ssize_t zram_remove_store(struct class *class, > + struct class_attribute *attr, > + const char *buf, > + size_t count) > +{ > + struct zram *zram; > + int ret, err, dev_id; > + > + mutex_lock(&zram_index_mutex); > + > + /* dev_id is gendisk->first_minor, which is `int' */ > + ret = kstrtoint(buf, 10, &dev_id); > + if (ret || dev_id < 0) { > + ret = -EINVAL; > + goto out; > + } > + > + zram = zram_lookup(dev_id); > + if (IS_ERR(zram)) { > + ret = PTR_ERR(zram); > + goto out; > + } > + > + /* > + * First, make ->disksize device attr RO, closing > + * zram_remove() vs disksize_store() race window Why don't you use zram->init_lock to protect the race? > + */ > + ret = sysfs_chmod_file(&disk_to_dev(zram->disk)->kobj, > + &dev_attr_disksize.attr, S_IRUGO); > + if (ret) > + goto out; > + > + ret = zram_reset_device(zram); > + if (ret == 0) { > + /* ->disksize is RO and there are no ->bd_openers */ > + zram_remove(zram); > + goto out; > + } > + > + /* > + * If there are still device bd_openers, try to make ->disksize > + * RW again and return. even if we fail to make ->disksize RW, > + * user still has RW ->reset attr. so it's possible to destroy > + * that device. > + */ > + err = sysfs_chmod_file(&disk_to_dev(zram->disk)->kobj, > + &dev_attr_disksize.attr, > + S_IWUSR | S_IRUGO); > + if (err) > + ret = err; > + > +out: > + mutex_unlock(&zram_index_mutex); > + return ret ? ret : count; > +} > + > +static struct class_attribute zram_control_class_attrs[] = { > + __ATTR_RO(zram_add), > + __ATTR_WO(zram_remove), > + __ATTR_NULL, > +}; > + > +static struct class zram_control_class = { > + .name = "zram-control", > + .owner = THIS_MODULE, > + .class_attrs = zram_control_class_attrs, > +}; > + > static int zram_exit_cb(int id, void *ptr, void *data) > { > zram_remove(ptr); > @@ -1286,6 +1396,7 @@ static int zram_exit_cb(int id, void *ptr, void *data) > > static void destroy_devices(void) > { > + class_unregister(&zram_control_class); > idr_for_each(&zram_index_idr, &zram_exit_cb, NULL); > idr_destroy(&zram_index_idr); > unregister_blkdev(zram_major, "zram"); > @@ -1295,14 +1406,23 @@ static int __init zram_init(void) > { > int ret, dev_id; > > + ret = class_register(&zram_control_class); > + if (ret) { > + pr_warn("Unable to register zram-control class\n"); > + return ret; > + } > + > zram_major = register_blkdev(0, "zram"); > if (zram_major <= 0) { > pr_warn("Unable to get major number\n"); > + class_unregister(&zram_control_class); > return -EBUSY; > } > > for (dev_id = 0; dev_id < num_devices; dev_id++) { > + mutex_lock(&zram_index_mutex); > ret = zram_add(dev_id); > + mutex_unlock(&zram_index_mutex); > if (ret < 0) > goto out_error; > } > -- > 2.4.0.rc2 > -- Kind regards, Minchan Kim