[PATCH 0/4] zram: minor features and cleanups

[PATCH 0/4] zram: minor features and cleanups

[Nitin Gupta]

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

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

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

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

-- 
1.7.6

[PATCH 1/4] 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 d70ec1a..207234e 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -780,6 +780,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);
@@ -794,11 +802,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

Re: [PATCH 1/4] Kernel config option for number of devices

[Greg KH]

On Wed, Aug 24, 2011 at 08:34:47AM +0700, Nitin Gupta wrote:
> 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(-)

This patch fails to apply to my staging-next branch:

	patching file drivers/staging/zram/Kconfig
	patching file drivers/staging/zram/zram_drv.c
	Hunk #1 FAILED at 780.
	Hunk #2 FAILED at 794.
	2 out of 2 hunks FAILED -- saving rejects to file drivers/staging/zram/zram_drv.c.rej

Care to refresh all 4 patches here against that branch and resend them
to me so that I can apply them?

thanks,

greg k-h

[PATCH 2/4] 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 207234e..81d6c43 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)
 {
@@ -784,13 +784,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;
 	}
@@ -803,15 +802,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;
 	}
@@ -820,8 +820,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:
@@ -833,8 +833,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)
@@ -843,11 +843,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 abe5221..59b01d6 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 3/4] Simplify zram disk resizing interface

[Nitin Gupta]

Also remove unnecessary messages.

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 81d6c43..d7fb207 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);
 
@@ -735,7 +717,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
diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
index 59b01d6..83e774c 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 u64 max_zpage_size = PAGE_SIZE / 4 * 3;
 
 /*
  * NOTE: max_zpage_size must be less than or equal to:
-- 
1.7.6

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

[Jerome Marchand]

On 08/24/2011 03:34 AM, Nitin Gupta wrote:
> Also remove unnecessary messages.
> 
> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
> ---
>  drivers/staging/zram/zram_drv.c |   42 +++++++++++---------------------------
>  drivers/staging/zram/zram_drv.h |    2 +-
>  2 files changed, 13 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 81d6c43..d7fb207 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());

With your next patch, this will not happen anymore, unless someone explicitly sets
the disk size to zero. If zero means default, it should be documented. It looks weird
anyway: if something like that should be done, it probably should be done in
disksize_store() for clarity.
Otherwise, your next patch should remove this chunk of code.

Jerome

>  
>  	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);
>  
> @@ -735,7 +717,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
> diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
> index 59b01d6..83e774c 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 u64 max_zpage_size = PAGE_SIZE / 4 * 3;
>  
>  /*
>   * NOTE: max_zpage_size must be less than or equal to:

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

[Nitin Gupta]

On 08/30/2011 06:11 AM, Jerome Marchand wrote:
> On 08/24/2011 03:34 AM, Nitin Gupta wrote:
>> Also remove unnecessary messages.
>>
>> Signed-off-by: Nitin Gupta<ngupta@vflare.org>
>> ---
>>   drivers/staging/zram/zram_drv.c |   42 +++++++++++---------------------------
>>   drivers/staging/zram/zram_drv.h |    2 +-
>>   2 files changed, 13 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>> index 81d6c43..d7fb207 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());
>
> With your next patch, this will not happen anymore, unless someone explicitly sets
> the disk size to zero. If zero means default, it should be documented. It looks weird
> anyway: if something like that should be done, it probably should be done in
> disksize_store() for clarity.
> Otherwise, your next patch should remove this chunk of code.
>

Thanks for the review.  I have now removed that check in v2 patches 
since we now set some default value during initialization.

Thanks,
Nitin

[PATCH 4/4] 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>
---
 drivers/staging/zram/zram_drv.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index d7fb207..2489f07 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);
 }
 
@@ -716,8 +716,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