* [PATCH 1/5] zram: Fix sparse warnings
2011-09-09 23:00 [PATCH 0/5 v3] zram: minor cleanups and features Nitin Gupta
@ 2011-09-09 23:01 ` Nitin Gupta
2011-09-09 23:01 ` [PATCH 2/5] zram: Kernel config option for number of devices Nitin Gupta
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ messages in thread
From: Nitin Gupta @ 2011-09-09 23:01 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 2/5] zram: Kernel config option for number of devices
2011-09-09 23:00 [PATCH 0/5 v3] zram: minor cleanups and features Nitin Gupta
2011-09-09 23:01 ` [PATCH 1/5] zram: Fix sparse warnings Nitin Gupta
@ 2011-09-09 23:01 ` Nitin Gupta
2011-09-09 23:09 ` Greg KH
2011-09-09 23:01 ` [PATCH 3/5] zram: Make gobal variables use unique names Nitin Gupta
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Nitin Gupta @ 2011-09-09 23:01 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] zram: Kernel config option for number of devices
2011-09-09 23:01 ` [PATCH 2/5] zram: Kernel config option for number of devices Nitin Gupta
@ 2011-09-09 23:09 ` Greg KH
2011-09-10 2:15 ` Nitin Gupta
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2011-09-09 23:09 UTC (permalink / raw)
To: Nitin Gupta
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] zram: Kernel config option for number of devices
2011-09-09 23:09 ` Greg KH
@ 2011-09-10 2:15 ` Nitin Gupta
0 siblings, 0 replies; 20+ messages in thread
From: Nitin Gupta @ 2011-09-10 2:15 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] zram: Make gobal variables use unique names
2011-09-09 23:00 [PATCH 0/5 v3] zram: minor cleanups and features Nitin Gupta
2011-09-09 23:01 ` [PATCH 1/5] zram: Fix sparse warnings Nitin Gupta
2011-09-09 23:01 ` [PATCH 2/5] zram: Kernel config option for number of devices Nitin Gupta
@ 2011-09-09 23:01 ` Nitin Gupta
2011-09-09 23:10 ` Greg KH
2011-09-09 23:01 ` [PATCH 4/5] zram: Simplify zram disk resizing interface Nitin Gupta
2011-09-09 23:01 ` [PATCH 5/5] zram: Set initial disksize to some default value Nitin Gupta
4 siblings, 1 reply; 20+ messages in thread
From: Nitin Gupta @ 2011-09-09 23:01 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel, Noah Watkins
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/5] zram: Make gobal variables use unique names
2011-09-09 23:01 ` [PATCH 3/5] zram: Make gobal variables use unique names Nitin Gupta
@ 2011-09-09 23:10 ` Greg KH
2011-09-10 0:35 ` Nitin Gupta
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2011-09-09 23:10 UTC (permalink / raw)
To: Nitin Gupta
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel, Noah Watkins
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] zram: Make gobal variables use unique names
2011-09-09 23:10 ` Greg KH
@ 2011-09-10 0:35 ` Nitin Gupta
2011-09-10 0:45 ` Greg KH
0 siblings, 1 reply; 20+ messages in thread
From: Nitin Gupta @ 2011-09-10 0:35 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel, Noah Watkins
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] zram: Make gobal variables use unique names
2011-09-10 0:35 ` Nitin Gupta
@ 2011-09-10 0:45 ` Greg KH
0 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2011-09-10 0:45 UTC (permalink / raw)
To: Nitin Gupta
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel, Noah Watkins
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/5] zram: Simplify zram disk resizing interface
2011-09-09 23:00 [PATCH 0/5 v3] zram: minor cleanups and features Nitin Gupta
` (2 preceding siblings ...)
2011-09-09 23:01 ` [PATCH 3/5] zram: Make gobal variables use unique names Nitin Gupta
@ 2011-09-09 23:01 ` Nitin Gupta
2011-09-09 23:11 ` Greg KH
2011-09-09 23:01 ` [PATCH 5/5] zram: Set initial disksize to some default value Nitin Gupta
4 siblings, 1 reply; 20+ messages in thread
From: Nitin Gupta @ 2011-09-09 23:01 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] zram: Simplify zram disk resizing interface
2011-09-09 23:01 ` [PATCH 4/5] zram: Simplify zram disk resizing interface Nitin Gupta
@ 2011-09-09 23:11 ` Greg KH
2011-09-10 0:12 ` Nitin Gupta
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2011-09-09 23:11 UTC (permalink / raw)
To: Nitin Gupta
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
"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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] zram: Simplify zram disk resizing interface
2011-09-09 23:11 ` Greg KH
@ 2011-09-10 0:12 ` Nitin Gupta
2011-09-10 0:41 ` Greg KH
0 siblings, 1 reply; 20+ messages in thread
From: Nitin Gupta @ 2011-09-10 0:12 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] zram: Simplify zram disk resizing interface
2011-09-10 0:12 ` Nitin Gupta
@ 2011-09-10 0:41 ` Greg KH
0 siblings, 0 replies; 20+ messages in thread
From: Greg KH @ 2011-09-10 0:41 UTC (permalink / raw)
To: Nitin Gupta
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] zram: Set initial disksize to some default value
2011-09-09 23:00 [PATCH 0/5 v3] zram: minor cleanups and features Nitin Gupta
` (3 preceding siblings ...)
2011-09-09 23:01 ` [PATCH 4/5] zram: Simplify zram disk resizing interface Nitin Gupta
@ 2011-09-09 23:01 ` Nitin Gupta
2011-09-09 23:12 ` Greg KH
4 siblings, 1 reply; 20+ messages in thread
From: Nitin Gupta @ 2011-09-09 23:01 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] zram: Set initial disksize to some default value
2011-09-09 23:01 ` [PATCH 5/5] zram: Set initial disksize to some default value Nitin Gupta
@ 2011-09-09 23:12 ` Greg KH
2011-09-10 0:18 ` Nitin Gupta
0 siblings, 1 reply; 20+ messages in thread
From: Greg KH @ 2011-09-09 23:12 UTC (permalink / raw)
To: Nitin Gupta
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] zram: Set initial disksize to some default value
2011-09-09 23:12 ` Greg KH
@ 2011-09-10 0:18 ` Nitin Gupta
2011-09-10 0:42 ` Greg KH
2011-09-10 0:50 ` Valdis.Kletnieks
0 siblings, 2 replies; 20+ messages in thread
From: Nitin Gupta @ 2011-09-10 0:18 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] zram: Set initial disksize to some default value
2011-09-10 0:18 ` Nitin Gupta
@ 2011-09-10 0:42 ` Greg KH
2011-09-10 14:51 ` Nitin Gupta
2011-09-10 0:50 ` Valdis.Kletnieks
1 sibling, 1 reply; 20+ messages in thread
From: Greg KH @ 2011-09-10 0:42 UTC (permalink / raw)
To: Nitin Gupta
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] zram: Set initial disksize to some default value
2011-09-10 0:42 ` Greg KH
@ 2011-09-10 14:51 ` Nitin Gupta
0 siblings, 0 replies; 20+ messages in thread
From: Nitin Gupta @ 2011-09-10 14:51 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] zram: Set initial disksize to some default value
2011-09-10 0:18 ` Nitin Gupta
2011-09-10 0:42 ` Greg KH
@ 2011-09-10 0:50 ` Valdis.Kletnieks
2011-09-10 2:04 ` Nitin Gupta
1 sibling, 1 reply; 20+ messages in thread
From: Valdis.Kletnieks @ 2011-09-10 0:50 UTC (permalink / raw)
To: Nitin Gupta
Cc: Greg KH, Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] zram: Set initial disksize to some default value
2011-09-10 0:50 ` Valdis.Kletnieks
@ 2011-09-10 2:04 ` Nitin Gupta
0 siblings, 0 replies; 20+ messages in thread
From: Nitin Gupta @ 2011-09-10 2:04 UTC (permalink / raw)
To: Valdis.Kletnieks
Cc: Greg KH, Jerome Marchand, Pekka Enberg, Robert Jennings,
Linux Driver Project, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread