From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752641AbdDDCbR (ORCPT <rfc822;w@1wt.eu>);
        Mon, 3 Apr 2017 22:31:17 -0400
Received: from mail-pg0-f65.google.com ([74.125.83.65]:36009 "EHLO
        mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752595AbdDDCbQ (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 3 Apr 2017 22:31:16 -0400
Date: Tue, 4 Apr 2017 11:31:15 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Minchan Kim <minchan@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>, kernel-team@lge.com
Subject: Re: [PATCH 4/5] zram: remove zram_meta structure
Message-ID: <20170404023115.GC475@jagdpanzerIV.localdomain>
References: <1491196653-7388-1-git-send-email-minchan@kernel.org>
 <1491196653-7388-5-git-send-email-minchan@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1491196653-7388-5-git-send-email-minchan@kernel.org>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On (04/03/17 14:17), Minchan Kim wrote:
[..]
> -static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
> +static bool zram_meta_alloc(struct zram *zram, u64 disksize)
>  {
>  	size_t num_pages;
> -	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
> -
> -	if (!meta)
> -		return NULL;
>  
>  	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 out_error;
> -	}
> +	zram->table = vzalloc(num_pages * sizeof(*zram->table));
> +	if (!zram->table)
> +		return false;
>  
> -	meta->mem_pool = zs_create_pool(pool_name);
> -	if (!meta->mem_pool) {
> -		pr_err("Error creating memory pool\n");
> -		goto out_error;
> +	zram->mem_pool = zs_create_pool(zram->disk->disk_name);
> +	if (!zram->mem_pool) {
> +		vfree(zram->table);
> +		return false;
>  	}
>  
> -	return meta;
> -
> -out_error:
> -	vfree(meta->table);
> -	kfree(meta);
> -	return NULL;
> +	return true;
>  }

[..]

> @@ -1020,7 +989,6 @@ static ssize_t disksize_store(struct device *dev,
>  {
>  	u64 disksize;
>  	struct zcomp *comp;
> -	struct zram_meta *meta;
>  	struct zram *zram = dev_to_zram(dev);
>  	int err;
>  
> @@ -1029,8 +997,7 @@ static ssize_t disksize_store(struct device *dev,
>  		return -EINVAL;
>  
>  	disksize = PAGE_ALIGN(disksize);
> -	meta = zram_meta_alloc(zram->disk->disk_name, disksize);
> -	if (!meta)
> +	if (!zram_meta_alloc(zram, disksize))
>  		return -ENOMEM;
>  
>  	comp = zcomp_create(zram->compressor);
> @@ -1048,7 +1015,6 @@ static ssize_t disksize_store(struct device *dev,
>  		goto out_destroy_comp;
>  	}
>  
> -	zram->meta = meta;
>  	zram->comp = comp;
>  	zram->disksize = disksize;
>  	set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> @@ -1061,7 +1027,7 @@ static ssize_t disksize_store(struct device *dev,
>  	up_write(&zram->init_lock);
>  	zcomp_destroy(comp);
>  out_free_meta:
> -	zram_meta_free(meta, disksize);
> +	zram_meta_free(zram, disksize);
>  	return err;
>  }

OK, I don't think it's the same.

we used to have

	struct zram_meta *zram_meta_alloc()
	{
		meta->table = vzalloc()
		meta->mem_pool = zs_create_pool();
		return meta;
	}


	disksize_store()
	{
		meta = zram_meta_alloc();
		if (init_done(zram)) {
			pr_info("Cannot change disksize for initialized device\n");
			goto out_destroy_comp;
		}

		zram->meta = meta;
		^^^^^^^^^^^^^^^^^^
	}

now we have

	struct zram_meta *zram_meta_alloc()
	{
		zram->table = vzalloc()
		zram->mem_pool = zs_create_pool();
		return true;
	}


	disksize_store()
	{
		zram_meta_alloc();
		^^^^^^^^^^^^^^^^^
		if (init_done(zram)) {
			pr_info("Cannot change disksize for initialized device\n");
			goto out_destroy_comp;
		}
	}


by the time we call init_done(zram) on already init device zram->table
and zram->mem_pool are overwritten and lost. right?


[..]
> -struct zram_meta {
> +struct zram {
>  	struct zram_table_entry *table;
>  	struct zs_pool *mem_pool;
> -};
> -
> -struct zram {
> -	struct zram_meta *meta;
>  	struct zcomp *comp;
>  	struct gendisk *disk;
>  	/* Prevent concurrent execution of device init */


we still have several zram_meta_FOO() left overs in zram_drv.c

	-ss