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

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

[Nitin Gupta]

(applies to staging-next)

Changes since 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):
  Fix sparse warnings
  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   |   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] Fix sparse warnings

[Nitin Gupta]

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

Re: [PATCH 1/5] Fix sparse warnings

[Jerome Marchand]

On 09/08/2011 03:29 AM, Nitin Gupta wrote:
> ---

I'm afraid this patch is missing a description.

Jerome

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

Re: [PATCH 1/5] Fix sparse warnings

[Nitin Gupta]

On 09/08/2011 04:17 AM, Jerome Marchand wrote:
> On 09/08/2011 03:29 AM, Nitin Gupta wrote:
>> ---
>
> I'm afraid this patch is missing a description.
>

Since it just fixes couple of trivial sparse warnings,
I though no description is necessary? Anyways, here is
the description if needed for patch process:

Fixed some sparse warnings by making zram_slot_free_notify()
static and max_zpage_size as size_t.

---
Thanks,
Nitin

Re: [PATCH 1/5] Fix sparse warnings

[Harvey Harrison]

On Thu, Sep 8, 2011 at 8:13 AM, Nitin Gupta <ngupta@vflare.org> wrote:
> On 09/08/2011 04:17 AM, Jerome Marchand wrote:
>
> Since it just fixes couple of trivial sparse warnings,
> I though no description is necessary? Anyways, here is
> the description if needed for patch process:
>
> Fixed some sparse warnings by making zram_slot_free_notify()
> static and max_zpage_size as size_t.
>

Please just paste the exact sparse warnings you fixed with this patch,
then it will be more
understandable _why_ you made the above changes.

Harvey

Re: [PATCH 1/5] Fix sparse warnings

[Nitin Gupta]

On 09/08/2011 01:32 PM, Harvey Harrison wrote:

> On Thu, Sep 8, 2011 at 8:13 AM, Nitin Gupta <ngupta@vflare.org> wrote:
>> On 09/08/2011 04:17 AM, Jerome Marchand wrote:
>>
>> Since it just fixes couple of trivial sparse warnings,
>> I though no description is necessary? Anyways, here is
>> the description if needed for patch process:
>>
>> Fixed some sparse warnings by making zram_slot_free_notify()
>> static and max_zpage_size as size_t.
>>
> 
> Please just paste the exact sparse warnings you fixed with this patch,
> then it will be more
> understandable _why_ you made the above changes.
> 



new description:

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.

Thanks,
Nitin

Re: [PATCH 1/5] Fix sparse warnings

[Dan Carpenter]

Just resend the whole patch.

When you're applying patches from mutt normally you have a macro set
up to automatically run git-am.  Under your system maintainers have
to manually fix your patch and it breaks the work flow.

regards,
dan carpenter

Re: [PATCH 1/5 resend] 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

Re: [PATCH 1/5 resend] Fix sparse warnings

[Greg KH]

On Fri, Sep 09, 2011 at 01:19:35PM -0400, Nitin Gupta wrote:
> 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(-)

Close, but can you please resend the series with a proper Subject: line
that shows what subsystem/driver this is for?  I explained how to do
this properly in a prior email.

thanks,

greg k-h

[PATCH 2/5] 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] 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] 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] 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