Re: [PATCH 4/5] zram: remove zram_meta structure
On (04/04/17 14:40), Minchan Kim wrote:
> > [..]
> > > -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
>
> I intentionally used that term because I don't think better name
> to wrap logis which allocates table and mempool.
ah, it was intentional. um, OK, let's keep zram_meta_foo().
-ss
Re: [PATCH 4/5] zram: remove zram_meta structure
On Tue, Apr 04, 2017 at 11:31:15AM +0900, Sergey Senozhatsky wrote:
< snip >
>
> [..]
> > -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
I intentionally used that term because I don't think better name
to wrap logis which allocates table and mempool.
Feel free to suggest if you have better.
Thanks.
Re: [PATCH 4/5] zram: remove zram_meta structure
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
Re: [PATCH 4/5] zram: remove zram_meta structure
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
