From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754535AbbCDASI (ORCPT ); Tue, 3 Mar 2015 19:18:08 -0500 Received: from mail-pd0-f178.google.com ([209.85.192.178]:33098 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752684AbbCDASG (ORCPT ); Tue, 3 Mar 2015 19:18:06 -0500 Date: Wed, 4 Mar 2015 09:18:06 +0900 From: Sergey Senozhatsky To: Andrew Morton Cc: Sergey Senozhatsky , Minchan Kim , Nitin Gupta , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH 5/8] zram: add dynamic device add/remove functionality Message-ID: <20150304001806.GA1864@swordfish> References: <1425386990-6339-1-git-send-email-sergey.senozhatsky@gmail.com> <1425386990-6339-6-git-send-email-sergey.senozhatsky@gmail.com> <20150303140146.1b2345cf490a477303ff3270@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150303140146.1b2345cf490a477303ff3270@linux-foundation.org> 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 (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 > > --- > > 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 > > +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 > > +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 > > +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/ > > 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 > > #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"; > > > > /* 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! > >