[PATCH 0/5 v3] zram: minor cleanups and features

[PATCH 0/5 v3] zram: minor cleanups and features

[Nitin Gupta]

Changelog: v3 vs v2:
 - Provide proper summary for patch 1/5 (fix for sparse warnings)
 
Changelog: v2 vs v1:
 - Fixed a sparse warning (make zram_slot_free_notify static)
 - Remove check for zero disksize since we now set it to
   some default size during initialization.

These changes address many of the issues list at:
http://code.google.com/p/compcache/issues/list

Thanks to Jerome Marchand for reviews.

Signed-off-by: Nitin Gupta <ngupta@vflare.org>

Nitin Gupta (5):
  zram: Fix sparse warnings
  zram: Kernel config option for number of devices
  zram: Make gobal variables use unique names
  zram: Simplify zram disk resizing interface
  zram: Set initial disksize to some default value

 drivers/staging/zram/Kconfig      |    9 ++++
 drivers/staging/zram/zram_drv.c   |   95 ++++++++++++++++---------------------
 drivers/staging/zram/zram_drv.h   |    6 +-
 drivers/staging/zram/zram_sysfs.c |    4 +-
 4 files changed, 55 insertions(+), 59 deletions(-)

-- 
1.7.6

[PATCH 1/5] zram: Fix sparse warnings

[Nitin Gupta]

Fixes sparse warning:
zram_drv.c:666:6: warning: symbol 'zram_slot_free_notify' was not
declared. Should it be static?

Also, max_zpage_size is now size_t just to be consistent with data-type
of other variables maintaining sizes of various kinds.

Signed-off-by: Nitin Gupta <ngupta@vflare.org>
---
 drivers/staging/zram/zram_drv.c |    3 ++-
 drivers/staging/zram/zram_drv.h |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index d70ec1a..03ec5a2 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -684,7 +684,8 @@ fail:
 	return ret;
 }
 
-void zram_slot_free_notify(struct block_device *bdev, unsigned long index)
+static void zram_slot_free_notify(struct block_device *bdev,
+				unsigned long index)
 {
 	struct zram *zram;
 
diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
index abe5221..70ce886 100644
--- a/drivers/staging/zram/zram_drv.h
+++ b/drivers/staging/zram/zram_drv.h
@@ -47,7 +47,7 @@ static const unsigned default_disksize_perc_ram = 25;
  * Pages that compress to size greater than this are stored
  * uncompressed in memory.
  */
-static const unsigned max_zpage_size = PAGE_SIZE / 4 * 3;
+static const size_t max_zpage_size = PAGE_SIZE / 4 * 3;
 
 /*
  * NOTE: max_zpage_size must be less than or equal to:
-- 
1.7.6

[PATCH 2/5] zram: Kernel config option for number of devices

[Nitin Gupta]

Allows configuring default number of zram devices
as kernel config option. User can override this
value using 'num_devices' module parameter.

Signed-off-by: Nitin Gupta <ngupta@vflare.org>
---
 drivers/staging/zram/Kconfig    |    9 +++++++++
 drivers/staging/zram/zram_drv.c |   13 ++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/zram/Kconfig b/drivers/staging/zram/Kconfig
index 3bec4db..ca31cb3 100644
--- a/drivers/staging/zram/Kconfig
+++ b/drivers/staging/zram/Kconfig
@@ -21,6 +21,15 @@ config ZRAM
 	  See zram.txt for more information.
 	  Project home: http://compcache.googlecode.com/
 
+config ZRAM_NUM_DEVICES
+	int "Default number of zram devices"
+	depends on ZRAM
+	range 1 32
+	default 1
+	help
+	  Select default number of zram devices. You can override this value
+	  using 'num_devices' module parameter.
+
 config ZRAM_DEBUG
 	bool "Compressed RAM block device debug support"
 	depends on ZRAM
diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 03ec5a2..55f2c0e 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -781,6 +781,14 @@ static int __init zram_init(void)
 {
 	int ret, dev_id;
 
+	/*
+	 * Module parameter not specified by user. Use default
+	 * value as defined during kernel config.
+	 */
+	if (num_devices == 0) {
+		num_devices = CONFIG_ZRAM_NUM_DEVICES;
+	}
+
 	if (num_devices > max_num_devices) {
 		pr_warning("Invalid value for num_devices: %u\n",
 				num_devices);
@@ -795,11 +803,6 @@ static int __init zram_init(void)
 		goto out;
 	}
 
-	if (!num_devices) {
-		pr_info("num_devices not specified. Using default: 1\n");
-		num_devices = 1;
-	}
-
 	/* Allocate the device array and initialize each one */
 	pr_info("Creating %u devices ...\n", num_devices);
 	devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
-- 
1.7.6

[PATCH 3/5] zram: Make gobal variables use unique names

[Nitin Gupta]

Global variables 'num_devices' and 'devices' are too
general to be global. This patch switches the name to
be "zram_devices".

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Nitin Gupta <ngupta@vflare.org>
---
 drivers/staging/zram/zram_drv.c   |   36 ++++++++++++++++++------------------
 drivers/staging/zram/zram_drv.h   |    4 ++--
 drivers/staging/zram/zram_sysfs.c |    4 ++--
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 55f2c0e..b2e29dd 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -37,10 +37,10 @@
 
 /* Globals */
 static int zram_major;
-struct zram *devices;
+struct zram *zram_devices;
 
 /* Module params (documentation at end) */
-unsigned int num_devices;
+unsigned int zram_num_devices;
 
 static void zram_stat_inc(u32 *v)
 {
@@ -785,13 +785,12 @@ static int __init zram_init(void)
 	 * Module parameter not specified by user. Use default
 	 * value as defined during kernel config.
 	 */
-	if (num_devices == 0) {
-		num_devices = CONFIG_ZRAM_NUM_DEVICES;
-	}
+	if (zram_num_devices == 0)
+		zram_num_devices = CONFIG_ZRAM_NUM_DEVICES;
 
-	if (num_devices > max_num_devices) {
+	if (zram_num_devices > max_num_devices) {
 		pr_warning("Invalid value for num_devices: %u\n",
-				num_devices);
+				zram_num_devices);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -804,15 +803,16 @@ static int __init zram_init(void)
 	}
 
 	/* Allocate the device array and initialize each one */
-	pr_info("Creating %u devices ...\n", num_devices);
-	devices = kzalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
-	if (!devices) {
+	pr_info("Creating %u devices ...\n", zram_num_devices);
+	zram_devices = kzalloc(zram_num_devices * sizeof(struct zram),
+				GFP_KERNEL);
+	if (!zram_devices) {
 		ret = -ENOMEM;
 		goto unregister;
 	}
 
-	for (dev_id = 0; dev_id < num_devices; dev_id++) {
-		ret = create_device(&devices[dev_id], dev_id);
+	for (dev_id = 0; dev_id < zram_num_devices; dev_id++) {
+		ret = create_device(&zram_devices[dev_id], dev_id);
 		if (ret)
 			goto free_devices;
 	}
@@ -821,8 +821,8 @@ static int __init zram_init(void)
 
 free_devices:
 	while (dev_id)
-		destroy_device(&devices[--dev_id]);
-	kfree(devices);
+		destroy_device(&zram_devices[--dev_id]);
+	kfree(zram_devices);
 unregister:
 	unregister_blkdev(zram_major, "zram");
 out:
@@ -834,8 +834,8 @@ static void __exit zram_exit(void)
 	int i;
 	struct zram *zram;
 
-	for (i = 0; i < num_devices; i++) {
-		zram = &devices[i];
+	for (i = 0; i < zram_num_devices; i++) {
+		zram = &zram_devices[i];
 
 		destroy_device(zram);
 		if (zram->init_done)
@@ -844,11 +844,11 @@ static void __exit zram_exit(void)
 
 	unregister_blkdev(zram_major, "zram");
 
-	kfree(devices);
+	kfree(zram_devices);
 	pr_debug("Cleanup done!\n");
 }
 
-module_param(num_devices, uint, 0);
+module_param_named(num_devices, zram_num_devices, uint, 0);
 MODULE_PARM_DESC(num_devices, "Number of zram devices");
 
 module_init(zram_init);
diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
index 70ce886..2a9eefd 100644
--- a/drivers/staging/zram/zram_drv.h
+++ b/drivers/staging/zram/zram_drv.h
@@ -123,8 +123,8 @@ struct zram {
 	struct zram_stats stats;
 };
 
-extern struct zram *devices;
-extern unsigned int num_devices;
+extern struct zram *zram_devices;
+extern unsigned int zram_num_devices;
 #ifdef CONFIG_SYSFS
 extern struct attribute_group zram_disk_attr_group;
 #endif
diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
index a70cc01..df1f9dc 100644
--- a/drivers/staging/zram/zram_sysfs.c
+++ b/drivers/staging/zram/zram_sysfs.c
@@ -34,8 +34,8 @@ static struct zram *dev_to_zram(struct device *dev)
 	int i;
 	struct zram *zram = NULL;
 
-	for (i = 0; i < num_devices; i++) {
-		zram = &devices[i];
+	for (i = 0; i < zram_num_devices; i++) {
+		zram = &zram_devices[i];
 		if (disk_to_dev(zram->disk) == dev)
 			break;
 	}
-- 
1.7.6

[PATCH 4/5] zram: Simplify zram disk resizing interface

[Nitin Gupta]

Also remove unnecessary messages.

Signed-off-by: Nitin Gupta <ngupta@vflare.org>
Reviewed-by: Jerome Marchand <jmarchan@redhat.com>
---
 drivers/staging/zram/zram_drv.c |   42 +++++++++++---------------------------
 1 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index b2e29dd..b1c4abd 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -104,33 +104,16 @@ static int page_zero_filled(void *ptr)
 	return 1;
 }
 
-static void zram_set_disksize(struct zram *zram, size_t totalram_bytes)
+static u64 zram_default_disksize_bytes(void)
 {
-	if (!zram->disksize) {
-		pr_info(
-		"disk size not provided. You can use disksize_kb module "
-		"param to specify size.\nUsing default: (%u%% of RAM).\n",
-		default_disksize_perc_ram
-		);
-		zram->disksize = default_disksize_perc_ram *
-					(totalram_bytes / 100);
-	}
-
-	if (zram->disksize > 2 * (totalram_bytes)) {
-		pr_info(
-		"There is little point creating a zram of greater than "
-		"twice the size of memory since we expect a 2:1 compression "
-		"ratio. Note that zram uses about 0.1%% of the size of "
-		"the disk when not in use so a huge zram is "
-		"wasteful.\n"
-		"\tMemory Size: %zu kB\n"
-		"\tSize you selected: %llu kB\n"
-		"Continuing anyway ...\n",
-		totalram_bytes >> 10, zram->disksize
-		);
-	}
-
-	zram->disksize &= PAGE_MASK;
+	return ((totalram_pages << PAGE_SHIFT) *
+		default_disksize_perc_ram / 100) & PAGE_MASK;
+}
+
+static void zram_set_disksize(struct zram *zram, u64 size_bytes)
+{
+	zram->disksize = size_bytes;
+	set_capacity(zram->disk, size_bytes >> SECTOR_SHIFT);
 }
 
 static void zram_free_page(struct zram *zram, size_t index)
@@ -632,7 +615,8 @@ int zram_init_device(struct zram *zram)
 		return 0;
 	}
 
-	zram_set_disksize(zram, totalram_pages << PAGE_SHIFT);
+	if (!zram->disksize)
+		zram_set_disksize(zram, zram_default_disksize_bytes());
 
 	zram->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
 	if (!zram->compress_workmem) {
@@ -658,8 +642,6 @@ int zram_init_device(struct zram *zram)
 		goto fail;
 	}
 
-	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
-
 	/* zram devices sort of resembles non-rotational disks */
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
 
@@ -736,7 +718,7 @@ static int create_device(struct zram *zram, int device_id)
 	snprintf(zram->disk->disk_name, 16, "zram%d", device_id);
 
 	/* Actual capacity set using syfs (/sys/block/zram<id>/disksize */
-	set_capacity(zram->disk, 0);
+	zram_set_disksize(zram, 0);
 
 	/*
 	 * To ensure that we always get PAGE_SIZE aligned
-- 
1.7.6

[PATCH 5/5] zram: Set initial disksize to some default value

[Nitin Gupta]

Currently, we set initial disksize as 0, which forces
user to write some value to corresponding zram device's
sysfs node, before the device can be used. Now, we avoid
this step by providing some default size initially.

To change the disksize, user must:
 - Reset disk.
	Ex: echo 1 > /sys/block/zram0/reset
(NOTE: disksize is set to the default value after reset)

 - Set new disksize.
	Ex: echo $((256*1024*1024)) > /sys/block/zram0/disksize

Signed-off-by: Nitin Gupta <ngupta@vflare.org>
Reviewed-by: Jerome Marchand <jmarchan@redhat.com>
---
 drivers/staging/zram/zram_drv.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index b1c4abd..9451d07 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -599,7 +599,7 @@ void zram_reset_device(struct zram *zram)
 	/* Reset stats */
 	memset(&zram->stats, 0, sizeof(zram->stats));
 
-	zram->disksize = 0;
+	zram_set_disksize(zram, zram_default_disksize_bytes());
 	mutex_unlock(&zram->init_lock);
 }
 
@@ -615,9 +615,6 @@ int zram_init_device(struct zram *zram)
 		return 0;
 	}
 
-	if (!zram->disksize)
-		zram_set_disksize(zram, zram_default_disksize_bytes());
-
 	zram->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
 	if (!zram->compress_workmem) {
 		pr_err("Error allocating compressor working memory!\n");
@@ -717,8 +714,12 @@ static int create_device(struct zram *zram, int device_id)
 	zram->disk->private_data = zram;
 	snprintf(zram->disk->disk_name, 16, "zram%d", device_id);
 
-	/* Actual capacity set using syfs (/sys/block/zram<id>/disksize */
-	zram_set_disksize(zram, 0);
+	/*
+	 * Set some default disksize. To set another disksize, user
+	 * must reset the device and then write a new disksize to
+	 * corresponding device's sysfs node.
+	 */
+	zram_set_disksize(zram, zram_default_disksize_bytes());
 
 	/*
 	 * To ensure that we always get PAGE_SIZE aligned
-- 
1.7.6

Re: [PATCH 2/5] zram: Kernel config option for number of devices

[Greg KH]

On Fri, Sep 09, 2011 at 07:01:01PM -0400, Nitin Gupta wrote:
> Allows configuring default number of zram devices
> as kernel config option. User can override this
> value using 'num_devices' module parameter.

Ick, another config value, is this really needed?  Why can't we rely on
the module parameter instead?


> 
> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
> ---
>  drivers/staging/zram/Kconfig    |    9 +++++++++
>  drivers/staging/zram/zram_drv.c |   13 ++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/zram/Kconfig b/drivers/staging/zram/Kconfig
> index 3bec4db..ca31cb3 100644
> --- a/drivers/staging/zram/Kconfig
> +++ b/drivers/staging/zram/Kconfig
> @@ -21,6 +21,15 @@ config ZRAM
>  	  See zram.txt for more information.
>  	  Project home: http://compcache.googlecode.com/
>  
> +config ZRAM_NUM_DEVICES
> +	int "Default number of zram devices"
> +	depends on ZRAM
> +	range 1 32
> +	default 1
> +	help
> +	  Select default number of zram devices. You can override this value
> +	  using 'num_devices' module parameter.
> +
>  config ZRAM_DEBUG
>  	bool "Compressed RAM block device debug support"
>  	depends on ZRAM
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 03ec5a2..55f2c0e 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -781,6 +781,14 @@ static int __init zram_init(void)
>  {
>  	int ret, dev_id;
>  
> +	/*
> +	 * Module parameter not specified by user. Use default
> +	 * value as defined during kernel config.
> +	 */
> +	if (num_devices == 0) {
> +		num_devices = CONFIG_ZRAM_NUM_DEVICES;
> +	}

Wrong coding style, which, for some odd reason, checkpatch doesn't
catch.  Please remove the braces.

thanks,

greg k-h

Re: [PATCH 3/5] zram: Make gobal variables use unique names

[Greg KH]

On Fri, Sep 09, 2011 at 07:01:02PM -0400, Nitin Gupta wrote:
> Global variables 'num_devices' and 'devices' are too
> general to be global. This patch switches the name to
> be "zram_devices".
> 
> Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
> Signed-off-by: Nitin Gupta <ngupta@vflare.org>

I've already applied this patch, right?

confused,

greg k-h

Re: [PATCH 4/5] zram: Simplify zram disk resizing interface

[Greg KH]

"simplify" it in what way?

What just got changed, and why, and did it just break any documentation?


On Fri, Sep 09, 2011 at 07:01:03PM -0400, Nitin Gupta wrote:
> Also remove unnecessary messages.
> 
> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
> Reviewed-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  drivers/staging/zram/zram_drv.c |   42 +++++++++++---------------------------
>  1 files changed, 12 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index b2e29dd..b1c4abd 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -104,33 +104,16 @@ static int page_zero_filled(void *ptr)
>  	return 1;
>  }
>  
> -static void zram_set_disksize(struct zram *zram, size_t totalram_bytes)
> +static u64 zram_default_disksize_bytes(void)
>  {
> -	if (!zram->disksize) {
> -		pr_info(
> -		"disk size not provided. You can use disksize_kb module "
> -		"param to specify size.\nUsing default: (%u%% of RAM).\n",
> -		default_disksize_perc_ram
> -		);
> -		zram->disksize = default_disksize_perc_ram *
> -					(totalram_bytes / 100);
> -	}
> -
> -	if (zram->disksize > 2 * (totalram_bytes)) {
> -		pr_info(
> -		"There is little point creating a zram of greater than "
> -		"twice the size of memory since we expect a 2:1 compression "
> -		"ratio. Note that zram uses about 0.1%% of the size of "
> -		"the disk when not in use so a huge zram is "
> -		"wasteful.\n"
> -		"\tMemory Size: %zu kB\n"
> -		"\tSize you selected: %llu kB\n"
> -		"Continuing anyway ...\n",
> -		totalram_bytes >> 10, zram->disksize
> -		);
> -	}

Why are these print messages removed, aren't they useful to people?

greg k-h

Re: [PATCH 5/5] zram: Set initial disksize to some default value

[Greg KH]

On Fri, Sep 09, 2011 at 07:01:04PM -0400, Nitin Gupta wrote:
> Currently, we set initial disksize as 0, which forces
> user to write some value to corresponding zram device's
> sysfs node, before the device can be used. Now, we avoid
> this step by providing some default size initially.
> 
> To change the disksize, user must:
>  - Reset disk.
> 	Ex: echo 1 > /sys/block/zram0/reset
> (NOTE: disksize is set to the default value after reset)
> 
>  - Set new disksize.
> 	Ex: echo $((256*1024*1024)) > /sys/block/zram0/disksize

So, what tools just broke with this change?

And where is the sysfs file documentation change that should go along
with this?

greg k-h

Re: [PATCH 4/5] zram: Simplify zram disk resizing interface

[Nitin Gupta]

On 09/09/2011 07:11 PM, Greg KH wrote:

> "simplify" it in what way?
> 
> What just got changed, and why, and did it just break any documentation?
> 


Its simplification in the sense that earlier we had to be pass total RAM
size in bytes when setting zram disksize using zram_set_disksize()
which is not intuitive.  The replaced version instead just requires
the new size to be set which seems to make more sense.

> 
> On Fri, Sep 09, 2011 at 07:01:03PM -0400, Nitin Gupta wrote:
>> Also remove unnecessary messages.
>>
>> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
>> Reviewed-by: Jerome Marchand <jmarchan@redhat.com>
>> ---
>>  drivers/staging/zram/zram_drv.c |   42 +++++++++++---------------------------
>>  1 files changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>> index b2e29dd..b1c4abd 100644
>> --- a/drivers/staging/zram/zram_drv.c
>> +++ b/drivers/staging/zram/zram_drv.c
>> @@ -104,33 +104,16 @@ static int page_zero_filled(void *ptr)
>>  	return 1;
>>  }
>>  
>> -static void zram_set_disksize(struct zram *zram, size_t totalram_bytes)
>> +static u64 zram_default_disksize_bytes(void)
>>  {
>> -	if (!zram->disksize) {
>> -		pr_info(
>> -		"disk size not provided. You can use disksize_kb module "
>> -		"param to specify size.\nUsing default: (%u%% of RAM).\n",
>> -		default_disksize_perc_ram
>> -		);
>> -		zram->disksize = default_disksize_perc_ram *
>> -					(totalram_bytes / 100);
>> -	}
>> -
>> -	if (zram->disksize > 2 * (totalram_bytes)) {
>> -		pr_info(
>> -		"There is little point creating a zram of greater than "
>> -		"twice the size of memory since we expect a 2:1 compression "
>> -		"ratio. Note that zram uses about 0.1%% of the size of "
>> -		"the disk when not in use so a huge zram is "
>> -		"wasteful.\n"
>> -		"\tMemory Size: %zu kB\n"
>> -		"\tSize you selected: %llu kB\n"
>> -		"Continuing anyway ...\n",
>> -		totalram_bytes >> 10, zram->disksize
>> -		);
>> -	}
> 
> Why are these print messages removed, aren't they useful to people?
> 


The first message is no longer required since we now set some default
value (25% of RAM) instead of zero. So, if user doesn't provide any
disksize value, we use the default.

The second message really doesn't belong in a kernel driver. That's more
of a documentation or user commonsense thing.  If a user really thinks
that the data to be written is going to be highly compressible, setting
zram to such large value makes some sense and there is little point in
throwing out this big warning in system logs.

Thanks,
Nitin

Re: [PATCH 5/5] zram: Set initial disksize to some default value

[Nitin Gupta]

On 09/09/2011 07:12 PM, Greg KH wrote:

> On Fri, Sep 09, 2011 at 07:01:04PM -0400, Nitin Gupta wrote:
>> Currently, we set initial disksize as 0, which forces
>> user to write some value to corresponding zram device's
>> sysfs node, before the device can be used. Now, we avoid
>> this step by providing some default size initially.
>>
>> To change the disksize, user must:
>>  - Reset disk.
>> 	Ex: echo 1 > /sys/block/zram0/reset
>> (NOTE: disksize is set to the default value after reset)
>>
>>  - Set new disksize.
>> 	Ex: echo $((256*1024*1024)) > /sys/block/zram0/disksize
> 
> So, what tools just broke with this change?
> 
> And where is the sysfs file documentation change that should go along
> with this?
> 


This change does not change any sysfs names or behavior, so does not
break any scripts that assume the current behavior. I also verified that
sysfs documentation (ABI/testing/sysfs-block-zram) need not be changed
and same with drivers/staging/zram/zram.txt.

PS: zram_set_disksize() change (patch 1/5) also does not break any
documentation.

Thanks,
Nitin

Re: [PATCH 3/5] zram: Make gobal variables use unique names

[Nitin Gupta]

On 09/09/2011 07:10 PM, Greg KH wrote:

> On Fri, Sep 09, 2011 at 07:01:02PM -0400, Nitin Gupta wrote:
>> Global variables 'num_devices' and 'devices' are too
>> general to be global. This patch switches the name to
>> be "zram_devices".
>>
>> Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
>> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
> 
> I've already applied this patch, right?
> 


I just checked another (staging-next) tree which is sync'ed more
recently and yes this patch has been applied.  Since git.kernel.org
seems to be down I couldn't see this change on my working tree.

However, that previous change actually also renamed module parameter
name 'num_devices' to 'zram_num_devices' which does not make much sense
since the boot parameter would now be 'zram.zram_num_devices='.  All
that's needed is to rename that global variable to some less generic
name but the boot/command line parameter name should be retained as
'zram.num_devices=' which is what this new patch does.

So, would it possible for you to back-out that original change and apply
this change instead. Or, should I send a new patch series that "fixes"
that previous change?

Nitin

Re: [PATCH 4/5] zram: Simplify zram disk resizing interface

[Greg KH]

On Fri, Sep 09, 2011 at 08:12:49PM -0400, Nitin Gupta wrote:
> On 09/09/2011 07:11 PM, Greg KH wrote:
> 
> > "simplify" it in what way?
> > 
> > What just got changed, and why, and did it just break any documentation?
> > 
> 
> 
> Its simplification in the sense that earlier we had to be pass total RAM
> size in bytes when setting zram disksize using zram_set_disksize()
> which is not intuitive.  The replaced version instead just requires
> the new size to be set which seems to make more sense.

Then why not mention that?

Is there some character limit in the changelog comment section that I
don't know about?  :)

Please, be much more descriptive so we have a chance to know what is
going on.

> The first message is no longer required since we now set some default
> value (25% of RAM) instead of zero. So, if user doesn't provide any
> disksize value, we use the default.
> 
> The second message really doesn't belong in a kernel driver. That's more
> of a documentation or user commonsense thing.  If a user really thinks
> that the data to be written is going to be highly compressible, setting
> zram to such large value makes some sense and there is little point in
> throwing out this big warning in system logs.

Again, put this in the changelog...

greg k-h

Re: [PATCH 5/5] zram: Set initial disksize to some default value

[Greg KH]

On Fri, Sep 09, 2011 at 08:18:56PM -0400, Nitin Gupta wrote:
> On 09/09/2011 07:12 PM, Greg KH wrote:
> 
> > On Fri, Sep 09, 2011 at 07:01:04PM -0400, Nitin Gupta wrote:
> >> Currently, we set initial disksize as 0, which forces
> >> user to write some value to corresponding zram device's
> >> sysfs node, before the device can be used. Now, we avoid
> >> this step by providing some default size initially.
> >>
> >> To change the disksize, user must:
> >>  - Reset disk.
> >> 	Ex: echo 1 > /sys/block/zram0/reset
> >> (NOTE: disksize is set to the default value after reset)
> >>
> >>  - Set new disksize.
> >> 	Ex: echo $((256*1024*1024)) > /sys/block/zram0/disksize
> > 
> > So, what tools just broke with this change?
> > 
> > And where is the sysfs file documentation change that should go along
> > with this?
> > 
> 
> 
> This change does not change any sysfs names or behavior, so does not
> break any scripts that assume the current behavior. I also verified that
> sysfs documentation (ABI/testing/sysfs-block-zram) need not be changed
> and same with drivers/staging/zram/zram.txt.

Huh, you just said the behavior changed as described in your comment
above.  You said the user no longer has to do a step to accomplish
something.  What happens if they do do that after this patch?

confused,

greg k-h

Re: [PATCH 3/5] zram: Make gobal variables use unique names

[Greg KH]

On Fri, Sep 09, 2011 at 08:35:54PM -0400, Nitin Gupta wrote:
> On 09/09/2011 07:10 PM, Greg KH wrote:
> 
> > On Fri, Sep 09, 2011 at 07:01:02PM -0400, Nitin Gupta wrote:
> >> Global variables 'num_devices' and 'devices' are too
> >> general to be global. This patch switches the name to
> >> be "zram_devices".
> >>
> >> Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
> >> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
> > 
> > I've already applied this patch, right?
> > 
> 
> 
> I just checked another (staging-next) tree which is sync'ed more
> recently and yes this patch has been applied.  Since git.kernel.org
> seems to be down I couldn't see this change on my working tree.
> 
> However, that previous change actually also renamed module parameter
> name 'num_devices' to 'zram_num_devices' which does not make much sense
> since the boot parameter would now be 'zram.zram_num_devices='.  All
> that's needed is to rename that global variable to some less generic
> name but the boot/command line parameter name should be retained as
> 'zram.num_devices=' which is what this new patch does.
> 
> So, would it possible for you to back-out that original change and apply
> this change instead. Or, should I send a new patch series that "fixes"
> that previous change?

Send a patch that "fixes" it.

thanks,

greg k-h

Re: [PATCH 5/5] zram: Set initial disksize to some default value

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On Fri, 09 Sep 2011 20:18:56 EDT, Nitin Gupta said:
> On 09/09/2011 07:12 PM, Greg KH wrote:
> 
> > On Fri, Sep 09, 2011 at 07:01:04PM -0400, Nitin Gupta wrote:
> >> Currently, we set initial disksize as 0, which forces
> >> user to write some value to corresponding zram device's
> >> sysfs node, before the device can be used. Now, we avoid
> >> this step by providing some default size initially.
> >>
> >> To change the disksize, user must:
> >>  - Reset disk.
> >> 	Ex: echo 1 > /sys/block/zram0/reset
> >> (NOTE: disksize is set to the default value after reset)
> >>
> >>  - Set new disksize.
> >> 	Ex: echo $((256*1024*1024)) > /sys/block/zram0/disksize
> > 
> > So, what tools just broke with this change?
> > 
> > And where is the sysfs file documentation change that should go along
> > with this?
> > 
> 
> 
> This change does not change any sysfs names or behavior, so does not
> break any scripts that assume the current behavior.

Consider the current case:   We init size to zero, and a startup script changes
it to 1024 and then sticks 768 of data in there.  No Problem.

We now get a kernel that's built with a default of 512.  The startup script
tries to change it to 1024, but doesn't know it has to splat something into
'reset' for it to actually work.  It then tries to stick 768 of data in there,
and hilarity ensues...

If I haven't misunderstood...


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 5/5] zram: Set initial disksize to some default value

[Nitin Gupta]

On 09/09/2011 08:50 PM, Valdis.Kletnieks@vt.edu wrote:

> On Fri, 09 Sep 2011 20:18:56 EDT, Nitin Gupta said:
>> On 09/09/2011 07:12 PM, Greg KH wrote:
>>
>>> On Fri, Sep 09, 2011 at 07:01:04PM -0400, Nitin Gupta wrote:
>>>> Currently, we set initial disksize as 0, which forces
>>>> user to write some value to corresponding zram device's
>>>> sysfs node, before the device can be used. Now, we avoid
>>>> this step by providing some default size initially.
>>>>
>>>> To change the disksize, user must:
>>>>  - Reset disk.
>>>> 	Ex: echo 1 > /sys/block/zram0/reset
>>>> (NOTE: disksize is set to the default value after reset)
>>>>
>>>>  - Set new disksize.
>>>> 	Ex: echo $((256*1024*1024)) > /sys/block/zram0/disksize
>>>
>>> So, what tools just broke with this change?
>>>
>>> And where is the sysfs file documentation change that should go along
>>> with this?
>>>
>>
>>
>> This change does not change any sysfs names or behavior, so does not
>> break any scripts that assume the current behavior.
> 
> Consider the current case:   We init size to zero, and a startup script changes
> it to 1024 and then sticks 768 of data in there.  No Problem.
> 
> We now get a kernel that's built with a default of 512.  The startup script
> tries to change it to 1024, but doesn't know it has to splat something into
> 'reset' for it to actually work.  It then tries to stick 768 of data in there,
> and hilarity ensues...
> 
> If I haven't misunderstood...
> 


Setting default size of say 512 doesn't make the device initialized.
The initialization happens only when user issues any I/O to the device
and only after initialization the disksize cannot be changed.
So, when user writes say 768 as new disksize in uninitialized state, the
new disksize will take effect and there will be no surprises.

Device reset is needed only when user wants to discard existing data and
bring device to uninitialized state again, after which disksize can be
changed again.

Nitin

Re: [PATCH 2/5] zram: Kernel config option for number of devices

[Nitin Gupta]

On 09/09/2011 07:09 PM, Greg KH wrote:

> On Fri, Sep 09, 2011 at 07:01:01PM -0400, Nitin Gupta wrote:
>> Allows configuring default number of zram devices
>> as kernel config option. User can override this
>> value using 'num_devices' module parameter.
> 
> Ick, another config value, is this really needed?  Why can't we rely on
> the module parameter instead?
> 
> 


It is just an added convenience for users that tend to always use more
than one device. So, a distribution may set it to say 4 and users won't
have to go through any documentation just to create an additional zram
device.

This also makes its behavior similar to the usual ramdisk which also
accepts default number of ramdisks (BLK_DEV_RAM_COUNT) as kconfig option.

>>
>> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
>> ---
>>  drivers/staging/zram/Kconfig    |    9 +++++++++
>>  drivers/staging/zram/zram_drv.c |   13 ++++++++-----
>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/zram/Kconfig b/drivers/staging/zram/Kconfig
>> index 3bec4db..ca31cb3 100644
>> --- a/drivers/staging/zram/Kconfig
>> +++ b/drivers/staging/zram/Kconfig
>> @@ -21,6 +21,15 @@ config ZRAM
>>  	  See zram.txt for more information.
>>  	  Project home: http://compcache.googlecode.com/
>>  
>> +config ZRAM_NUM_DEVICES
>> +	int "Default number of zram devices"
>> +	depends on ZRAM
>> +	range 1 32
>> +	default 1
>> +	help
>> +	  Select default number of zram devices. You can override this value
>> +	  using 'num_devices' module parameter.
>> +
>>  config ZRAM_DEBUG
>>  	bool "Compressed RAM block device debug support"
>>  	depends on ZRAM
>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>> index 03ec5a2..55f2c0e 100644
>> --- a/drivers/staging/zram/zram_drv.c
>> +++ b/drivers/staging/zram/zram_drv.c
>> @@ -781,6 +781,14 @@ static int __init zram_init(void)
>>  {
>>  	int ret, dev_id;
>>  
>> +	/*
>> +	 * Module parameter not specified by user. Use default
>> +	 * value as defined during kernel config.
>> +	 */
>> +	if (num_devices == 0) {
>> +		num_devices = CONFIG_ZRAM_NUM_DEVICES;
>> +	}
> 
> Wrong coding style, which, for some odd reason, checkpatch doesn't
> catch.  Please remove the braces.
> 


Ok, will correct this.

Thanks,
Nitin

Re: [PATCH 5/5] zram: Set initial disksize to some default value

[Nitin Gupta]

On 09/09/2011 08:42 PM, Greg KH wrote:

> On Fri, Sep 09, 2011 at 08:18:56PM -0400, Nitin Gupta wrote:
>> On 09/09/2011 07:12 PM, Greg KH wrote:
>>
>>> On Fri, Sep 09, 2011 at 07:01:04PM -0400, Nitin Gupta wrote:
>>>> Currently, we set initial disksize as 0, which forces
>>>> user to write some value to corresponding zram device's
>>>> sysfs node, before the device can be used. Now, we avoid
>>>> this step by providing some default size initially.
>>>>
>>>> To change the disksize, user must:
>>>>  - Reset disk.
>>>> 	Ex: echo 1 > /sys/block/zram0/reset
>>>> (NOTE: disksize is set to the default value after reset)
>>>>
>>>>  - Set new disksize.
>>>> 	Ex: echo $((256*1024*1024)) > /sys/block/zram0/disksize
>>>
>>> So, what tools just broke with this change?
>>>
>>> And where is the sysfs file documentation change that should go along
>>> with this?
>>>
>>
>>
>> This change does not change any sysfs names or behavior, so does not
>> break any scripts that assume the current behavior. I also verified that
>> sysfs documentation (ABI/testing/sysfs-block-zram) need not be changed
>> and same with drivers/staging/zram/zram.txt.
> 
> Huh, you just said the behavior changed as described in your comment
> above.  You said the user no longer has to do a step to accomplish
> something.  What happens if they do do that after this patch?
> 


The current documentation states that:

  "" (from drivers/staging/zram/zram.txt)
2) Set Disksize (Optional):
        Set disk size by writing the value to sysfs node 'disksize'
        (in bytes). If disksize is not given, default value of 25%
        of RAM is used.
  ""

So existing code did not correctly implement this behavior -- user had
to set a disksize for zram to be usable. With this patch, the code now
behaves exactly as described in documentation. So, we now avoid the
extra step of setting a disksize as documented.  The user can still, of
course, set another disk value if needed.

So, in summary, no documentation changes are required with this change.
I will also add this explanation in changelog when submitting v4 patches.

Thanks,
Nitin