[PATCH] zram: fix incorrectly stat with failed_reads

[PATCH] zram: fix incorrectly stat with failed_reads

[Chao Yu]

Since we allocate a temporary buffer in zram_bvec_read to handle partial page
operations in this commit 924bd88d703e53d30f393fac6117f8f1bc79aab6 (Staging:
zram: allow partial page operations), our ->failed_reads value may be incorrect
as we do not increase its value when failed to allocate the temporary buffer.

Let's fix this issue and correct the annotation of failed_reads.

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 drivers/block/zram/zram_drv.c | 1 +
 drivers/block/zram/zram_drv.h | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index dfa4024..bf8ea1b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -413,6 +413,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 
 	if (!uncmem) {
 		pr_info("Unable to allocate temp memory\n");
+		atomic64_inc(&zram->stats.failed_reads);
 		ret = -ENOMEM;
 		goto out_cleanup;
 	}
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 5b0afde..e0f725c 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -84,7 +84,7 @@ struct zram_stats {
 	atomic64_t compr_data_size;	/* compressed size of pages stored */
 	atomic64_t num_reads;	/* failed + successful */
 	atomic64_t num_writes;	/* --do-- */
-	atomic64_t failed_reads;	/* should NEVER! happen */
+	atomic64_t failed_reads;	/* can happen when memory is too low */
 	atomic64_t failed_writes;	/* can happen when memory is too low */
 	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
 	atomic64_t notify_free;	/* no. of swap slot free notifications */
-- 
2.0.1.474.g72c7794

Re: [PATCH] zram: fix incorrectly stat with failed_reads

[Minchan Kim]

On Mon, Aug 11, 2014 at 04:39:17PM +0800, Chao Yu wrote:
> Since we allocate a temporary buffer in zram_bvec_read to handle partial page
> operations in this commit 924bd88d703e53d30f393fac6117f8f1bc79aab6 (Staging:
> zram: allow partial page operations), our ->failed_reads value may be incorrect
> as we do not increase its value when failed to allocate the temporary buffer.
> 
> Let's fix this issue and correct the annotation of failed_reads.

Good catch.
Just out of curiosity.

Did you catch it by code review or real workload?

> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  drivers/block/zram/zram_drv.c | 1 +
>  drivers/block/zram/zram_drv.h | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index dfa4024..bf8ea1b 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -413,6 +413,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
>  
>  	if (!uncmem) {
>  		pr_info("Unable to allocate temp memory\n");
> +		atomic64_inc(&zram->stats.failed_reads);
>  		ret = -ENOMEM;
>  		goto out_cleanup;
>  	}
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 5b0afde..e0f725c 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -84,7 +84,7 @@ struct zram_stats {
>  	atomic64_t compr_data_size;	/* compressed size of pages stored */
>  	atomic64_t num_reads;	/* failed + successful */
>  	atomic64_t num_writes;	/* --do-- */
> -	atomic64_t failed_reads;	/* should NEVER! happen */
> +	atomic64_t failed_reads;	/* can happen when memory is too low */
>  	atomic64_t failed_writes;	/* can happen when memory is too low */
>  	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
>  	atomic64_t notify_free;	/* no. of swap slot free notifications */
> -- 
> 2.0.1.474.g72c7794

How abouting moving failed_reads/writes in zram_bvec_rw?

int zram_bvec_rw(..)
{
        if (rw == READ) {
                atomic64_inc(num_reads);
                ret = zram_bvec_read(xxx);
        } else {
                atomic64_inc(&num_writes);
                ret = zram_bvec_write(xxx);
        }

        if (unlikely(ret)) {
                if (rw == READ)
                        atomic64_inc(failed_reads);
                else
                        atomic64_inc(failed_writes);
        }
}

> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Kind regards,
Minchan Kim

RE: [PATCH] zram: fix incorrectly stat with failed_reads

[Chao Yu]

Hi Minchan,

> -----Original Message-----
> From: Minchan Kim [mailto:minchan@kernel.org]
> Sent: Tuesday, August 12, 2014 3:38 PM
> To: Chao Yu
> Cc: ngupta@vflare.org; linux-kernel@vger.kernel.org; Jerome Marchand; Sergey Senozhatsky;
> Andrew Morton
> Subject: Re: [PATCH] zram: fix incorrectly stat with failed_reads
> 
> On Mon, Aug 11, 2014 at 04:39:17PM +0800, Chao Yu wrote:
> > Since we allocate a temporary buffer in zram_bvec_read to handle partial page
> > operations in this commit 924bd88d703e53d30f393fac6117f8f1bc79aab6 (Staging:
> > zram: allow partial page operations), our ->failed_reads value may be incorrect
> > as we do not increase its value when failed to allocate the temporary buffer.
> >
> > Let's fix this issue and correct the annotation of failed_reads.
> 
> Good catch.
> Just out of curiosity.
> 
> Did you catch it by code review or real workload?

I found this one by code review.

> 
> >
> > Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 1 +
> >  drivers/block/zram/zram_drv.h | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index dfa4024..bf8ea1b 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -413,6 +413,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> >
> >  	if (!uncmem) {
> >  		pr_info("Unable to allocate temp memory\n");
> > +		atomic64_inc(&zram->stats.failed_reads);
> >  		ret = -ENOMEM;
> >  		goto out_cleanup;
> >  	}
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 5b0afde..e0f725c 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -84,7 +84,7 @@ struct zram_stats {
> >  	atomic64_t compr_data_size;	/* compressed size of pages stored */
> >  	atomic64_t num_reads;	/* failed + successful */
> >  	atomic64_t num_writes;	/* --do-- */
> > -	atomic64_t failed_reads;	/* should NEVER! happen */
> > +	atomic64_t failed_reads;	/* can happen when memory is too low */
> >  	atomic64_t failed_writes;	/* can happen when memory is too low */
> >  	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
> >  	atomic64_t notify_free;	/* no. of swap slot free notifications */
> > --
> > 2.0.1.474.g72c7794
> 
> How abouting moving failed_reads/writes in zram_bvec_rw?

I think this implementation of yours is pretty clean and simple because this one
hides every error paths inside zram_bvec_{read,write}, so we do not need to
consider covering the stat code to everywhere of inside.

IMHO, your implementation also fixed the issue in following call chain:
->zram_bvec_rw
  ->zram_bvec_write
    ->zram_decompress_page
      ->atomic64_inc(failed_reads)    ---at perspective of user, this kind of
      failure should be regarded as the write one's, because user pay fewer
      attention to inner error of zram.

Please correct me if I'm wrong, and thanks for your review and suggestion. :)

Regards,
Yu

> 
> int zram_bvec_rw(..)
> {
>         if (rw == READ) {
>                 atomic64_inc(num_reads);
>                 ret = zram_bvec_read(xxx);
>         } else {
>                 atomic64_inc(&num_writes);
>                 ret = zram_bvec_write(xxx);
>         }
> 
>         if (unlikely(ret)) {
>                 if (rw == READ)
>                         atomic64_inc(failed_reads);
>                 else
>                         atomic64_inc(failed_writes);
>         }
> }
> 
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> Kind regards,
> Minchan Kim

Re: [PATCH] zram: fix incorrectly stat with failed_reads

[Sergey Senozhatsky]

On (08/12/14 16:37), Minchan Kim wrote:
> On Mon, Aug 11, 2014 at 04:39:17PM +0800, Chao Yu wrote:
> > Since we allocate a temporary buffer in zram_bvec_read to handle partial page
> > operations in this commit 924bd88d703e53d30f393fac6117f8f1bc79aab6 (Staging:
> > zram: allow partial page operations), our ->failed_reads value may be incorrect
> > as we do not increase its value when failed to allocate the temporary buffer.
> > 
> > Let's fix this issue and correct the annotation of failed_reads.
> 
> Good catch.
> Just out of curiosity.
> 
> Did you catch it by code review or real workload?
> 
> > 
> > Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 1 +
> >  drivers/block/zram/zram_drv.h | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index dfa4024..bf8ea1b 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -413,6 +413,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> >  
> >  	if (!uncmem) {
> >  		pr_info("Unable to allocate temp memory\n");
> > +		atomic64_inc(&zram->stats.failed_reads);
> >  		ret = -ENOMEM;
> >  		goto out_cleanup;
> >  	}
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 5b0afde..e0f725c 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -84,7 +84,7 @@ struct zram_stats {
> >  	atomic64_t compr_data_size;	/* compressed size of pages stored */
> >  	atomic64_t num_reads;	/* failed + successful */
> >  	atomic64_t num_writes;	/* --do-- */
> > -	atomic64_t failed_reads;	/* should NEVER! happen */
> > +	atomic64_t failed_reads;	/* can happen when memory is too low */
> >  	atomic64_t failed_writes;	/* can happen when memory is too low */
> >  	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
> >  	atomic64_t notify_free;	/* no. of swap slot free notifications */
> > -- 
> > 2.0.1.474.g72c7794
> 
> How abouting moving failed_reads/writes in zram_bvec_rw?
> 
> int zram_bvec_rw(..)
> {
>         if (rw == READ) {
>                 atomic64_inc(num_reads);
>                 ret = zram_bvec_read(xxx);
>         } else {
>                 atomic64_inc(&num_writes);
>                 ret = zram_bvec_write(xxx);
>         }
> 
>         if (unlikely(ret)) {
>                 if (rw == READ)
>                         atomic64_inc(failed_reads);
>                 else
>                         atomic64_inc(failed_writes);
>         }
> }
> 

looks good. these stats were supposed to be here.

	-ss

> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> -- 
> Kind regards,
> Minchan Kim
> 

RE: [PATCH] zram: fix incorrectly stat with failed_reads

[Chao Yu]

Hi Minchan,

> -----Original Message-----
> From: Minchan Kim [mailto:minchan@kernel.org]
> Sent: Tuesday, August 12, 2014 3:38 PM
> To: Chao Yu
> Cc: ngupta@vflare.org; linux-kernel@vger.kernel.org; Jerome Marchand; Sergey Senozhatsky;
> Andrew Morton
> Subject: Re: [PATCH] zram: fix incorrectly stat with failed_reads
> 
> On Mon, Aug 11, 2014 at 04:39:17PM +0800, Chao Yu wrote:
> > Since we allocate a temporary buffer in zram_bvec_read to handle partial page
> > operations in this commit 924bd88d703e53d30f393fac6117f8f1bc79aab6 (Staging:
> > zram: allow partial page operations), our ->failed_reads value may be incorrect
> > as we do not increase its value when failed to allocate the temporary buffer.
> >
> > Let's fix this issue and correct the annotation of failed_reads.

[snip]

> How abouting moving failed_reads/writes in zram_bvec_rw?
> 
> int zram_bvec_rw(..)
> {
>         if (rw == READ) {
>                 atomic64_inc(num_reads);
>                 ret = zram_bvec_read(xxx);
>         } else {
>                 atomic64_inc(&num_writes);
>                 ret = zram_bvec_write(xxx);
>         }
> 
>         if (unlikely(ret)) {
>                 if (rw == READ)
>                         atomic64_inc(failed_reads);
>                 else
>                         atomic64_inc(failed_writes);
>         }
> }

I will send a v2 patch base on above codes of yours, please help to review the
following new patch.

> 
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> Kind regards,
> Minchan Kim