[PATCH -next] zram: fix compr_data_size stat

[PATCH -next] zram: fix compr_data_size stat

[Yisheng Xie]

The compr_data_size is a stat for compressed size of pages stored, which
should add comp_len when we compresse a page.

Meanwhile fix typos in comment:
* ZRAM_SAME means page consists the same element not the entirely zero page
* read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()

Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
---
 drivers/block/zram/zram_drv.c | 3 ++-
 drivers/block/zram/zram_drv.h | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e27daca..b2b75b1 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -455,7 +455,7 @@ void zram_page_end_io(struct bio *bio)
 }
 
 /*
- * Returns 0 if the submission is successful.
+ * Returns 1 if the submission is successful.
  */
 static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
 			unsigned long entry, struct bio *parent)
@@ -1073,6 +1073,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
 	}  else {
 		zram_set_handle(zram, index, handle);
 		zram_set_obj_size(zram, index, comp_len);
+		atomic64_add(comp_len, &zram->stats.compr_data_size);
 	}
 	zram_slot_unlock(zram, index);
 
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 5573dc2..31762db 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -60,7 +60,7 @@
 
 /* Flags for zram pages (table[page_no].value) */
 enum zram_pageflags {
-	/* Page consists entirely of zeros */
+	/* Page consists the same element */
 	ZRAM_SAME = ZRAM_FLAG_SHIFT,
 	ZRAM_ACCESS,	/* page is now accessed */
 	ZRAM_WB,	/* page is stored on backing_device */
-- 
1.8.3.1

Re: [PATCH -next] zram: fix compr_data_size stat

[Sergey Senozhatsky]

On (08/11/17 09:10), Yisheng Xie wrote:
> The compr_data_size is a stat for compressed size of pages stored, which
> should add comp_len when we compresse a page.
> 
> Meanwhile fix typos in comment:
> * ZRAM_SAME means page consists the same element not the entirely zero page
> * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
> 
> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

> ---
>  drivers/block/zram/zram_drv.c | 3 ++-
>  drivers/block/zram/zram_drv.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index e27daca..b2b75b1 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -455,7 +455,7 @@ void zram_page_end_io(struct bio *bio)
>  }
>  
>  /*
> - * Returns 0 if the submission is successful.
> + * Returns 1 if the submission is successful.
>   */
>  static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
>  			unsigned long entry, struct bio *parent)

ok.


> @@ -1073,6 +1073,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
>  	}  else {
>  		zram_set_handle(zram, index, handle);
>  		zram_set_obj_size(zram, index, comp_len);
> +		atomic64_add(comp_len, &zram->stats.compr_data_size);

hm. looks like nothing increments ->compr_data_size as of now... we only
decrement it. so it's possible to have something like

# cat /sys/block/zram0/mm_stat 

163594240 18446744073709549720 80388096        0 80388096    15927        0

oops... good catch.


>  	}
>  	zram_slot_unlock(zram, index);
>  
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 5573dc2..31762db 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -60,7 +60,7 @@
>  
>  /* Flags for zram pages (table[page_no].value) */
>  enum zram_pageflags {
> -	/* Page consists entirely of zeros */
> +	/* Page consists the same element */
>  	ZRAM_SAME = ZRAM_FLAG_SHIFT,
>  	ZRAM_ACCESS,	/* page is now accessed */
>  	ZRAM_WB,	/* page is stored on backing_device */

ok.

	-ss

Re: [PATCH -next] zram: fix compr_data_size stat

[Sergey Senozhatsky]

On (08/11/17 10:35), Sergey Senozhatsky wrote:
> On (08/11/17 09:10), Yisheng Xie wrote:
> > The compr_data_size is a stat for compressed size of pages stored, which
> > should add comp_len when we compresse a page.
> > 
> > Meanwhile fix typos in comment:
> > * ZRAM_SAME means page consists the same element not the entirely zero page
> > * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
> > 
> > Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> 
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

seems that some of the changes can be folded into the existing -next patches.

	-ss

Re: [PATCH -next] zram: fix compr_data_size stat

[Yisheng Xie]

On 2017/8/11 9:38, Sergey Senozhatsky wrote:
> On (08/11/17 10:35), Sergey Senozhatsky wrote:
>> On (08/11/17 09:10), Yisheng Xie wrote:
>>> The compr_data_size is a stat for compressed size of pages stored, which
>>> should add comp_len when we compresse a page.
>>>
>>> Meanwhile fix typos in comment:
>>> * ZRAM_SAME means page consists the same element not the entirely zero page
>>> * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
>>>
>>> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
>>
>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> seems that some of the changes can be folded into the existing -next patches.

Yes, feel free to fold it :)

I find Minchan is removing zram_rw_page(), so maybe the return value of
read_from_bdev_async and write_to_bdev can be re-writed, it seems we need to
call page_endio() depend these return value anymore, after delete zram_rw_page().

Thanks
Yisheng Xie

Re: [PATCH -next] zram: fix compr_data_size stat

[Yisheng Xie]

On 2017/8/11 10:07, Yisheng Xie wrote:
> 
> 
> On 2017/8/11 9:38, Sergey Senozhatsky wrote:
>> On (08/11/17 10:35), Sergey Senozhatsky wrote:
>>> On (08/11/17 09:10), Yisheng Xie wrote:
>>>> The compr_data_size is a stat for compressed size of pages stored, which
>>>> should add comp_len when we compresse a page.
>>>>
>>>> Meanwhile fix typos in comment:
>>>> * ZRAM_SAME means page consists the same element not the entirely zero page
>>>> * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
>>>>
>>>> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
>>>
>>> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>
>> seems that some of the changes can be folded into the existing -next patches.
> 
> Yes, feel free to fold it :)
> 
> I find Minchan is removing zram_rw_page(), so maybe the return value of
> read_from_bdev_async and write_to_bdev can be re-writed, it seems we need to
I mean no need to (sorry for typo.)
> call page_endio() depend these return value anymore, after delete zram_rw_page().
> 
> Thanks
> Yisheng Xie
> 
> 
> .
> 

Re: [PATCH -next] zram: fix compr_data_size stat

[Minchan Kim]

Hi Yisheng,

Thanks for the all fixup!
A minor nitpick below:

On Fri, Aug 11, 2017 at 09:10:32AM +0800, Yisheng Xie wrote:
> The compr_data_size is a stat for compressed size of pages stored, which
> should add comp_len when we compresse a page.
> 
> Meanwhile fix typos in comment:
> * ZRAM_SAME means page consists the same element not the entirely zero page
> * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
> 
> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> ---
>  drivers/block/zram/zram_drv.c | 3 ++-
>  drivers/block/zram/zram_drv.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index e27daca..b2b75b1 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -455,7 +455,7 @@ void zram_page_end_io(struct bio *bio)
>  }
>  
>  /*
> - * Returns 0 if the submission is successful.
> + * Returns 1 if the submission is successful.
>   */
>  static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
>  			unsigned long entry, struct bio *parent)
> @@ -1073,6 +1073,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
>  	}  else {
>  		zram_set_handle(zram, index, handle);
>  		zram_set_obj_size(zram, index, comp_len);
> +		atomic64_add(comp_len, &zram->stats.compr_data_size);

I want to add the accounting line to zs_unmap_object.
Otherwise, looks good to me.
Thanks!

Re: [PATCH -next] zram: fix compr_data_size stat

[Yisheng Xie]

Hi, Minchan

Thanks for your comment!
On 2017/8/14 16:08, Minchan Kim wrote:
> Hi Yisheng,
> 
> Thanks for the all fixup!
> A minor nitpick below:
> 
> On Fri, Aug 11, 2017 at 09:10:32AM +0800, Yisheng Xie wrote:
>> The compr_data_size is a stat for compressed size of pages stored, which
>> should add comp_len when we compresse a page.
>>
>> Meanwhile fix typos in comment:
>> * ZRAM_SAME means page consists the same element not the entirely zero page
>> * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
>>
>> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
>> ---
>>  drivers/block/zram/zram_drv.c | 3 ++-
>>  drivers/block/zram/zram_drv.h | 2 +-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>> index e27daca..b2b75b1 100644
>> --- a/drivers/block/zram/zram_drv.c
>> +++ b/drivers/block/zram/zram_drv.c
>> @@ -455,7 +455,7 @@ void zram_page_end_io(struct bio *bio)
>>  }
>>  
>>  /*
>> - * Returns 0 if the submission is successful.
>> + * Returns 1 if the submission is successful.
>>   */
>>  static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
>>  			unsigned long entry, struct bio *parent)
>> @@ -1073,6 +1073,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
>>  	}  else {
>>  		zram_set_handle(zram, index, handle);
>>  		zram_set_obj_size(zram, index, comp_len);
>> +		atomic64_add(comp_len, &zram->stats.compr_data_size);
> 
> I want to add the accounting line to zs_unmap_object.

Right, this accounting can be done without slot lock!

You will fold this patch? or should I send another version?

Thanks
Yisheng

Re: [PATCH -next] zram: fix compr_data_size stat

[Minchan Kim]

On Mon, Aug 14, 2017 at 05:24:16PM +0800, Yisheng Xie wrote:
> Hi, Minchan
> 
> Thanks for your comment!
> On 2017/8/14 16:08, Minchan Kim wrote:
> > Hi Yisheng,
> > 
> > Thanks for the all fixup!
> > A minor nitpick below:
> > 
> > On Fri, Aug 11, 2017 at 09:10:32AM +0800, Yisheng Xie wrote:
> >> The compr_data_size is a stat for compressed size of pages stored, which
> >> should add comp_len when we compresse a page.
> >>
> >> Meanwhile fix typos in comment:
> >> * ZRAM_SAME means page consists the same element not the entirely zero page
> >> * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
> >>
> >> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> >> ---
> >>  drivers/block/zram/zram_drv.c | 3 ++-
> >>  drivers/block/zram/zram_drv.h | 2 +-
> >>  2 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> >> index e27daca..b2b75b1 100644
> >> --- a/drivers/block/zram/zram_drv.c
> >> +++ b/drivers/block/zram/zram_drv.c
> >> @@ -455,7 +455,7 @@ void zram_page_end_io(struct bio *bio)
> >>  }
> >>  
> >>  /*
> >> - * Returns 0 if the submission is successful.
> >> + * Returns 1 if the submission is successful.
> >>   */
> >>  static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
> >>  			unsigned long entry, struct bio *parent)
> >> @@ -1073,6 +1073,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
> >>  	}  else {
> >>  		zram_set_handle(zram, index, handle);
> >>  		zram_set_obj_size(zram, index, comp_len);
> >> +		atomic64_add(comp_len, &zram->stats.compr_data_size);
> > 
> > I want to add the accounting line to zs_unmap_object.
> 
> Right, this accounting can be done without slot lock!
> 
> You will fold this patch? or should I send another version?

Could you separate same element comment part and others?
With that, we can apply same element comment correctpart
and fold other parts which fixes writeback feature bugs
to original patches.

Thanks!

Re: [PATCH -next] zram: fix compr_data_size stat

[Yisheng Xie]

On 2017/8/14 17:34, Minchan Kim wrote:
> On Mon, Aug 14, 2017 at 05:24:16PM +0800, Yisheng Xie wrote:
>> Hi, Minchan
>>
>> Thanks for your comment!
>> On 2017/8/14 16:08, Minchan Kim wrote:
>>> Hi Yisheng,
>>>
>>> Thanks for the all fixup!
>>> A minor nitpick below:
>>>
>>> On Fri, Aug 11, 2017 at 09:10:32AM +0800, Yisheng Xie wrote:
>>>> The compr_data_size is a stat for compressed size of pages stored, which
>>>> should add comp_len when we compresse a page.
>>>>
>>>> Meanwhile fix typos in comment:
>>>> * ZRAM_SAME means page consists the same element not the entirely zero page
>>>> * read_from_bdev_async() return 1 to avoid call page_endio() in zram_rw_page()
>>>>
>>>> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
>>>> ---
>>>>  drivers/block/zram/zram_drv.c | 3 ++-
>>>>  drivers/block/zram/zram_drv.h | 2 +-
>>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>>>> index e27daca..b2b75b1 100644
>>>> --- a/drivers/block/zram/zram_drv.c
>>>> +++ b/drivers/block/zram/zram_drv.c
>>>> @@ -455,7 +455,7 @@ void zram_page_end_io(struct bio *bio)
>>>>  }
>>>>  
>>>>  /*
>>>> - * Returns 0 if the submission is successful.
>>>> + * Returns 1 if the submission is successful.
>>>>   */
>>>>  static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
>>>>  			unsigned long entry, struct bio *parent)
>>>> @@ -1073,6 +1073,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
>>>>  	}  else {
>>>>  		zram_set_handle(zram, index, handle);
>>>>  		zram_set_obj_size(zram, index, comp_len);
>>>> +		atomic64_add(comp_len, &zram->stats.compr_data_size);
>>>
>>> I want to add the accounting line to zs_unmap_object.
>>
>> Right, this accounting can be done without slot lock!
>>
>> You will fold this patch? or should I send another version?
> 
> Could you separate same element comment part and others?

Of course.

> With that, we can apply same element comment correctpart
> and fold other parts which fixes writeback feature bugs
> to original patches.
> 
> Thanks!
> 
> .
>