Re: [PATCH v3 3/4] zram: get rid of lockdep warning
From: Jerome Marchand
Date: Mon Jan 21 2013 - 14:48:37 EST
On 01/21/2013 06:21 AM, Minchan Kim wrote:
> Lockdep complains about recursive deadlock of zram->init_lock.
> [1] made it false positive because we can't request IO to zram
> before setting disksize. Anyway, we should shut lockdep up to
> avoid many reporting from user.
>
> Cc: Jerome Marchand <jmarchan@xxxxxxxxxx>
> Cc: Nitin Gupta <ngupta@xxxxxxxxxx>
> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> ---
> drivers/staging/zram/zram_drv.c | 115 +++++++++++++++++++------------------
> drivers/staging/zram/zram_drv.h | 12 +++-
> drivers/staging/zram/zram_sysfs.c | 10 +++-
> 3 files changed, 79 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index e95e37c..1f6938a 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -462,19 +462,12 @@ error:
> void __zram_reset_device(struct zram *zram)
> {
> size_t index;
> + struct zram_meta meta;
>
> if (!zram->init_done)
> goto out;
>
> zram->init_done = 0;
> -
> - /* Free various per-device buffers */
> - kfree(zram->compress_workmem);
> - free_pages((unsigned long)zram->compress_buffer, 1);
> -
> - zram->compress_workmem = NULL;
> - zram->compress_buffer = NULL;
> -
> /* Free all pages that are still in this zram device */
> for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
> unsigned long handle = zram->table[index].handle;
> @@ -484,11 +477,11 @@ void __zram_reset_device(struct zram *zram)
> zs_free(zram->mem_pool, handle);
> }
>
> - vfree(zram->table);
> - zram->table = NULL;
> -
> - zs_destroy_pool(zram->mem_pool);
> - zram->mem_pool = NULL;
> + meta.compress_workmem = zram->compress_workmem;
> + meta.compress_buffer = zram->compress_buffer;
> + meta.table = zram->table;
> + meta.mem_pool = zram->mem_pool;
> + zram_meta_free(&meta);
>
> /* Reset stats */
> memset(&zram->stats, 0, sizeof(zram->stats));
> @@ -505,12 +498,59 @@ void zram_reset_device(struct zram *zram)
> up_write(&zram->init_lock);
> }
>
> -/* zram->init_lock should be held */
> -int zram_init_device(struct zram *zram)
> +void zram_meta_free(struct zram_meta *meta)
> +{
> + zs_destroy_pool(meta->mem_pool);
> + kfree(meta->compress_workmem);
> + free_pages((unsigned long)meta->compress_buffer, 1);
> + vfree(meta->table);
> + kfree(meta);
At all callsite, meta is a local variable. We don't want to free it.
> +}
> +
> +int zram_meta_alloc(struct zram_meta *meta, u64 disksize)
> {
> - int ret;
> size_t num_pages;
>
> + meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> + if (!meta->compress_workmem) {
> + pr_err("Error allocating compressor working memory!\n");
> + goto out;
> + }
> +
> + meta->compress_buffer =
> + (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO, 1);
> + if (!meta->compress_buffer) {
> + pr_err("Error allocating compressor buffer space\n");
> + goto free_workmem;
> + }
> +
> + num_pages = disksize >> PAGE_SHIFT;
> + meta->table = vzalloc(num_pages * sizeof(*meta->table));
> + if (!meta->table) {
> + pr_err("Error allocating zram address table\n");
> + goto free_buffer;
> + }
> +
> + meta->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM);
> + if (!meta->mem_pool) {
> + pr_err("Error creating memory pool\n");
> + goto free_table;
> + }
> +
> + return 0;
> +
> +free_table:
> + vfree(meta->table);
> +free_buffer:
> + free_pages((unsigned long)meta->compress_buffer, 1);
> +free_workmem:
> + kfree(meta->compress_workmem);
> +out:
> + return -ENOMEM;
> +}
> +
> +void zram_init_device(struct zram *zram, struct zram_meta *meta)
> +{
> if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> pr_info(
> "There is little point creating a zram of greater than "
> @@ -525,51 +565,16 @@ int zram_init_device(struct zram *zram)
> );
> }
>
> - zram->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> - if (!zram->compress_workmem) {
> - pr_err("Error allocating compressor working memory!\n");
> - ret = -ENOMEM;
> - goto fail_no_table;
> - }
> -
> - zram->compress_buffer =
> - (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
> - if (!zram->compress_buffer) {
> - pr_err("Error allocating compressor buffer space\n");
> - ret = -ENOMEM;
> - goto fail_no_table;
> - }
> -
> - num_pages = zram->disksize >> PAGE_SHIFT;
> - zram->table = vzalloc(num_pages * sizeof(*zram->table));
> - if (!zram->table) {
> - pr_err("Error allocating zram address table\n");
> - ret = -ENOMEM;
> - goto fail_no_table;
> - }
> -
> /* zram devices sort of resembles non-rotational disks */
> queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
>
> - zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM);
> - if (!zram->mem_pool) {
> - pr_err("Error creating memory pool\n");
> - ret = -ENOMEM;
> - goto fail;
> - }
> -
> + zram->mem_pool = meta->mem_pool;
> + zram->compress_workmem = meta->compress_workmem;
> + zram->compress_buffer = meta->compress_buffer;
> + zram->table = meta->table;
> zram->init_done = 1;
>
> pr_debug("Initialization done!\n");
> - return 0;
> -
> -fail_no_table:
> - /* To prevent accessing table entries during cleanup */
> - zram->disksize = 0;
> -fail:
> - __zram_reset_device(zram);
Here goes the last call to __zram_reset_device() outside of zram_reset_device().
I guess we should get rid of it.
Jerome
> - pr_err("Initialization failed: err=%d\n", ret);
> - return ret;
> }
>
> static void zram_slot_free_notify(struct block_device *bdev,
> diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
> index 5b671d1..fbe7db0 100644
> --- a/drivers/staging/zram/zram_drv.h
> +++ b/drivers/staging/zram/zram_drv.h
> @@ -83,11 +83,19 @@ struct zram_stats {
> u32 bad_compress; /* % of pages with compression ratio>=75% */
> };
>
> +struct zram_meta {
> + void *compress_workmem;
> + void *compress_buffer;
> + struct table *table;
> + struct zs_pool *mem_pool;
> +};
> +
> struct zram {
> struct zs_pool *mem_pool;
> void *compress_workmem;
> void *compress_buffer;
> struct table *table;
> +
> spinlock_t stat64_lock; /* protect 64-bit stats */
> struct rw_semaphore lock; /* protect compression buffers and table
> * against concurrent read and writes */
> @@ -111,7 +119,9 @@ unsigned int zram_get_num_devices(void);
> extern struct attribute_group zram_disk_attr_group;
> #endif
>
> -extern int zram_init_device(struct zram *zram);
> extern void zram_reset_device(struct zram *zram);
> +extern int zram_meta_alloc(struct zram_meta *meta, u64 disksize);
> +extern void zram_meta_free(struct zram_meta *meta);
> +extern void zram_init_device(struct zram *zram, struct zram_meta *meta);
>
> #endif
> diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
> index 369db12..31433ef 100644
> --- a/drivers/staging/zram/zram_sysfs.c
> +++ b/drivers/staging/zram/zram_sysfs.c
> @@ -56,22 +56,28 @@ static ssize_t disksize_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t len)
> {
> u64 disksize;
> + struct zram_meta meta;
> struct zram *zram = dev_to_zram(dev);
>
> disksize = memparse(buf, NULL);
> if (!disksize)
> return -EINVAL;
>
> + disksize = PAGE_ALIGN(disksize);
> + if (zram_meta_alloc(&meta, disksize))
> + return -ENOMEM;
> +
> down_write(&zram->init_lock);
> if (zram->init_done) {
> up_write(&zram->init_lock);
> + zram_meta_free(&meta);
> pr_info("Cannot change disksize for initialized device\n");
> return -EBUSY;
> }
>
> - zram->disksize = PAGE_ALIGN(disksize);
> + zram->disksize = disksize;
> set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> - zram_init_device(zram);
> + zram_init_device(zram, &meta);
> up_write(&zram->init_lock);
>
> return len;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/