Re: [PATCH 4/5] zram: remove zram_meta structure

[Minchan Kim <minchan@kernel.org>]

On Tue, Apr 04, 2017 at 11:31:15AM +0900, Sergey Senozhatsky wrote:
> 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?

Right you are.
I will fix it!

> 
> 
> [..]
> > -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