[PATCH 0/3] zram stats rework and code cleanup

[PATCH 0/3] zram stats rework and code cleanup

[Sergey Senozhatsky]

This patchset includes code clean up and zram stats rework.

Sergey Senozhatsky (3):
  zram: drop `init_done' struct zram member
  zram: do not pass rw argument to __zram_make_request()
  zram: rework reported to end-user zram statistics

 drivers/block/zram/zram_drv.c | 213 ++++++++++++++----------------------------
 drivers/block/zram/zram_drv.h |  18 ++--
 2 files changed, 76 insertions(+), 155 deletions(-)

-- 
1.8.5.2.487.g7559984

[PATCH 1/3] zram: drop `init_done' struct zram member

[Sergey Senozhatsky]

Introduce init_done() helper function which allows us to drop
`init_done' struct zram member. init_done() uses the fact that
->init_done == 1 equals to ->meta != NULL.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 21 +++++++++++----------
 drivers/block/zram/zram_drv.h |  1 -
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 011e55d..c323e05 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -42,6 +42,11 @@ static struct zram *zram_devices;
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
 
+static inline int init_done(struct zram *zram)
+{
+	return zram->meta != NULL;
+}
+
 static inline struct zram *dev_to_zram(struct device *dev)
 {
 	return (struct zram *)dev_to_disk(dev)->private_data;
@@ -60,7 +65,7 @@ static ssize_t initstate_show(struct device *dev,
 {
 	struct zram *zram = dev_to_zram(dev);
 
-	return sprintf(buf, "%u\n", zram->init_done);
+	return sprintf(buf, "%u\n", init_done(zram));
 }
 
 static ssize_t num_reads_show(struct device *dev,
@@ -133,7 +138,7 @@ static ssize_t mem_used_total_show(struct device *dev,
 	struct zram_meta *meta = zram->meta;
 
 	down_read(&zram->init_lock);
-	if (zram->init_done)
+	if (init_done(zram))
 		val = zs_get_total_size_bytes(meta->mem_pool);
 	up_read(&zram->init_lock);
 
@@ -546,14 +551,12 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 	struct zram_meta *meta;
 
 	down_write(&zram->init_lock);
-	if (!zram->init_done) {
+	if (!init_done(zram)) {
 		up_write(&zram->init_lock);
 		return;
 	}
 
 	meta = zram->meta;
-	zram->init_done = 0;
-
 	/* Free all pages that are still in this zram device */
 	for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
 		unsigned long handle = meta->table[index].handle;
@@ -594,8 +597,6 @@ static void zram_init_device(struct zram *zram, struct zram_meta *meta)
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
 
 	zram->meta = meta;
-	zram->init_done = 1;
-
 	pr_debug("Initialization done!\n");
 }
 
@@ -613,7 +614,7 @@ static ssize_t disksize_store(struct device *dev,
 	disksize = PAGE_ALIGN(disksize);
 	meta = zram_meta_alloc(disksize);
 	down_write(&zram->init_lock);
-	if (zram->init_done) {
+	if (init_done(zram)) {
 		up_write(&zram->init_lock);
 		zram_meta_free(meta);
 		pr_info("Cannot change disksize for initialized device\n");
@@ -734,7 +735,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
 	struct zram *zram = queue->queuedata;
 
 	down_read(&zram->init_lock);
-	if (unlikely(!zram->init_done))
+	if (unlikely(!init_done(zram)))
 		goto error;
 
 	if (!valid_io_request(zram, bio)) {
@@ -857,7 +858,7 @@ static int create_device(struct zram *zram, int device_id)
 		goto out_free_disk;
 	}
 
-	zram->init_done = 0;
+	zram->meta = NULL;
 	return 0;
 
 out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index ad8aa35..e81e9cd 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -95,7 +95,6 @@ struct zram {
 	struct zram_meta *meta;
 	struct request_queue *queue;
 	struct gendisk *disk;
-	int init_done;
 	/* Prevent concurrent execution of device init, reset and R/W request */
 	struct rw_semaphore init_lock;
 	/*
-- 
1.8.5.2.487.g7559984

Re: [PATCH 1/3] zram: drop `init_done' struct zram member

[Jerome Marchand]

On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> Introduce init_done() helper function which allows us to drop
> `init_done' struct zram member. init_done() uses the fact that
> ->init_done == 1 equals to ->meta != NULL.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Acked-by: Jerome Marchand <jmarchan@redhat.com>

> ---
>  drivers/block/zram/zram_drv.c | 21 +++++++++++----------
>  drivers/block/zram/zram_drv.h |  1 -
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 011e55d..c323e05 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -42,6 +42,11 @@ static struct zram *zram_devices;
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +static inline int init_done(struct zram *zram)
> +{
> +	return zram->meta != NULL;
> +}
> +
>  static inline struct zram *dev_to_zram(struct device *dev)
>  {
>  	return (struct zram *)dev_to_disk(dev)->private_data;
> @@ -60,7 +65,7 @@ static ssize_t initstate_show(struct device *dev,
>  {
>  	struct zram *zram = dev_to_zram(dev);
>  
> -	return sprintf(buf, "%u\n", zram->init_done);
> +	return sprintf(buf, "%u\n", init_done(zram));
>  }
>  
>  static ssize_t num_reads_show(struct device *dev,
> @@ -133,7 +138,7 @@ static ssize_t mem_used_total_show(struct device *dev,
>  	struct zram_meta *meta = zram->meta;
>  
>  	down_read(&zram->init_lock);
> -	if (zram->init_done)
> +	if (init_done(zram))
>  		val = zs_get_total_size_bytes(meta->mem_pool);
>  	up_read(&zram->init_lock);
>  
> @@ -546,14 +551,12 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>  	struct zram_meta *meta;
>  
>  	down_write(&zram->init_lock);
> -	if (!zram->init_done) {
> +	if (!init_done(zram)) {
>  		up_write(&zram->init_lock);
>  		return;
>  	}
>  
>  	meta = zram->meta;
> -	zram->init_done = 0;
> -
>  	/* Free all pages that are still in this zram device */
>  	for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
>  		unsigned long handle = meta->table[index].handle;
> @@ -594,8 +597,6 @@ static void zram_init_device(struct zram *zram, struct zram_meta *meta)
>  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
>  
>  	zram->meta = meta;
> -	zram->init_done = 1;
> -
>  	pr_debug("Initialization done!\n");
>  }
>  
> @@ -613,7 +614,7 @@ static ssize_t disksize_store(struct device *dev,
>  	disksize = PAGE_ALIGN(disksize);
>  	meta = zram_meta_alloc(disksize);
>  	down_write(&zram->init_lock);
> -	if (zram->init_done) {
> +	if (init_done(zram)) {
>  		up_write(&zram->init_lock);
>  		zram_meta_free(meta);
>  		pr_info("Cannot change disksize for initialized device\n");
> @@ -734,7 +735,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
>  	struct zram *zram = queue->queuedata;
>  
>  	down_read(&zram->init_lock);
> -	if (unlikely(!zram->init_done))
> +	if (unlikely(!init_done(zram)))
>  		goto error;
>  
>  	if (!valid_io_request(zram, bio)) {
> @@ -857,7 +858,7 @@ static int create_device(struct zram *zram, int device_id)
>  		goto out_free_disk;
>  	}
>  
> -	zram->init_done = 0;
> +	zram->meta = NULL;
>  	return 0;
>  
>  out_free_disk:
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index ad8aa35..e81e9cd 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -95,7 +95,6 @@ struct zram {
>  	struct zram_meta *meta;
>  	struct request_queue *queue;
>  	struct gendisk *disk;
> -	int init_done;
>  	/* Prevent concurrent execution of device init, reset and R/W request */
>  	struct rw_semaphore init_lock;
>  	/*
> 

Re: [PATCH 1/3] zram: drop `init_done' struct zram member

[Minchan Kim]

On Tue, Jan 14, 2014 at 12:37:38PM +0300, Sergey Senozhatsky wrote:
> Introduce init_done() helper function which allows us to drop
> `init_done' struct zram member. init_done() uses the fact that
> ->init_done == 1 equals to ->meta != NULL.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

[PATCH 2/3] zram: do not pass rw argument to __zram_make_request()

[Sergey Senozhatsky]

Do not pass rw argument down the __zram_make_request() -> zram_bvec_rw()
chain, decode it in zram_bvec_rw() instead. Besides, this is the place
where we distinguish READ (+READ AHEAD) and WRITE bio data directions, so
account zram RW stats here, instead of __zram_make_request(). This also
allows to account a real number of zram READ/WRITE operations, not just
requests (single RW request may cause a number of zram RW ops with separate
locking, compression/decompression, etc).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index c323e05..2a7682c 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -533,14 +533,21 @@ out:
 }
 
 static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
-			int offset, struct bio *bio, int rw)
+			int offset, struct bio *bio)
 {
 	int ret;
+	int rw = bio_data_dir(bio);
 
-	if (rw == READ)
+	if (rw == READA)
+		rw = READ;
+
+	if (rw == READ) {
+		atomic64_inc(&zram->stats.num_reads);
 		ret = zram_bvec_read(zram, bvec, index, offset, bio);
-	else
+	} else {
+		atomic64_inc(&zram->stats.num_writes);
 		ret = zram_bvec_write(zram, bvec, index, offset);
+	}
 
 	return ret;
 }
@@ -670,22 +677,13 @@ out:
 	return ret;
 }
 
-static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
+static void __zram_make_request(struct zram *zram, struct bio *bio)
 {
 	int offset;
 	u32 index;
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
-	switch (rw) {
-	case READ:
-		atomic64_inc(&zram->stats.num_reads);
-		break;
-	case WRITE:
-		atomic64_inc(&zram->stats.num_writes);
-		break;
-	}
-
 	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
 	offset = (bio->bi_iter.bi_sector &
 		  (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
@@ -704,16 +702,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
 			bv.bv_len = max_transfer_size;
 			bv.bv_offset = bvec.bv_offset;
 
-			if (zram_bvec_rw(zram, &bv, index, offset, bio, rw) < 0)
+			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
 				goto out;
 
 			bv.bv_len = bvec.bv_len - max_transfer_size;
 			bv.bv_offset += max_transfer_size;
-			if (zram_bvec_rw(zram, &bv, index+1, 0, bio, rw) < 0)
+			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
 				goto out;
 		} else
-			if (zram_bvec_rw(zram, &bvec, index, offset, bio, rw)
-			    < 0)
+			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
 				goto out;
 
 		update_position(&index, &offset, &bvec);
@@ -743,7 +740,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
 		goto error;
 	}
 
-	__zram_make_request(zram, bio, bio_data_dir(bio));
+	__zram_make_request(zram, bio);
 	up_read(&zram->init_lock);
 
 	return;
-- 
1.8.5.2.487.g7559984

Re: [PATCH 2/3] zram: do not pass rw argument to __zram_make_request()

[Jerome Marchand]

On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> Do not pass rw argument down the __zram_make_request() -> zram_bvec_rw()
> chain, decode it in zram_bvec_rw() instead. Besides, this is the place
> where we distinguish READ (+READ AHEAD) and WRITE bio data directions, so
> account zram RW stats here, instead of __zram_make_request(). This also
> allows to account a real number of zram READ/WRITE operations, not just
> requests (single RW request may cause a number of zram RW ops with separate
> locking, compression/decompression, etc).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index c323e05..2a7682c 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -533,14 +533,21 @@ out:
>  }
>  
>  static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
> -			int offset, struct bio *bio, int rw)
> +			int offset, struct bio *bio)
>  {
>  	int ret;
> +	int rw = bio_data_dir(bio);
>  
> -	if (rw == READ)
> +	if (rw == READA)
> +		rw = READ;

This could never happen: bio_data_dir() can only return READ or WRITE.

Jerome

> +
> +	if (rw == READ) {
> +		atomic64_inc(&zram->stats.num_reads);
>  		ret = zram_bvec_read(zram, bvec, index, offset, bio);
> -	else
> +	} else {
> +		atomic64_inc(&zram->stats.num_writes);
>  		ret = zram_bvec_write(zram, bvec, index, offset);
> +	}
>  
>  	return ret;
>  }
> @@ -670,22 +677,13 @@ out:
>  	return ret;
>  }
>  
> -static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
> +static void __zram_make_request(struct zram *zram, struct bio *bio)
>  {
>  	int offset;
>  	u32 index;
>  	struct bio_vec bvec;
>  	struct bvec_iter iter;
>  
> -	switch (rw) {
> -	case READ:
> -		atomic64_inc(&zram->stats.num_reads);
> -		break;
> -	case WRITE:
> -		atomic64_inc(&zram->stats.num_writes);
> -		break;
> -	}
> -
>  	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
>  	offset = (bio->bi_iter.bi_sector &
>  		  (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> @@ -704,16 +702,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
>  			bv.bv_len = max_transfer_size;
>  			bv.bv_offset = bvec.bv_offset;
>  
> -			if (zram_bvec_rw(zram, &bv, index, offset, bio, rw) < 0)
> +			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
>  				goto out;
>  
>  			bv.bv_len = bvec.bv_len - max_transfer_size;
>  			bv.bv_offset += max_transfer_size;
> -			if (zram_bvec_rw(zram, &bv, index+1, 0, bio, rw) < 0)
> +			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
>  				goto out;
>  		} else
> -			if (zram_bvec_rw(zram, &bvec, index, offset, bio, rw)
> -			    < 0)
> +			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
>  				goto out;
>  
>  		update_position(&index, &offset, &bvec);
> @@ -743,7 +740,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
>  		goto error;
>  	}
>  
> -	__zram_make_request(zram, bio, bio_data_dir(bio));
> +	__zram_make_request(zram, bio);
>  	up_read(&zram->init_lock);
>  
>  	return;
> 

Re: [PATCH 2/3] zram: do not pass rw argument to __zram_make_request()

[Sergey Senozhatsky]

On (01/14/14 12:02), Jerome Marchand wrote:
> >  static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
> > -			int offset, struct bio *bio, int rw)
> > +			int offset, struct bio *bio)
> >  {
> >  	int ret;
> > +	int rw = bio_data_dir(bio);
> >  
> > -	if (rw == READ)
> > +	if (rw == READA)
> > +		rw = READ;
> 
> This could never happen: bio_data_dir() can only return READ or WRITE.
> 

thanks. my bad. will replace with bio_rw().

	-ss

> Jerome
> 
> > +
> > +	if (rw == READ) {
> > +		atomic64_inc(&zram->stats.num_reads);
> >  		ret = zram_bvec_read(zram, bvec, index, offset, bio);
> > -	else
> > +	} else {
> > +		atomic64_inc(&zram->stats.num_writes);
> >  		ret = zram_bvec_write(zram, bvec, index, offset);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -670,22 +677,13 @@ out:
> >  	return ret;
> >  }
> >  
> > -static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
> > +static void __zram_make_request(struct zram *zram, struct bio *bio)
> >  {
> >  	int offset;
> >  	u32 index;
> >  	struct bio_vec bvec;
> >  	struct bvec_iter iter;
> >  
> > -	switch (rw) {
> > -	case READ:
> > -		atomic64_inc(&zram->stats.num_reads);
> > -		break;
> > -	case WRITE:
> > -		atomic64_inc(&zram->stats.num_writes);
> > -		break;
> > -	}
> > -
> >  	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
> >  	offset = (bio->bi_iter.bi_sector &
> >  		  (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> > @@ -704,16 +702,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
> >  			bv.bv_len = max_transfer_size;
> >  			bv.bv_offset = bvec.bv_offset;
> >  
> > -			if (zram_bvec_rw(zram, &bv, index, offset, bio, rw) < 0)
> > +			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
> >  				goto out;
> >  
> >  			bv.bv_len = bvec.bv_len - max_transfer_size;
> >  			bv.bv_offset += max_transfer_size;
> > -			if (zram_bvec_rw(zram, &bv, index+1, 0, bio, rw) < 0)
> > +			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
> >  				goto out;
> >  		} else
> > -			if (zram_bvec_rw(zram, &bvec, index, offset, bio, rw)
> > -			    < 0)
> > +			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
> >  				goto out;
> >  
> >  		update_position(&index, &offset, &bvec);
> > @@ -743,7 +740,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
> >  		goto error;
> >  	}
> >  
> > -	__zram_make_request(zram, bio, bio_data_dir(bio));
> > +	__zram_make_request(zram, bio);
> >  	up_read(&zram->init_lock);
> >  
> >  	return;
> > 
> 

Re: [PATCH 2/3] zram: do not pass rw argument to __zram_make_request()

[Jerome Marchand]

On 01/14/2014 12:13 PM, Sergey Senozhatsky wrote:
> On (01/14/14 12:02), Jerome Marchand wrote:
>>>  static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
>>> -			int offset, struct bio *bio, int rw)
>>> +			int offset, struct bio *bio)
>>>  {
>>>  	int ret;
>>> +	int rw = bio_data_dir(bio);
>>>  
>>> -	if (rw == READ)
>>> +	if (rw == READA)
>>> +		rw = READ;
>>
>> This could never happen: bio_data_dir() can only return READ or WRITE.
>>
> 
> thanks. my bad. will replace with bio_rw().

There is no point in doing that. In read-ahead case, bio_data_dir()
already returns READ. Since we don't do anything special in read-ahead
case, just keep bio_data_dir() and drop this test.

> 
> 	-ss
> 
>> Jerome
>>
>>> +
>>> +	if (rw == READ) {
>>> +		atomic64_inc(&zram->stats.num_reads);
>>>  		ret = zram_bvec_read(zram, bvec, index, offset, bio);
>>> -	else
>>> +	} else {
>>> +		atomic64_inc(&zram->stats.num_writes);
>>>  		ret = zram_bvec_write(zram, bvec, index, offset);
>>> +	}
>>>  
>>>  	return ret;
>>>  }
>>> @@ -670,22 +677,13 @@ out:
>>>  	return ret;
>>>  }
>>>  
>>> -static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
>>> +static void __zram_make_request(struct zram *zram, struct bio *bio)
>>>  {
>>>  	int offset;
>>>  	u32 index;
>>>  	struct bio_vec bvec;
>>>  	struct bvec_iter iter;
>>>  
>>> -	switch (rw) {
>>> -	case READ:
>>> -		atomic64_inc(&zram->stats.num_reads);
>>> -		break;
>>> -	case WRITE:
>>> -		atomic64_inc(&zram->stats.num_writes);
>>> -		break;
>>> -	}
>>> -
>>>  	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
>>>  	offset = (bio->bi_iter.bi_sector &
>>>  		  (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
>>> @@ -704,16 +702,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
>>>  			bv.bv_len = max_transfer_size;
>>>  			bv.bv_offset = bvec.bv_offset;
>>>  
>>> -			if (zram_bvec_rw(zram, &bv, index, offset, bio, rw) < 0)
>>> +			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
>>>  				goto out;
>>>  
>>>  			bv.bv_len = bvec.bv_len - max_transfer_size;
>>>  			bv.bv_offset += max_transfer_size;
>>> -			if (zram_bvec_rw(zram, &bv, index+1, 0, bio, rw) < 0)
>>> +			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
>>>  				goto out;
>>>  		} else
>>> -			if (zram_bvec_rw(zram, &bvec, index, offset, bio, rw)
>>> -			    < 0)
>>> +			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
>>>  				goto out;
>>>  
>>>  		update_position(&index, &offset, &bvec);
>>> @@ -743,7 +740,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
>>>  		goto error;
>>>  	}
>>>  
>>> -	__zram_make_request(zram, bio, bio_data_dir(bio));
>>> +	__zram_make_request(zram, bio);
>>>  	up_read(&zram->init_lock);
>>>  
>>>  	return;
>>>
>>

Re: [PATCHv2 2/3] zram: do not pass rw argument to __zram_make_request()

[Sergey Senozhatsky]

Do not pass rw argument down the __zram_make_request() -> zram_bvec_rw()
chain, decode it in zram_bvec_rw() instead. Besides, this is the place
where we distinguish READ (+READ AHEAD) and WRITE bio data directions, so
account zram RW stats here, instead of __zram_make_request(). This also
allows to account a real number of zram READ/WRITE operations, not just
requests (single RW request may cause a number of zram RW ops with separate
locking, compression/decompression, etc).

v2: use bio_rw() instead of bio_data_dir(). noted by Jerome Marchand.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index c323e05..2a7682c 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -533,14 +533,21 @@ out:
 }
 
 static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
-			int offset, struct bio *bio, int rw)
+			int offset, struct bio *bio)
 {
 	int ret;
+	int rw = bio_rw(bio);
 
-	if (rw == READ)
+	if (rw == READA)
+		rw = READ;
+
+	if (rw == READ) {
+		atomic64_inc(&zram->stats.num_reads);
 		ret = zram_bvec_read(zram, bvec, index, offset, bio);
-	else
+	} else {
+		atomic64_inc(&zram->stats.num_writes);
 		ret = zram_bvec_write(zram, bvec, index, offset);
+	}
 
 	return ret;
 }
@@ -670,22 +677,13 @@ out:
 	return ret;
 }
 
-static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
+static void __zram_make_request(struct zram *zram, struct bio *bio)
 {
 	int offset;
 	u32 index;
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
-	switch (rw) {
-	case READ:
-		atomic64_inc(&zram->stats.num_reads);
-		break;
-	case WRITE:
-		atomic64_inc(&zram->stats.num_writes);
-		break;
-	}
-
 	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
 	offset = (bio->bi_iter.bi_sector &
 		  (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
@@ -704,16 +702,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
 			bv.bv_len = max_transfer_size;
 			bv.bv_offset = bvec.bv_offset;
 
-			if (zram_bvec_rw(zram, &bv, index, offset, bio, rw) < 0)
+			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
 				goto out;
 
 			bv.bv_len = bvec.bv_len - max_transfer_size;
 			bv.bv_offset += max_transfer_size;
-			if (zram_bvec_rw(zram, &bv, index+1, 0, bio, rw) < 0)
+			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
 				goto out;
 		} else
-			if (zram_bvec_rw(zram, &bvec, index, offset, bio, rw)
-			    < 0)
+			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
 				goto out;
 
 		update_position(&index, &offset, &bvec);
@@ -743,7 +740,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
 		goto error;
 	}
 
-	__zram_make_request(zram, bio, bio_data_dir(bio));
+	__zram_make_request(zram, bio);
 	up_read(&zram->init_lock);
 
 	return;

Re: [PATCHv2 2/3] zram: do not pass rw argument to __zram_make_request()

[Minchan Kim]

Hello Sergey,

On Tue, Jan 14, 2014 at 02:27:23PM +0300, Sergey Senozhatsky wrote:
> Do not pass rw argument down the __zram_make_request() -> zram_bvec_rw()
> chain, decode it in zram_bvec_rw() instead. Besides, this is the place
> where we distinguish READ (+READ AHEAD) and WRITE bio data directions, so
> account zram RW stats here, instead of __zram_make_request(). This also
> allows to account a real number of zram READ/WRITE operations, not just
> requests (single RW request may cause a number of zram RW ops with separate
> locking, compression/decompression, etc).
> 
> v2: use bio_rw() instead of bio_data_dir(). noted by Jerome Marchand.

We don't have any special logic for READA so please use bio_data_dir
instead of bio_rw.

And please Ccing Andrew Morton when you send a updated patch.
Thanks.

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index c323e05..2a7682c 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -533,14 +533,21 @@ out:
>  }
>  
>  static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
> -			int offset, struct bio *bio, int rw)
> +			int offset, struct bio *bio)
>  {
>  	int ret;
> +	int rw = bio_rw(bio);
>  
> -	if (rw == READ)
> +	if (rw == READA)
> +		rw = READ;
> +
> +	if (rw == READ) {
> +		atomic64_inc(&zram->stats.num_reads);
>  		ret = zram_bvec_read(zram, bvec, index, offset, bio);
> -	else
> +	} else {
> +		atomic64_inc(&zram->stats.num_writes);
>  		ret = zram_bvec_write(zram, bvec, index, offset);
> +	}
>  
>  	return ret;
>  }
> @@ -670,22 +677,13 @@ out:
>  	return ret;
>  }
>  
> -static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
> +static void __zram_make_request(struct zram *zram, struct bio *bio)
>  {
>  	int offset;
>  	u32 index;
>  	struct bio_vec bvec;
>  	struct bvec_iter iter;
>  
> -	switch (rw) {
> -	case READ:
> -		atomic64_inc(&zram->stats.num_reads);
> -		break;
> -	case WRITE:
> -		atomic64_inc(&zram->stats.num_writes);
> -		break;
> -	}
> -
>  	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
>  	offset = (bio->bi_iter.bi_sector &
>  		  (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> @@ -704,16 +702,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
>  			bv.bv_len = max_transfer_size;
>  			bv.bv_offset = bvec.bv_offset;
>  
> -			if (zram_bvec_rw(zram, &bv, index, offset, bio, rw) < 0)
> +			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
>  				goto out;
>  
>  			bv.bv_len = bvec.bv_len - max_transfer_size;
>  			bv.bv_offset += max_transfer_size;
> -			if (zram_bvec_rw(zram, &bv, index+1, 0, bio, rw) < 0)
> +			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
>  				goto out;
>  		} else
> -			if (zram_bvec_rw(zram, &bvec, index, offset, bio, rw)
> -			    < 0)
> +			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
>  				goto out;
>  
>  		update_position(&index, &offset, &bvec);
> @@ -743,7 +740,7 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
>  		goto error;
>  	}
>  
> -	__zram_make_request(zram, bio, bio_data_dir(bio));
> +	__zram_make_request(zram, bio);
>  	up_read(&zram->init_lock);
>  
>  	return;
> 
> --
> 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

[PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
`show' functions and reduce code duplication.

2) Account and report back to user numbers of failed READ and WRITE
operations.

3) Remove `good' and `bad' compressed sub-requests stats. RW request may
cause a number of RW sub-requests. zram used to account `good' compressed
sub-queries (with compressed size less than 50% of original size), `bad'
compressed sub-queries (with compressed size greater that 75% of original
size), leaving sub-requests with compression size between 50% and 75% of
original size not accounted and not reported. Account each sub-request
compression size so we can calculate real device compression ratio.

4) reported zram stats:
  - num_writes  -- number of writes
  - num_reads  -- number of reads
  - pages_stored -- number of pages currently stored
  - compressed_size -- compressed size of pages stored
  - pages_zero  -- number of zero filled pages
  - failed_read -- number of failed reads
  - failed_writes -- can happen when memory is too low
  - invalid_io   -- non-page-aligned I/O requests
  - notify_free  -- number of swap slot free notifications
  - memory_used -- zs pool zs_get_total_size_bytes()

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
 drivers/block/zram/zram_drv.h |  17 ++---
 2 files changed, 54 insertions(+), 130 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 2a7682c..8bddaff 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -42,6 +42,17 @@ static struct zram *zram_devices;
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
 
+#define ZRAM_ATTR_RO(name)						\
+static ssize_t zram_attr_##name##_show(struct device *d,		\
+				struct device_attribute *attr, char *b)	\
+{									\
+	struct zram *zram = dev_to_zram(d);				\
+	return sprintf(b, "%llu\n",					\
+		(u64)atomic64_read(&zram->stats.name));			\
+}									\
+static struct device_attribute dev_attr_##name =			\
+	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
+
 static inline int init_done(struct zram *zram)
 {
 	return zram->meta != NULL;
@@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
 	return (struct zram *)dev_to_disk(dev)->private_data;
 }
 
-static ssize_t disksize_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%llu\n", zram->disksize);
-}
-
-static ssize_t initstate_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%u\n", init_done(zram));
-}
-
-static ssize_t num_reads_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%llu\n",
-			(u64)atomic64_read(&zram->stats.num_reads));
-}
-
-static ssize_t num_writes_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%llu\n",
-			(u64)atomic64_read(&zram->stats.num_writes));
-}
-
-static ssize_t invalid_io_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%llu\n",
-			(u64)atomic64_read(&zram->stats.invalid_io));
-}
-
-static ssize_t notify_free_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%llu\n",
-			(u64)atomic64_read(&zram->stats.notify_free));
-}
-
-static ssize_t zero_pages_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
-}
-
-static ssize_t orig_data_size_show(struct device *dev,
+static ssize_t memory_used_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
+	u64 val = 0;
 	struct zram *zram = dev_to_zram(dev);
+	struct zram_meta *meta = zram->meta;
 
-	return sprintf(buf, "%llu\n",
-		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
+	down_read(&zram->init_lock);
+	if (init_done(zram))
+		val = zs_get_total_size_bytes(meta->mem_pool);
+	up_read(&zram->init_lock);
+	return sprintf(buf, "%llu\n", val);
 }
 
-static ssize_t compr_data_size_show(struct device *dev,
+static ssize_t disksize_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct zram *zram = dev_to_zram(dev);
-
-	return sprintf(buf, "%llu\n",
-			(u64)atomic64_read(&zram->stats.compr_size));
+	return sprintf(buf, "%llu\n", zram->disksize);
 }
 
-static ssize_t mem_used_total_show(struct device *dev,
+static ssize_t initstate_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	u64 val = 0;
+	u32 val = 0;
 	struct zram *zram = dev_to_zram(dev);
-	struct zram_meta *meta = zram->meta;
-
 	down_read(&zram->init_lock);
-	if (init_done(zram))
-		val = zs_get_total_size_bytes(meta->mem_pool);
+	val = init_done(zram);
 	up_read(&zram->init_lock);
-
-	return sprintf(buf, "%llu\n", val);
+	return sprintf(buf, "%u\n", val);
 }
 
 /* flag operations needs meta->tb_lock */
@@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
 {
 	struct zram_meta *meta = zram->meta;
 	unsigned long handle = meta->table[index].handle;
-	u16 size = meta->table[index].size;
 
 	if (unlikely(!handle)) {
 		/*
@@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
 		 */
 		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
 			zram_clear_flag(meta, index, ZRAM_ZERO);
-			atomic_dec(&zram->stats.pages_zero);
+			atomic64_dec(&zram->stats.pages_zero);
 		}
 		return;
 	}
 
-	if (unlikely(size > max_zpage_size))
-		atomic_dec(&zram->stats.bad_compress);
-
 	zs_free(meta->mem_pool, handle);
 
-	if (size <= PAGE_SIZE / 2)
-		atomic_dec(&zram->stats.good_compress);
-
-	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
-	atomic_dec(&zram->stats.pages_stored);
+	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
+	atomic64_dec(&zram->stats.pages_stored);
 
 	meta->table[index].handle = 0;
 	meta->table[index].size = 0;
@@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 			  u32 index, int offset, struct bio *bio)
 {
-	int ret;
+	int ret = -EINVAL;
 	struct page *page;
 	unsigned char *user_mem, *uncmem = NULL;
 	struct zram_meta *meta = zram->meta;
@@ -406,6 +349,8 @@ out_cleanup:
 	kunmap_atomic(user_mem);
 	if (is_partial_io(bvec))
 		kfree(uncmem);
+	if (ret)
+		atomic64_inc(&zram->stats.failed_reads);
 	return ret;
 }
 
@@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		zram_set_flag(meta, index, ZRAM_ZERO);
 		write_unlock(&zram->meta->tb_lock);
 
-		atomic_inc(&zram->stats.pages_zero);
+		atomic64_inc(&zram->stats.pages_zero);
 		ret = 0;
 		goto out;
 	}
@@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	}
 
 	if (unlikely(clen > max_zpage_size)) {
-		atomic_inc(&zram->stats.bad_compress);
 		clen = PAGE_SIZE;
 		src = NULL;
 		if (is_partial_io(bvec))
@@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	write_unlock(&zram->meta->tb_lock);
 
 	/* Update stats */
-	atomic64_add(clen, &zram->stats.compr_size);
-	atomic_inc(&zram->stats.pages_stored);
-	if (clen <= PAGE_SIZE / 2)
-		atomic_inc(&zram->stats.good_compress);
-
+	atomic64_add(clen, &zram->stats.compressed_size);
+	atomic64_inc(&zram->stats.pages_stored);
 out:
 	if (locked)
 		mutex_unlock(&meta->buffer_lock);
@@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
 
 static void zram_init_device(struct zram *zram, struct zram_meta *meta)
 {
-	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
-		pr_info(
-		"There is little point creating a zram of greater than "
-		"twice the size of memory since we expect a 2:1 compression "
-		"ratio. Note that zram uses about 0.1%% of the size of "
-		"the disk when not in use so a huge zram is "
-		"wasteful.\n"
-		"\tMemory Size: %lu kB\n"
-		"\tSize you selected: %llu kB\n"
-		"Continuing anyway ...\n",
-		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
-		);
-	}
-
 	/* zram devices sort of resembles non-rotational disks */
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
-
 	zram->meta = meta;
 	pr_debug("Initialization done!\n");
 }
@@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
 		disksize_show, disksize_store);
 static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
 static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
-static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
-static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
-static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
-static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
-static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
-static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
-static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
-static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
+static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
+
+ZRAM_ATTR_RO(num_reads);
+ZRAM_ATTR_RO(num_writes);
+ZRAM_ATTR_RO(pages_stored);
+ZRAM_ATTR_RO(invalid_io);
+ZRAM_ATTR_RO(notify_free);
+ZRAM_ATTR_RO(pages_zero);
+ZRAM_ATTR_RO(compressed_size);
 
 static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_disksize.attr,
@@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_reset.attr,
 	&dev_attr_num_reads.attr,
 	&dev_attr_num_writes.attr,
+	&dev_attr_pages_stored.attr,
 	&dev_attr_invalid_io.attr,
 	&dev_attr_notify_free.attr,
-	&dev_attr_zero_pages.attr,
-	&dev_attr_orig_data_size.attr,
-	&dev_attr_compr_data_size.attr,
-	&dev_attr_mem_used_total.attr,
+	&dev_attr_pages_zero.attr,
+	&dev_attr_compressed_size.attr,
+	&dev_attr_memory_used.attr,
 	NULL,
 };
 
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index e81e9cd..277023d 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -64,22 +64,19 @@ enum zram_pageflags {
 struct table {
 	unsigned long handle;
 	u16 size;	/* object size (excluding header) */
-	u8 count;	/* object ref count (not yet used) */
-	u8 flags;
+	u16 flags;
 } __aligned(4);
 
 struct zram_stats {
-	atomic64_t compr_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 num_writes;	/* number of writes */
+	atomic64_t num_reads;	/* number of reads */
+	atomic64_t pages_stored;	/* no. of pages currently stored */
+	atomic64_t compressed_size;	/* compressed size of pages stored */
+	atomic64_t pages_zero;		/* no. of zero filled pages */
+	atomic64_t failed_reads;	/* no. of failed reads */
 	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 */
-	atomic_t pages_zero;		/* no. of zero filled pages */
-	atomic_t pages_stored;	/* no. of pages currently stored */
-	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
-	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
 };
 
 struct zram_meta {
-- 
1.8.5.2.487.g7559984

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> `show' functions and reduce code duplication.
> 
> 2) Account and report back to user numbers of failed READ and WRITE
> operations.
> 
> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> cause a number of RW sub-requests. zram used to account `good' compressed
> sub-queries (with compressed size less than 50% of original size), `bad'
> compressed sub-queries (with compressed size greater that 75% of original
> size), leaving sub-requests with compression size between 50% and 75% of
> original size not accounted and not reported.

That's weird: good/bad_compress are accounted, but it seems to me that
they are to never used in any way. If so, there is indeed no reason to
keep them.

> Account each sub-request
> compression size so we can calculate real device compression ratio.

Your patch doesn't change the way pages_stored and compr[essed]_size
are accounted. What does your patch change that allow us to calculate
the "real" compression ratio?

> 
> 4) reported zram stats:
>   - num_writes  -- number of writes
>   - num_reads  -- number of reads
>   - pages_stored -- number of pages currently stored
>   - compressed_size -- compressed size of pages stored

Wouldn't it be more practical to report the original and compressed
data sizes using the same units as it is currently done?

Jerome

>   - pages_zero  -- number of zero filled pages
>   - failed_read -- number of failed reads
>   - failed_writes -- can happen when memory is too low
>   - invalid_io   -- non-page-aligned I/O requests
>   - notify_free  -- number of swap slot free notifications
>   - memory_used -- zs pool zs_get_total_size_bytes()
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
>  drivers/block/zram/zram_drv.h |  17 ++---
>  2 files changed, 54 insertions(+), 130 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 2a7682c..8bddaff 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +#define ZRAM_ATTR_RO(name)						\
> +static ssize_t zram_attr_##name##_show(struct device *d,		\
> +				struct device_attribute *attr, char *b)	\
> +{									\
> +	struct zram *zram = dev_to_zram(d);				\
> +	return sprintf(b, "%llu\n",					\
> +		(u64)atomic64_read(&zram->stats.name));			\
> +}									\
> +static struct device_attribute dev_attr_##name =			\
> +	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> +
>  static inline int init_done(struct zram *zram)
>  {
>  	return zram->meta != NULL;
> @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
>  	return (struct zram *)dev_to_disk(dev)->private_data;
>  }
>  
> -static ssize_t disksize_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n", zram->disksize);
> -}
> -
> -static ssize_t initstate_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%u\n", init_done(zram));
> -}
> -
> -static ssize_t num_reads_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.num_reads));
> -}
> -
> -static ssize_t num_writes_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.num_writes));
> -}
> -
> -static ssize_t invalid_io_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.invalid_io));
> -}
> -
> -static ssize_t notify_free_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.notify_free));
> -}
> -
> -static ssize_t zero_pages_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
> -}
> -
> -static ssize_t orig_data_size_show(struct device *dev,
> +static ssize_t memory_used_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> +	u64 val = 0;
>  	struct zram *zram = dev_to_zram(dev);
> +	struct zram_meta *meta = zram->meta;
>  
> -	return sprintf(buf, "%llu\n",
> -		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
> +	down_read(&zram->init_lock);
> +	if (init_done(zram))
> +		val = zs_get_total_size_bytes(meta->mem_pool);
> +	up_read(&zram->init_lock);
> +	return sprintf(buf, "%llu\n", val);
>  }
>  
> -static ssize_t compr_data_size_show(struct device *dev,
> +static ssize_t disksize_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.compr_size));
> +	return sprintf(buf, "%llu\n", zram->disksize);
>  }
>  
> -static ssize_t mem_used_total_show(struct device *dev,
> +static ssize_t initstate_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	u64 val = 0;
> +	u32 val = 0;
>  	struct zram *zram = dev_to_zram(dev);
> -	struct zram_meta *meta = zram->meta;
> -
>  	down_read(&zram->init_lock);
> -	if (init_done(zram))
> -		val = zs_get_total_size_bytes(meta->mem_pool);
> +	val = init_done(zram);
>  	up_read(&zram->init_lock);
> -
> -	return sprintf(buf, "%llu\n", val);
> +	return sprintf(buf, "%u\n", val);
>  }
>  
>  /* flag operations needs meta->tb_lock */
> @@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
>  {
>  	struct zram_meta *meta = zram->meta;
>  	unsigned long handle = meta->table[index].handle;
> -	u16 size = meta->table[index].size;
>  
>  	if (unlikely(!handle)) {
>  		/*
> @@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
>  		 */
>  		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
>  			zram_clear_flag(meta, index, ZRAM_ZERO);
> -			atomic_dec(&zram->stats.pages_zero);
> +			atomic64_dec(&zram->stats.pages_zero);
>  		}
>  		return;
>  	}
>  
> -	if (unlikely(size > max_zpage_size))
> -		atomic_dec(&zram->stats.bad_compress);
> -
>  	zs_free(meta->mem_pool, handle);
>  
> -	if (size <= PAGE_SIZE / 2)
> -		atomic_dec(&zram->stats.good_compress);
> -
> -	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
> -	atomic_dec(&zram->stats.pages_stored);
> +	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
> +	atomic64_dec(&zram->stats.pages_stored);
>  
>  	meta->table[index].handle = 0;
>  	meta->table[index].size = 0;
> @@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
>  			  u32 index, int offset, struct bio *bio)
>  {
> -	int ret;
> +	int ret = -EINVAL;
>  	struct page *page;
>  	unsigned char *user_mem, *uncmem = NULL;
>  	struct zram_meta *meta = zram->meta;
> @@ -406,6 +349,8 @@ out_cleanup:
>  	kunmap_atomic(user_mem);
>  	if (is_partial_io(bvec))
>  		kfree(uncmem);
> +	if (ret)
> +		atomic64_inc(&zram->stats.failed_reads);
>  	return ret;
>  }
>  
> @@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		zram_set_flag(meta, index, ZRAM_ZERO);
>  		write_unlock(&zram->meta->tb_lock);
>  
> -		atomic_inc(&zram->stats.pages_zero);
> +		atomic64_inc(&zram->stats.pages_zero);
>  		ret = 0;
>  		goto out;
>  	}
> @@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	}
>  
>  	if (unlikely(clen > max_zpage_size)) {
> -		atomic_inc(&zram->stats.bad_compress);
>  		clen = PAGE_SIZE;
>  		src = NULL;
>  		if (is_partial_io(bvec))
> @@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	write_unlock(&zram->meta->tb_lock);
>  
>  	/* Update stats */
> -	atomic64_add(clen, &zram->stats.compr_size);
> -	atomic_inc(&zram->stats.pages_stored);
> -	if (clen <= PAGE_SIZE / 2)
> -		atomic_inc(&zram->stats.good_compress);
> -
> +	atomic64_add(clen, &zram->stats.compressed_size);
> +	atomic64_inc(&zram->stats.pages_stored);
>  out:
>  	if (locked)
>  		mutex_unlock(&meta->buffer_lock);
> @@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>  
>  static void zram_init_device(struct zram *zram, struct zram_meta *meta)
>  {
> -	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> -		pr_info(
> -		"There is little point creating a zram of greater than "
> -		"twice the size of memory since we expect a 2:1 compression "
> -		"ratio. Note that zram uses about 0.1%% of the size of "
> -		"the disk when not in use so a huge zram is "
> -		"wasteful.\n"
> -		"\tMemory Size: %lu kB\n"
> -		"\tSize you selected: %llu kB\n"
> -		"Continuing anyway ...\n",
> -		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
> -		);
> -	}
> -
>  	/* zram devices sort of resembles non-rotational disks */
>  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> -
>  	zram->meta = meta;
>  	pr_debug("Initialization done!\n");
>  }
> @@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
>  		disksize_show, disksize_store);
>  static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
>  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> -static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
> -static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
> -static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
> -static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
> -static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
> -static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> -static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
> -static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> +static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
> +
> +ZRAM_ATTR_RO(num_reads);
> +ZRAM_ATTR_RO(num_writes);
> +ZRAM_ATTR_RO(pages_stored);
> +ZRAM_ATTR_RO(invalid_io);
> +ZRAM_ATTR_RO(notify_free);
> +ZRAM_ATTR_RO(pages_zero);
> +ZRAM_ATTR_RO(compressed_size);
>  
>  static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_disksize.attr,
> @@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_reset.attr,
>  	&dev_attr_num_reads.attr,
>  	&dev_attr_num_writes.attr,
> +	&dev_attr_pages_stored.attr,
>  	&dev_attr_invalid_io.attr,
>  	&dev_attr_notify_free.attr,
> -	&dev_attr_zero_pages.attr,
> -	&dev_attr_orig_data_size.attr,
> -	&dev_attr_compr_data_size.attr,
> -	&dev_attr_mem_used_total.attr,
> +	&dev_attr_pages_zero.attr,
> +	&dev_attr_compressed_size.attr,
> +	&dev_attr_memory_used.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index e81e9cd..277023d 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -64,22 +64,19 @@ enum zram_pageflags {
>  struct table {
>  	unsigned long handle;
>  	u16 size;	/* object size (excluding header) */
> -	u8 count;	/* object ref count (not yet used) */
> -	u8 flags;
> +	u16 flags;
>  } __aligned(4);
>  
>  struct zram_stats {
> -	atomic64_t compr_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 num_writes;	/* number of writes */
> +	atomic64_t num_reads;	/* number of reads */
> +	atomic64_t pages_stored;	/* no. of pages currently stored */
> +	atomic64_t compressed_size;	/* compressed size of pages stored */
> +	atomic64_t pages_zero;		/* no. of zero filled pages */
> +	atomic64_t failed_reads;	/* no. of failed reads */
>  	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 */
> -	atomic_t pages_zero;		/* no. of zero filled pages */
> -	atomic_t pages_stored;	/* no. of pages currently stored */
> -	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
> -	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
>  };
>  
>  struct zram_meta {
> 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

Hello Jerome,

On (01/14/14 11:38), Jerome Marchand wrote:
> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> > 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> > `show' functions and reduce code duplication.
> > 
> > 2) Account and report back to user numbers of failed READ and WRITE
> > operations.
> > 
> > 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> > cause a number of RW sub-requests. zram used to account `good' compressed
> > sub-queries (with compressed size less than 50% of original size), `bad'
> > compressed sub-queries (with compressed size greater that 75% of original
> > size), leaving sub-requests with compression size between 50% and 75% of
> > original size not accounted and not reported.
> 
> That's weird: good/bad_compress are accounted, but it seems to me that
> they are to never used in any way. If so, there is indeed no reason to
> keep them.
> 
>
> > Account each sub-request
> > compression size so we can calculate real device compression ratio.
> 
> Your patch doesn't change the way pages_stored and compr[essed]_size
> are accounted. What does your patch change that allow us to calculate
> the "real" compression ratio?

we have compressed size, number of stored pages and reported by zs pool
(as a zram memory_used attr) number of bytes used

u64 zs_get_total_size_bytes(struct zs_pool *pool)
{
        int i;
        u64 npages = 0;

        for (i = 0; i < ZS_SIZE_CLASSES; i++)
                npages += pool->size_class[i].pages_allocated;

        return npages << PAGE_SHIFT;
}

looks enough to calculate device overall data compression ratio.

	-ss

> > 
> > 4) reported zram stats:
> >   - num_writes  -- number of writes
> >   - num_reads  -- number of reads
> >   - pages_stored -- number of pages currently stored
> >   - compressed_size -- compressed size of pages stored
> 
> Wouldn't it be more practical to report the original and compressed
> data sizes using the same units as it is currently done?
> 

sorry, not sure I understand.

> Jerome
> 
> >   - pages_zero  -- number of zero filled pages
> >   - failed_read -- number of failed reads
> >   - failed_writes -- can happen when memory is too low
> >   - invalid_io   -- non-page-aligned I/O requests
> >   - notify_free  -- number of swap slot free notifications
> >   - memory_used -- zs pool zs_get_total_size_bytes()
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
> >  drivers/block/zram/zram_drv.h |  17 ++---
> >  2 files changed, 54 insertions(+), 130 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 2a7682c..8bddaff 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_ATTR_RO(name)						\
> > +static ssize_t zram_attr_##name##_show(struct device *d,		\
> > +				struct device_attribute *attr, char *b)	\
> > +{									\
> > +	struct zram *zram = dev_to_zram(d);				\
> > +	return sprintf(b, "%llu\n",					\
> > +		(u64)atomic64_read(&zram->stats.name));			\
> > +}									\
> > +static struct device_attribute dev_attr_##name =			\
> > +	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> > +
> >  static inline int init_done(struct zram *zram)
> >  {
> >  	return zram->meta != NULL;
> > @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
> >  	return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> >  
> > -static ssize_t disksize_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n", zram->disksize);
> > -}
> > -
> > -static ssize_t initstate_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%u\n", init_done(zram));
> > -}
> > -
> > -static ssize_t num_reads_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.num_reads));
> > -}
> > -
> > -static ssize_t num_writes_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.num_writes));
> > -}
> > -
> > -static ssize_t invalid_io_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.invalid_io));
> > -}
> > -
> > -static ssize_t notify_free_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.notify_free));
> > -}
> > -
> > -static ssize_t zero_pages_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
> > -}
> > -
> > -static ssize_t orig_data_size_show(struct device *dev,
> > +static ssize_t memory_used_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > +	u64 val = 0;
> >  	struct zram *zram = dev_to_zram(dev);
> > +	struct zram_meta *meta = zram->meta;
> >  
> > -	return sprintf(buf, "%llu\n",
> > -		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
> > +	down_read(&zram->init_lock);
> > +	if (init_done(zram))
> > +		val = zs_get_total_size_bytes(meta->mem_pool);
> > +	up_read(&zram->init_lock);
> > +	return sprintf(buf, "%llu\n", val);
> >  }
> >  
> > -static ssize_t compr_data_size_show(struct device *dev,
> > +static ssize_t disksize_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> >  	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.compr_size));
> > +	return sprintf(buf, "%llu\n", zram->disksize);
> >  }
> >  
> > -static ssize_t mem_used_total_show(struct device *dev,
> > +static ssize_t initstate_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > -	u64 val = 0;
> > +	u32 val = 0;
> >  	struct zram *zram = dev_to_zram(dev);
> > -	struct zram_meta *meta = zram->meta;
> > -
> >  	down_read(&zram->init_lock);
> > -	if (init_done(zram))
> > -		val = zs_get_total_size_bytes(meta->mem_pool);
> > +	val = init_done(zram);
> >  	up_read(&zram->init_lock);
> > -
> > -	return sprintf(buf, "%llu\n", val);
> > +	return sprintf(buf, "%u\n", val);
> >  }
> >  
> >  /* flag operations needs meta->tb_lock */
> > @@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
> >  {
> >  	struct zram_meta *meta = zram->meta;
> >  	unsigned long handle = meta->table[index].handle;
> > -	u16 size = meta->table[index].size;
> >  
> >  	if (unlikely(!handle)) {
> >  		/*
> > @@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
> >  		 */
> >  		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
> >  			zram_clear_flag(meta, index, ZRAM_ZERO);
> > -			atomic_dec(&zram->stats.pages_zero);
> > +			atomic64_dec(&zram->stats.pages_zero);
> >  		}
> >  		return;
> >  	}
> >  
> > -	if (unlikely(size > max_zpage_size))
> > -		atomic_dec(&zram->stats.bad_compress);
> > -
> >  	zs_free(meta->mem_pool, handle);
> >  
> > -	if (size <= PAGE_SIZE / 2)
> > -		atomic_dec(&zram->stats.good_compress);
> > -
> > -	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
> > -	atomic_dec(&zram->stats.pages_stored);
> > +	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
> > +	atomic64_dec(&zram->stats.pages_stored);
> >  
> >  	meta->table[index].handle = 0;
> >  	meta->table[index].size = 0;
> > @@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> >  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> >  			  u32 index, int offset, struct bio *bio)
> >  {
> > -	int ret;
> > +	int ret = -EINVAL;
> >  	struct page *page;
> >  	unsigned char *user_mem, *uncmem = NULL;
> >  	struct zram_meta *meta = zram->meta;
> > @@ -406,6 +349,8 @@ out_cleanup:
> >  	kunmap_atomic(user_mem);
> >  	if (is_partial_io(bvec))
> >  		kfree(uncmem);
> > +	if (ret)
> > +		atomic64_inc(&zram->stats.failed_reads);
> >  	return ret;
> >  }
> >  
> > @@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  		zram_set_flag(meta, index, ZRAM_ZERO);
> >  		write_unlock(&zram->meta->tb_lock);
> >  
> > -		atomic_inc(&zram->stats.pages_zero);
> > +		atomic64_inc(&zram->stats.pages_zero);
> >  		ret = 0;
> >  		goto out;
> >  	}
> > @@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  	}
> >  
> >  	if (unlikely(clen > max_zpage_size)) {
> > -		atomic_inc(&zram->stats.bad_compress);
> >  		clen = PAGE_SIZE;
> >  		src = NULL;
> >  		if (is_partial_io(bvec))
> > @@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  	write_unlock(&zram->meta->tb_lock);
> >  
> >  	/* Update stats */
> > -	atomic64_add(clen, &zram->stats.compr_size);
> > -	atomic_inc(&zram->stats.pages_stored);
> > -	if (clen <= PAGE_SIZE / 2)
> > -		atomic_inc(&zram->stats.good_compress);
> > -
> > +	atomic64_add(clen, &zram->stats.compressed_size);
> > +	atomic64_inc(&zram->stats.pages_stored);
> >  out:
> >  	if (locked)
> >  		mutex_unlock(&meta->buffer_lock);
> > @@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> >  
> >  static void zram_init_device(struct zram *zram, struct zram_meta *meta)
> >  {
> > -	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> > -		pr_info(
> > -		"There is little point creating a zram of greater than "
> > -		"twice the size of memory since we expect a 2:1 compression "
> > -		"ratio. Note that zram uses about 0.1%% of the size of "
> > -		"the disk when not in use so a huge zram is "
> > -		"wasteful.\n"
> > -		"\tMemory Size: %lu kB\n"
> > -		"\tSize you selected: %llu kB\n"
> > -		"Continuing anyway ...\n",
> > -		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
> > -		);
> > -	}
> > -
> >  	/* zram devices sort of resembles non-rotational disks */
> >  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> > -
> >  	zram->meta = meta;
> >  	pr_debug("Initialization done!\n");
> >  }
> > @@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
> >  		disksize_show, disksize_store);
> >  static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
> >  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> > -static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
> > -static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
> > -static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
> > -static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
> > -static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
> > -static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> > -static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
> > -static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> > +static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
> > +
> > +ZRAM_ATTR_RO(num_reads);
> > +ZRAM_ATTR_RO(num_writes);
> > +ZRAM_ATTR_RO(pages_stored);
> > +ZRAM_ATTR_RO(invalid_io);
> > +ZRAM_ATTR_RO(notify_free);
> > +ZRAM_ATTR_RO(pages_zero);
> > +ZRAM_ATTR_RO(compressed_size);
> >  
> >  static struct attribute *zram_disk_attrs[] = {
> >  	&dev_attr_disksize.attr,
> > @@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
> >  	&dev_attr_reset.attr,
> >  	&dev_attr_num_reads.attr,
> >  	&dev_attr_num_writes.attr,
> > +	&dev_attr_pages_stored.attr,
> >  	&dev_attr_invalid_io.attr,
> >  	&dev_attr_notify_free.attr,
> > -	&dev_attr_zero_pages.attr,
> > -	&dev_attr_orig_data_size.attr,
> > -	&dev_attr_compr_data_size.attr,
> > -	&dev_attr_mem_used_total.attr,
> > +	&dev_attr_pages_zero.attr,
> > +	&dev_attr_compressed_size.attr,
> > +	&dev_attr_memory_used.attr,
> >  	NULL,
> >  };
> >  
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index e81e9cd..277023d 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -64,22 +64,19 @@ enum zram_pageflags {
> >  struct table {
> >  	unsigned long handle;
> >  	u16 size;	/* object size (excluding header) */
> > -	u8 count;	/* object ref count (not yet used) */
> > -	u8 flags;
> > +	u16 flags;
> >  } __aligned(4);
> >  
> >  struct zram_stats {
> > -	atomic64_t compr_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 num_writes;	/* number of writes */
> > +	atomic64_t num_reads;	/* number of reads */
> > +	atomic64_t pages_stored;	/* no. of pages currently stored */
> > +	atomic64_t compressed_size;	/* compressed size of pages stored */
> > +	atomic64_t pages_zero;		/* no. of zero filled pages */
> > +	atomic64_t failed_reads;	/* no. of failed reads */
> >  	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 */
> > -	atomic_t pages_zero;		/* no. of zero filled pages */
> > -	atomic_t pages_stored;	/* no. of pages currently stored */
> > -	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
> > -	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
> >  };
> >  
> >  struct zram_meta {
> > 
> 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 11:57 AM, Sergey Senozhatsky wrote:
> 
> Hello Jerome,
> 
> On (01/14/14 11:38), Jerome Marchand wrote:
>> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
>>> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
>>> `show' functions and reduce code duplication.
>>>
>>> 2) Account and report back to user numbers of failed READ and WRITE
>>> operations.
>>>
>>> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
>>> cause a number of RW sub-requests. zram used to account `good' compressed
>>> sub-queries (with compressed size less than 50% of original size), `bad'
>>> compressed sub-queries (with compressed size greater that 75% of original
>>> size), leaving sub-requests with compression size between 50% and 75% of
>>> original size not accounted and not reported.
>>
>> That's weird: good/bad_compress are accounted, but it seems to me that
>> they are to never used in any way. If so, there is indeed no reason to
>> keep them.
>>
>>
>>> Account each sub-request
>>> compression size so we can calculate real device compression ratio.
>>
>> Your patch doesn't change the way pages_stored and compr[essed]_size
>> are accounted. What does your patch change that allow us to calculate
>> the "real" compression ratio?
> 
> we have compressed size, number of stored pages and reported by zs pool
> (as a zram memory_used attr) number of bytes used
> 
> u64 zs_get_total_size_bytes(struct zs_pool *pool)
> {
>         int i;
>         u64 npages = 0;
> 
>         for (i = 0; i < ZS_SIZE_CLASSES; i++)
>                 npages += pool->size_class[i].pages_allocated;
> 
>         return npages << PAGE_SHIFT;
> }
> 
> looks enough to calculate device overall data compression ratio.

Yes. But don't we have all that already without your patch applied?
What does this patch change?

> 
> 	-ss
> 
>>>
>>> 4) reported zram stats:
>>>   - num_writes  -- number of writes
>>>   - num_reads  -- number of reads
>>>   - pages_stored -- number of pages currently stored
>>>   - compressed_size -- compressed size of pages stored
>>
>> Wouldn't it be more practical to report the original and compressed
>> data sizes using the same units as it is currently done?
>>
> 
> sorry, not sure I understand.

Currently users have access to orig_data_size and compr_data_size,
both in bytes. With your patch, they have access to pages_stored in
pages and compressed_size in bytes. I find the current set more
practical.

Jerome

> 
>> Jerome
>>
>>>   - pages_zero  -- number of zero filled pages
>>>   - failed_read -- number of failed reads
>>>   - failed_writes -- can happen when memory is too low
>>>   - invalid_io   -- non-page-aligned I/O requests
>>>   - notify_free  -- number of swap slot free notifications
>>>   - memory_used -- zs pool zs_get_total_size_bytes()
>>>
>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>> ---
>>>  drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
>>>  drivers/block/zram/zram_drv.h |  17 ++---
>>>  2 files changed, 54 insertions(+), 130 deletions(-)
>>>
>>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>>> index 2a7682c..8bddaff 100644
>>> --- a/drivers/block/zram/zram_drv.c
>>> +++ b/drivers/block/zram/zram_drv.c
>>> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
>>>  /* Module params (documentation at end) */
>>>  static unsigned int num_devices = 1;
>>>  
>>> +#define ZRAM_ATTR_RO(name)						\
>>> +static ssize_t zram_attr_##name##_show(struct device *d,		\
>>> +				struct device_attribute *attr, char *b)	\
>>> +{									\
>>> +	struct zram *zram = dev_to_zram(d);				\
>>> +	return sprintf(b, "%llu\n",					\
>>> +		(u64)atomic64_read(&zram->stats.name));			\
>>> +}									\
>>> +static struct device_attribute dev_attr_##name =			\
>>> +	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
>>> +
>>>  static inline int init_done(struct zram *zram)
>>>  {
>>>  	return zram->meta != NULL;
>>> @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
>>>  	return (struct zram *)dev_to_disk(dev)->private_data;
>>>  }
>>>  
>>> -static ssize_t disksize_show(struct device *dev,
>>> -		struct device_attribute *attr, char *buf)
>>> -{
>>> -	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%llu\n", zram->disksize);
>>> -}
>>> -
>>> -static ssize_t initstate_show(struct device *dev,
>>> -		struct device_attribute *attr, char *buf)
>>> -{
>>> -	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%u\n", init_done(zram));
>>> -}
>>> -
>>> -static ssize_t num_reads_show(struct device *dev,
>>> -		struct device_attribute *attr, char *buf)
>>> -{
>>> -	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%llu\n",
>>> -			(u64)atomic64_read(&zram->stats.num_reads));
>>> -}
>>> -
>>> -static ssize_t num_writes_show(struct device *dev,
>>> -		struct device_attribute *attr, char *buf)
>>> -{
>>> -	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%llu\n",
>>> -			(u64)atomic64_read(&zram->stats.num_writes));
>>> -}
>>> -
>>> -static ssize_t invalid_io_show(struct device *dev,
>>> -		struct device_attribute *attr, char *buf)
>>> -{
>>> -	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%llu\n",
>>> -			(u64)atomic64_read(&zram->stats.invalid_io));
>>> -}
>>> -
>>> -static ssize_t notify_free_show(struct device *dev,
>>> -		struct device_attribute *attr, char *buf)
>>> -{
>>> -	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%llu\n",
>>> -			(u64)atomic64_read(&zram->stats.notify_free));
>>> -}
>>> -
>>> -static ssize_t zero_pages_show(struct device *dev,
>>> -		struct device_attribute *attr, char *buf)
>>> -{
>>> -	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
>>> -}
>>> -
>>> -static ssize_t orig_data_size_show(struct device *dev,
>>> +static ssize_t memory_used_show(struct device *dev,
>>>  		struct device_attribute *attr, char *buf)
>>>  {
>>> +	u64 val = 0;
>>>  	struct zram *zram = dev_to_zram(dev);
>>> +	struct zram_meta *meta = zram->meta;
>>>  
>>> -	return sprintf(buf, "%llu\n",
>>> -		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
>>> +	down_read(&zram->init_lock);
>>> +	if (init_done(zram))
>>> +		val = zs_get_total_size_bytes(meta->mem_pool);
>>> +	up_read(&zram->init_lock);
>>> +	return sprintf(buf, "%llu\n", val);
>>>  }
>>>  
>>> -static ssize_t compr_data_size_show(struct device *dev,
>>> +static ssize_t disksize_show(struct device *dev,
>>>  		struct device_attribute *attr, char *buf)
>>>  {
>>>  	struct zram *zram = dev_to_zram(dev);
>>> -
>>> -	return sprintf(buf, "%llu\n",
>>> -			(u64)atomic64_read(&zram->stats.compr_size));
>>> +	return sprintf(buf, "%llu\n", zram->disksize);
>>>  }
>>>  
>>> -static ssize_t mem_used_total_show(struct device *dev,
>>> +static ssize_t initstate_show(struct device *dev,
>>>  		struct device_attribute *attr, char *buf)
>>>  {
>>> -	u64 val = 0;
>>> +	u32 val = 0;
>>>  	struct zram *zram = dev_to_zram(dev);
>>> -	struct zram_meta *meta = zram->meta;
>>> -
>>>  	down_read(&zram->init_lock);
>>> -	if (init_done(zram))
>>> -		val = zs_get_total_size_bytes(meta->mem_pool);
>>> +	val = init_done(zram);
>>>  	up_read(&zram->init_lock);
>>> -
>>> -	return sprintf(buf, "%llu\n", val);
>>> +	return sprintf(buf, "%u\n", val);
>>>  }
>>>  
>>>  /* flag operations needs meta->tb_lock */
>>> @@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
>>>  {
>>>  	struct zram_meta *meta = zram->meta;
>>>  	unsigned long handle = meta->table[index].handle;
>>> -	u16 size = meta->table[index].size;
>>>  
>>>  	if (unlikely(!handle)) {
>>>  		/*
>>> @@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
>>>  		 */
>>>  		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
>>>  			zram_clear_flag(meta, index, ZRAM_ZERO);
>>> -			atomic_dec(&zram->stats.pages_zero);
>>> +			atomic64_dec(&zram->stats.pages_zero);
>>>  		}
>>>  		return;
>>>  	}
>>>  
>>> -	if (unlikely(size > max_zpage_size))
>>> -		atomic_dec(&zram->stats.bad_compress);
>>> -
>>>  	zs_free(meta->mem_pool, handle);
>>>  
>>> -	if (size <= PAGE_SIZE / 2)
>>> -		atomic_dec(&zram->stats.good_compress);
>>> -
>>> -	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
>>> -	atomic_dec(&zram->stats.pages_stored);
>>> +	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
>>> +	atomic64_dec(&zram->stats.pages_stored);
>>>  
>>>  	meta->table[index].handle = 0;
>>>  	meta->table[index].size = 0;
>>> @@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>>>  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
>>>  			  u32 index, int offset, struct bio *bio)
>>>  {
>>> -	int ret;
>>> +	int ret = -EINVAL;
>>>  	struct page *page;
>>>  	unsigned char *user_mem, *uncmem = NULL;
>>>  	struct zram_meta *meta = zram->meta;
>>> @@ -406,6 +349,8 @@ out_cleanup:
>>>  	kunmap_atomic(user_mem);
>>>  	if (is_partial_io(bvec))
>>>  		kfree(uncmem);
>>> +	if (ret)
>>> +		atomic64_inc(&zram->stats.failed_reads);
>>>  	return ret;
>>>  }
>>>  
>>> @@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>>  		zram_set_flag(meta, index, ZRAM_ZERO);
>>>  		write_unlock(&zram->meta->tb_lock);
>>>  
>>> -		atomic_inc(&zram->stats.pages_zero);
>>> +		atomic64_inc(&zram->stats.pages_zero);
>>>  		ret = 0;
>>>  		goto out;
>>>  	}
>>> @@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>>  	}
>>>  
>>>  	if (unlikely(clen > max_zpage_size)) {
>>> -		atomic_inc(&zram->stats.bad_compress);
>>>  		clen = PAGE_SIZE;
>>>  		src = NULL;
>>>  		if (is_partial_io(bvec))
>>> @@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>>  	write_unlock(&zram->meta->tb_lock);
>>>  
>>>  	/* Update stats */
>>> -	atomic64_add(clen, &zram->stats.compr_size);
>>> -	atomic_inc(&zram->stats.pages_stored);
>>> -	if (clen <= PAGE_SIZE / 2)
>>> -		atomic_inc(&zram->stats.good_compress);
>>> -
>>> +	atomic64_add(clen, &zram->stats.compressed_size);
>>> +	atomic64_inc(&zram->stats.pages_stored);
>>>  out:
>>>  	if (locked)
>>>  		mutex_unlock(&meta->buffer_lock);
>>> @@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>>>  
>>>  static void zram_init_device(struct zram *zram, struct zram_meta *meta)
>>>  {
>>> -	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
>>> -		pr_info(
>>> -		"There is little point creating a zram of greater than "
>>> -		"twice the size of memory since we expect a 2:1 compression "
>>> -		"ratio. Note that zram uses about 0.1%% of the size of "
>>> -		"the disk when not in use so a huge zram is "
>>> -		"wasteful.\n"
>>> -		"\tMemory Size: %lu kB\n"
>>> -		"\tSize you selected: %llu kB\n"
>>> -		"Continuing anyway ...\n",
>>> -		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
>>> -		);
>>> -	}
>>> -
>>>  	/* zram devices sort of resembles non-rotational disks */
>>>  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
>>> -
>>>  	zram->meta = meta;
>>>  	pr_debug("Initialization done!\n");
>>>  }
>>> @@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
>>>  		disksize_show, disksize_store);
>>>  static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
>>>  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
>>> -static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
>>> -static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
>>> -static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
>>> -static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
>>> -static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
>>> -static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
>>> -static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
>>> -static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
>>> +static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
>>> +
>>> +ZRAM_ATTR_RO(num_reads);
>>> +ZRAM_ATTR_RO(num_writes);
>>> +ZRAM_ATTR_RO(pages_stored);
>>> +ZRAM_ATTR_RO(invalid_io);
>>> +ZRAM_ATTR_RO(notify_free);
>>> +ZRAM_ATTR_RO(pages_zero);
>>> +ZRAM_ATTR_RO(compressed_size);
>>>  
>>>  static struct attribute *zram_disk_attrs[] = {
>>>  	&dev_attr_disksize.attr,
>>> @@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
>>>  	&dev_attr_reset.attr,
>>>  	&dev_attr_num_reads.attr,
>>>  	&dev_attr_num_writes.attr,
>>> +	&dev_attr_pages_stored.attr,
>>>  	&dev_attr_invalid_io.attr,
>>>  	&dev_attr_notify_free.attr,
>>> -	&dev_attr_zero_pages.attr,
>>> -	&dev_attr_orig_data_size.attr,
>>> -	&dev_attr_compr_data_size.attr,
>>> -	&dev_attr_mem_used_total.attr,
>>> +	&dev_attr_pages_zero.attr,
>>> +	&dev_attr_compressed_size.attr,
>>> +	&dev_attr_memory_used.attr,
>>>  	NULL,
>>>  };
>>>  
>>> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
>>> index e81e9cd..277023d 100644
>>> --- a/drivers/block/zram/zram_drv.h
>>> +++ b/drivers/block/zram/zram_drv.h
>>> @@ -64,22 +64,19 @@ enum zram_pageflags {
>>>  struct table {
>>>  	unsigned long handle;
>>>  	u16 size;	/* object size (excluding header) */
>>> -	u8 count;	/* object ref count (not yet used) */
>>> -	u8 flags;
>>> +	u16 flags;
>>>  } __aligned(4);
>>>  
>>>  struct zram_stats {
>>> -	atomic64_t compr_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 num_writes;	/* number of writes */
>>> +	atomic64_t num_reads;	/* number of reads */
>>> +	atomic64_t pages_stored;	/* no. of pages currently stored */
>>> +	atomic64_t compressed_size;	/* compressed size of pages stored */
>>> +	atomic64_t pages_zero;		/* no. of zero filled pages */
>>> +	atomic64_t failed_reads;	/* no. of failed reads */
>>>  	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 */
>>> -	atomic_t pages_zero;		/* no. of zero filled pages */
>>> -	atomic_t pages_stored;	/* no. of pages currently stored */
>>> -	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
>>> -	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
>>>  };
>>>  
>>>  struct zram_meta {
>>>
>>

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 13:15), Jerome Marchand wrote:
> On 01/14/2014 11:57 AM, Sergey Senozhatsky wrote:
> > 
> > Hello Jerome,
> > 
> > On (01/14/14 11:38), Jerome Marchand wrote:
> >> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> >>> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> >>> `show' functions and reduce code duplication.
> >>>
> >>> 2) Account and report back to user numbers of failed READ and WRITE
> >>> operations.
> >>>
> >>> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> >>> cause a number of RW sub-requests. zram used to account `good' compressed
> >>> sub-queries (with compressed size less than 50% of original size), `bad'
> >>> compressed sub-queries (with compressed size greater that 75% of original
> >>> size), leaving sub-requests with compression size between 50% and 75% of
> >>> original size not accounted and not reported.
> >>
> >> That's weird: good/bad_compress are accounted, but it seems to me that
> >> they are to never used in any way. If so, there is indeed no reason to
> >> keep them.
> >>
> >>
> >>> Account each sub-request
> >>> compression size so we can calculate real device compression ratio.
> >>
> >> Your patch doesn't change the way pages_stored and compr[essed]_size
> >> are accounted. What does your patch change that allow us to calculate
> >> the "real" compression ratio?
> > 
> > we have compressed size, number of stored pages and reported by zs pool
> > (as a zram memory_used attr) number of bytes used
> > 
> > u64 zs_get_total_size_bytes(struct zs_pool *pool)
> > {
> >         int i;
> >         u64 npages = 0;
> > 
> >         for (i = 0; i < ZS_SIZE_CLASSES; i++)
> >                 npages += pool->size_class[i].pages_allocated;
> > 
> >         return npages << PAGE_SHIFT;
> > }
> > 
> > looks enough to calculate device overall data compression ratio.
> 
> Yes. But don't we have all that already without your patch applied?
> What does this patch change?
> 
>

oh. yes, bad wording. the commit message must be "*zram accounts* each
sub-request compression size so we can calculate real device compression
ratio." instead of  "Account each sub-request compression size so we can
calculate real device compression ratio.". otherwise, there is a false
feeling that patch change/introduce this functionality. will re-write
that commit message. sorry.

the patch does not change a lot of things and may be considered mainly as
a clean up patch. it:
-- removes unused and misleading bad/good stats
-- makes some attrs names more readable e.g. mem_used_total to
memory_used, compr_data_size to compressed_size
-- accounts and reports numbers of failed RW requests
-- removes ATTR_show() code duplication using ZRAM_ATTR_RO macro

	-ss

> > 
> > 	-ss
> > 
> >>>
> >>> 4) reported zram stats:
> >>>   - num_writes  -- number of writes
> >>>   - num_reads  -- number of reads
> >>>   - pages_stored -- number of pages currently stored
> >>>   - compressed_size -- compressed size of pages stored
> >>
> >> Wouldn't it be more practical to report the original and compressed
> >> data sizes using the same units as it is currently done?
> >>
> > 
> > sorry, not sure I understand.
> 
> Currently users have access to orig_data_size and compr_data_size,
> both in bytes. With your patch, they have access to pages_stored in
> pages and compressed_size in bytes. I find the current set more
> practical.
> 
> Jerome
> 
> > 
> >> Jerome
> >>
> >>>   - pages_zero  -- number of zero filled pages
> >>>   - failed_read -- number of failed reads
> >>>   - failed_writes -- can happen when memory is too low
> >>>   - invalid_io   -- non-page-aligned I/O requests
> >>>   - notify_free  -- number of swap slot free notifications
> >>>   - memory_used -- zs pool zs_get_total_size_bytes()
> >>>
> >>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >>> ---
> >>>  drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
> >>>  drivers/block/zram/zram_drv.h |  17 ++---
> >>>  2 files changed, 54 insertions(+), 130 deletions(-)
> >>>
> >>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> >>> index 2a7682c..8bddaff 100644
> >>> --- a/drivers/block/zram/zram_drv.c
> >>> +++ b/drivers/block/zram/zram_drv.c
> >>> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >>>  /* Module params (documentation at end) */
> >>>  static unsigned int num_devices = 1;
> >>>  
> >>> +#define ZRAM_ATTR_RO(name)						\
> >>> +static ssize_t zram_attr_##name##_show(struct device *d,		\
> >>> +				struct device_attribute *attr, char *b)	\
> >>> +{									\
> >>> +	struct zram *zram = dev_to_zram(d);				\
> >>> +	return sprintf(b, "%llu\n",					\
> >>> +		(u64)atomic64_read(&zram->stats.name));			\
> >>> +}									\
> >>> +static struct device_attribute dev_attr_##name =			\
> >>> +	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> >>> +
> >>>  static inline int init_done(struct zram *zram)
> >>>  {
> >>>  	return zram->meta != NULL;
> >>> @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
> >>>  	return (struct zram *)dev_to_disk(dev)->private_data;
> >>>  }
> >>>  
> >>> -static ssize_t disksize_show(struct device *dev,
> >>> -		struct device_attribute *attr, char *buf)
> >>> -{
> >>> -	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%llu\n", zram->disksize);
> >>> -}
> >>> -
> >>> -static ssize_t initstate_show(struct device *dev,
> >>> -		struct device_attribute *attr, char *buf)
> >>> -{
> >>> -	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%u\n", init_done(zram));
> >>> -}
> >>> -
> >>> -static ssize_t num_reads_show(struct device *dev,
> >>> -		struct device_attribute *attr, char *buf)
> >>> -{
> >>> -	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%llu\n",
> >>> -			(u64)atomic64_read(&zram->stats.num_reads));
> >>> -}
> >>> -
> >>> -static ssize_t num_writes_show(struct device *dev,
> >>> -		struct device_attribute *attr, char *buf)
> >>> -{
> >>> -	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%llu\n",
> >>> -			(u64)atomic64_read(&zram->stats.num_writes));
> >>> -}
> >>> -
> >>> -static ssize_t invalid_io_show(struct device *dev,
> >>> -		struct device_attribute *attr, char *buf)
> >>> -{
> >>> -	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%llu\n",
> >>> -			(u64)atomic64_read(&zram->stats.invalid_io));
> >>> -}
> >>> -
> >>> -static ssize_t notify_free_show(struct device *dev,
> >>> -		struct device_attribute *attr, char *buf)
> >>> -{
> >>> -	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%llu\n",
> >>> -			(u64)atomic64_read(&zram->stats.notify_free));
> >>> -}
> >>> -
> >>> -static ssize_t zero_pages_show(struct device *dev,
> >>> -		struct device_attribute *attr, char *buf)
> >>> -{
> >>> -	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
> >>> -}
> >>> -
> >>> -static ssize_t orig_data_size_show(struct device *dev,
> >>> +static ssize_t memory_used_show(struct device *dev,
> >>>  		struct device_attribute *attr, char *buf)
> >>>  {
> >>> +	u64 val = 0;
> >>>  	struct zram *zram = dev_to_zram(dev);
> >>> +	struct zram_meta *meta = zram->meta;
> >>>  
> >>> -	return sprintf(buf, "%llu\n",
> >>> -		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
> >>> +	down_read(&zram->init_lock);
> >>> +	if (init_done(zram))
> >>> +		val = zs_get_total_size_bytes(meta->mem_pool);
> >>> +	up_read(&zram->init_lock);
> >>> +	return sprintf(buf, "%llu\n", val);
> >>>  }
> >>>  
> >>> -static ssize_t compr_data_size_show(struct device *dev,
> >>> +static ssize_t disksize_show(struct device *dev,
> >>>  		struct device_attribute *attr, char *buf)
> >>>  {
> >>>  	struct zram *zram = dev_to_zram(dev);
> >>> -
> >>> -	return sprintf(buf, "%llu\n",
> >>> -			(u64)atomic64_read(&zram->stats.compr_size));
> >>> +	return sprintf(buf, "%llu\n", zram->disksize);
> >>>  }
> >>>  
> >>> -static ssize_t mem_used_total_show(struct device *dev,
> >>> +static ssize_t initstate_show(struct device *dev,
> >>>  		struct device_attribute *attr, char *buf)
> >>>  {
> >>> -	u64 val = 0;
> >>> +	u32 val = 0;
> >>>  	struct zram *zram = dev_to_zram(dev);
> >>> -	struct zram_meta *meta = zram->meta;
> >>> -
> >>>  	down_read(&zram->init_lock);
> >>> -	if (init_done(zram))
> >>> -		val = zs_get_total_size_bytes(meta->mem_pool);
> >>> +	val = init_done(zram);
> >>>  	up_read(&zram->init_lock);
> >>> -
> >>> -	return sprintf(buf, "%llu\n", val);
> >>> +	return sprintf(buf, "%u\n", val);
> >>>  }
> >>>  
> >>>  /* flag operations needs meta->tb_lock */
> >>> @@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
> >>>  {
> >>>  	struct zram_meta *meta = zram->meta;
> >>>  	unsigned long handle = meta->table[index].handle;
> >>> -	u16 size = meta->table[index].size;
> >>>  
> >>>  	if (unlikely(!handle)) {
> >>>  		/*
> >>> @@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
> >>>  		 */
> >>>  		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
> >>>  			zram_clear_flag(meta, index, ZRAM_ZERO);
> >>> -			atomic_dec(&zram->stats.pages_zero);
> >>> +			atomic64_dec(&zram->stats.pages_zero);
> >>>  		}
> >>>  		return;
> >>>  	}
> >>>  
> >>> -	if (unlikely(size > max_zpage_size))
> >>> -		atomic_dec(&zram->stats.bad_compress);
> >>> -
> >>>  	zs_free(meta->mem_pool, handle);
> >>>  
> >>> -	if (size <= PAGE_SIZE / 2)
> >>> -		atomic_dec(&zram->stats.good_compress);
> >>> -
> >>> -	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
> >>> -	atomic_dec(&zram->stats.pages_stored);
> >>> +	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
> >>> +	atomic64_dec(&zram->stats.pages_stored);
> >>>  
> >>>  	meta->table[index].handle = 0;
> >>>  	meta->table[index].size = 0;
> >>> @@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> >>>  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> >>>  			  u32 index, int offset, struct bio *bio)
> >>>  {
> >>> -	int ret;
> >>> +	int ret = -EINVAL;
> >>>  	struct page *page;
> >>>  	unsigned char *user_mem, *uncmem = NULL;
> >>>  	struct zram_meta *meta = zram->meta;
> >>> @@ -406,6 +349,8 @@ out_cleanup:
> >>>  	kunmap_atomic(user_mem);
> >>>  	if (is_partial_io(bvec))
> >>>  		kfree(uncmem);
> >>> +	if (ret)
> >>> +		atomic64_inc(&zram->stats.failed_reads);
> >>>  	return ret;
> >>>  }
> >>>  
> >>> @@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >>>  		zram_set_flag(meta, index, ZRAM_ZERO);
> >>>  		write_unlock(&zram->meta->tb_lock);
> >>>  
> >>> -		atomic_inc(&zram->stats.pages_zero);
> >>> +		atomic64_inc(&zram->stats.pages_zero);
> >>>  		ret = 0;
> >>>  		goto out;
> >>>  	}
> >>> @@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >>>  	}
> >>>  
> >>>  	if (unlikely(clen > max_zpage_size)) {
> >>> -		atomic_inc(&zram->stats.bad_compress);
> >>>  		clen = PAGE_SIZE;
> >>>  		src = NULL;
> >>>  		if (is_partial_io(bvec))
> >>> @@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >>>  	write_unlock(&zram->meta->tb_lock);
> >>>  
> >>>  	/* Update stats */
> >>> -	atomic64_add(clen, &zram->stats.compr_size);
> >>> -	atomic_inc(&zram->stats.pages_stored);
> >>> -	if (clen <= PAGE_SIZE / 2)
> >>> -		atomic_inc(&zram->stats.good_compress);
> >>> -
> >>> +	atomic64_add(clen, &zram->stats.compressed_size);
> >>> +	atomic64_inc(&zram->stats.pages_stored);
> >>>  out:
> >>>  	if (locked)
> >>>  		mutex_unlock(&meta->buffer_lock);
> >>> @@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> >>>  
> >>>  static void zram_init_device(struct zram *zram, struct zram_meta *meta)
> >>>  {
> >>> -	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> >>> -		pr_info(
> >>> -		"There is little point creating a zram of greater than "
> >>> -		"twice the size of memory since we expect a 2:1 compression "
> >>> -		"ratio. Note that zram uses about 0.1%% of the size of "
> >>> -		"the disk when not in use so a huge zram is "
> >>> -		"wasteful.\n"
> >>> -		"\tMemory Size: %lu kB\n"
> >>> -		"\tSize you selected: %llu kB\n"
> >>> -		"Continuing anyway ...\n",
> >>> -		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
> >>> -		);
> >>> -	}
> >>> -
> >>>  	/* zram devices sort of resembles non-rotational disks */
> >>>  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> >>> -
> >>>  	zram->meta = meta;
> >>>  	pr_debug("Initialization done!\n");
> >>>  }
> >>> @@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
> >>>  		disksize_show, disksize_store);
> >>>  static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
> >>>  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> >>> -static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
> >>> -static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
> >>> -static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
> >>> -static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
> >>> -static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
> >>> -static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> >>> -static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
> >>> -static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> >>> +static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
> >>> +
> >>> +ZRAM_ATTR_RO(num_reads);
> >>> +ZRAM_ATTR_RO(num_writes);
> >>> +ZRAM_ATTR_RO(pages_stored);
> >>> +ZRAM_ATTR_RO(invalid_io);
> >>> +ZRAM_ATTR_RO(notify_free);
> >>> +ZRAM_ATTR_RO(pages_zero);
> >>> +ZRAM_ATTR_RO(compressed_size);
> >>>  
> >>>  static struct attribute *zram_disk_attrs[] = {
> >>>  	&dev_attr_disksize.attr,
> >>> @@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
> >>>  	&dev_attr_reset.attr,
> >>>  	&dev_attr_num_reads.attr,
> >>>  	&dev_attr_num_writes.attr,
> >>> +	&dev_attr_pages_stored.attr,
> >>>  	&dev_attr_invalid_io.attr,
> >>>  	&dev_attr_notify_free.attr,
> >>> -	&dev_attr_zero_pages.attr,
> >>> -	&dev_attr_orig_data_size.attr,
> >>> -	&dev_attr_compr_data_size.attr,
> >>> -	&dev_attr_mem_used_total.attr,
> >>> +	&dev_attr_pages_zero.attr,
> >>> +	&dev_attr_compressed_size.attr,
> >>> +	&dev_attr_memory_used.attr,
> >>>  	NULL,
> >>>  };
> >>>  
> >>> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> >>> index e81e9cd..277023d 100644
> >>> --- a/drivers/block/zram/zram_drv.h
> >>> +++ b/drivers/block/zram/zram_drv.h
> >>> @@ -64,22 +64,19 @@ enum zram_pageflags {
> >>>  struct table {
> >>>  	unsigned long handle;
> >>>  	u16 size;	/* object size (excluding header) */
> >>> -	u8 count;	/* object ref count (not yet used) */
> >>> -	u8 flags;
> >>> +	u16 flags;
> >>>  } __aligned(4);
> >>>  
> >>>  struct zram_stats {
> >>> -	atomic64_t compr_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 num_writes;	/* number of writes */
> >>> +	atomic64_t num_reads;	/* number of reads */
> >>> +	atomic64_t pages_stored;	/* no. of pages currently stored */
> >>> +	atomic64_t compressed_size;	/* compressed size of pages stored */
> >>> +	atomic64_t pages_zero;		/* no. of zero filled pages */
> >>> +	atomic64_t failed_reads;	/* no. of failed reads */
> >>>  	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 */
> >>> -	atomic_t pages_zero;		/* no. of zero filled pages */
> >>> -	atomic_t pages_stored;	/* no. of pages currently stored */
> >>> -	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
> >>> -	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
> >>>  };
> >>>  
> >>>  struct zram_meta {
> >>>
> >>
> 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 11:38), Jerome Marchand wrote:
> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> > 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> > `show' functions and reduce code duplication.
> > 
> > 2) Account and report back to user numbers of failed READ and WRITE
> > operations.
> > 
> > 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> > cause a number of RW sub-requests. zram used to account `good' compressed
> > sub-queries (with compressed size less than 50% of original size), `bad'
> > compressed sub-queries (with compressed size greater that 75% of original
> > size), leaving sub-requests with compression size between 50% and 75% of
> > original size not accounted and not reported.
> 
> That's weird: good/bad_compress are accounted, but it seems to me that
> they are to never used in any way. If so, there is indeed no reason to
> keep them.
> 
>
> > Account each sub-request
> > compression size so we can calculate real device compression ratio.
> 
> Your patch doesn't change the way pages_stored and compr[essed]_size
> are accounted. What does your patch change that allow us to calculate
> the "real" compression ratio?
> 
> > 
> > 4) reported zram stats:
> >   - num_writes  -- number of writes
> >   - num_reads  -- number of reads
> >   - pages_stored -- number of pages currently stored
> >   - compressed_size -- compressed size of pages stored
> 
> Wouldn't it be more practical to report the original and compressed
> data sizes using the same units as it is currently done?
> 

hm, do we really need pages_stored stats? what kind of unseful information it
shows to end user?.. perhaps, it's better to replace it with accounted passed
bvec->bv_len (as uncompressed_size).

	-ss

> Jerome
> 
> >   - pages_zero  -- number of zero filled pages
> >   - failed_read -- number of failed reads
> >   - failed_writes -- can happen when memory is too low
> >   - invalid_io   -- non-page-aligned I/O requests
> >   - notify_free  -- number of swap slot free notifications
> >   - memory_used -- zs pool zs_get_total_size_bytes()
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
> >  drivers/block/zram/zram_drv.h |  17 ++---
> >  2 files changed, 54 insertions(+), 130 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 2a7682c..8bddaff 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_ATTR_RO(name)						\
> > +static ssize_t zram_attr_##name##_show(struct device *d,		\
> > +				struct device_attribute *attr, char *b)	\
> > +{									\
> > +	struct zram *zram = dev_to_zram(d);				\
> > +	return sprintf(b, "%llu\n",					\
> > +		(u64)atomic64_read(&zram->stats.name));			\
> > +}									\
> > +static struct device_attribute dev_attr_##name =			\
> > +	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> > +
> >  static inline int init_done(struct zram *zram)
> >  {
> >  	return zram->meta != NULL;
> > @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
> >  	return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> >  
> > -static ssize_t disksize_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n", zram->disksize);
> > -}
> > -
> > -static ssize_t initstate_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%u\n", init_done(zram));
> > -}
> > -
> > -static ssize_t num_reads_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.num_reads));
> > -}
> > -
> > -static ssize_t num_writes_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.num_writes));
> > -}
> > -
> > -static ssize_t invalid_io_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.invalid_io));
> > -}
> > -
> > -static ssize_t notify_free_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.notify_free));
> > -}
> > -
> > -static ssize_t zero_pages_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
> > -}
> > -
> > -static ssize_t orig_data_size_show(struct device *dev,
> > +static ssize_t memory_used_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > +	u64 val = 0;
> >  	struct zram *zram = dev_to_zram(dev);
> > +	struct zram_meta *meta = zram->meta;
> >  
> > -	return sprintf(buf, "%llu\n",
> > -		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
> > +	down_read(&zram->init_lock);
> > +	if (init_done(zram))
> > +		val = zs_get_total_size_bytes(meta->mem_pool);
> > +	up_read(&zram->init_lock);
> > +	return sprintf(buf, "%llu\n", val);
> >  }
> >  
> > -static ssize_t compr_data_size_show(struct device *dev,
> > +static ssize_t disksize_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> >  	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.compr_size));
> > +	return sprintf(buf, "%llu\n", zram->disksize);
> >  }
> >  
> > -static ssize_t mem_used_total_show(struct device *dev,
> > +static ssize_t initstate_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > -	u64 val = 0;
> > +	u32 val = 0;
> >  	struct zram *zram = dev_to_zram(dev);
> > -	struct zram_meta *meta = zram->meta;
> > -
> >  	down_read(&zram->init_lock);
> > -	if (init_done(zram))
> > -		val = zs_get_total_size_bytes(meta->mem_pool);
> > +	val = init_done(zram);
> >  	up_read(&zram->init_lock);
> > -
> > -	return sprintf(buf, "%llu\n", val);
> > +	return sprintf(buf, "%u\n", val);
> >  }
> >  
> >  /* flag operations needs meta->tb_lock */
> > @@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
> >  {
> >  	struct zram_meta *meta = zram->meta;
> >  	unsigned long handle = meta->table[index].handle;
> > -	u16 size = meta->table[index].size;
> >  
> >  	if (unlikely(!handle)) {
> >  		/*
> > @@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
> >  		 */
> >  		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
> >  			zram_clear_flag(meta, index, ZRAM_ZERO);
> > -			atomic_dec(&zram->stats.pages_zero);
> > +			atomic64_dec(&zram->stats.pages_zero);
> >  		}
> >  		return;
> >  	}
> >  
> > -	if (unlikely(size > max_zpage_size))
> > -		atomic_dec(&zram->stats.bad_compress);
> > -
> >  	zs_free(meta->mem_pool, handle);
> >  
> > -	if (size <= PAGE_SIZE / 2)
> > -		atomic_dec(&zram->stats.good_compress);
> > -
> > -	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
> > -	atomic_dec(&zram->stats.pages_stored);
> > +	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
> > +	atomic64_dec(&zram->stats.pages_stored);
> >  
> >  	meta->table[index].handle = 0;
> >  	meta->table[index].size = 0;
> > @@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> >  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> >  			  u32 index, int offset, struct bio *bio)
> >  {
> > -	int ret;
> > +	int ret = -EINVAL;
> >  	struct page *page;
> >  	unsigned char *user_mem, *uncmem = NULL;
> >  	struct zram_meta *meta = zram->meta;
> > @@ -406,6 +349,8 @@ out_cleanup:
> >  	kunmap_atomic(user_mem);
> >  	if (is_partial_io(bvec))
> >  		kfree(uncmem);
> > +	if (ret)
> > +		atomic64_inc(&zram->stats.failed_reads);
> >  	return ret;
> >  }
> >  
> > @@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  		zram_set_flag(meta, index, ZRAM_ZERO);
> >  		write_unlock(&zram->meta->tb_lock);
> >  
> > -		atomic_inc(&zram->stats.pages_zero);
> > +		atomic64_inc(&zram->stats.pages_zero);
> >  		ret = 0;
> >  		goto out;
> >  	}
> > @@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  	}
> >  
> >  	if (unlikely(clen > max_zpage_size)) {
> > -		atomic_inc(&zram->stats.bad_compress);
> >  		clen = PAGE_SIZE;
> >  		src = NULL;
> >  		if (is_partial_io(bvec))
> > @@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  	write_unlock(&zram->meta->tb_lock);
> >  
> >  	/* Update stats */
> > -	atomic64_add(clen, &zram->stats.compr_size);
> > -	atomic_inc(&zram->stats.pages_stored);
> > -	if (clen <= PAGE_SIZE / 2)
> > -		atomic_inc(&zram->stats.good_compress);
> > -
> > +	atomic64_add(clen, &zram->stats.compressed_size);
> > +	atomic64_inc(&zram->stats.pages_stored);
> >  out:
> >  	if (locked)
> >  		mutex_unlock(&meta->buffer_lock);
> > @@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> >  
> >  static void zram_init_device(struct zram *zram, struct zram_meta *meta)
> >  {
> > -	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> > -		pr_info(
> > -		"There is little point creating a zram of greater than "
> > -		"twice the size of memory since we expect a 2:1 compression "
> > -		"ratio. Note that zram uses about 0.1%% of the size of "
> > -		"the disk when not in use so a huge zram is "
> > -		"wasteful.\n"
> > -		"\tMemory Size: %lu kB\n"
> > -		"\tSize you selected: %llu kB\n"
> > -		"Continuing anyway ...\n",
> > -		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
> > -		);
> > -	}
> > -
> >  	/* zram devices sort of resembles non-rotational disks */
> >  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> > -
> >  	zram->meta = meta;
> >  	pr_debug("Initialization done!\n");
> >  }
> > @@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
> >  		disksize_show, disksize_store);
> >  static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
> >  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> > -static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
> > -static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
> > -static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
> > -static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
> > -static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
> > -static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> > -static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
> > -static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> > +static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
> > +
> > +ZRAM_ATTR_RO(num_reads);
> > +ZRAM_ATTR_RO(num_writes);
> > +ZRAM_ATTR_RO(pages_stored);
> > +ZRAM_ATTR_RO(invalid_io);
> > +ZRAM_ATTR_RO(notify_free);
> > +ZRAM_ATTR_RO(pages_zero);
> > +ZRAM_ATTR_RO(compressed_size);
> >  
> >  static struct attribute *zram_disk_attrs[] = {
> >  	&dev_attr_disksize.attr,
> > @@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
> >  	&dev_attr_reset.attr,
> >  	&dev_attr_num_reads.attr,
> >  	&dev_attr_num_writes.attr,
> > +	&dev_attr_pages_stored.attr,
> >  	&dev_attr_invalid_io.attr,
> >  	&dev_attr_notify_free.attr,
> > -	&dev_attr_zero_pages.attr,
> > -	&dev_attr_orig_data_size.attr,
> > -	&dev_attr_compr_data_size.attr,
> > -	&dev_attr_mem_used_total.attr,
> > +	&dev_attr_pages_zero.attr,
> > +	&dev_attr_compressed_size.attr,
> > +	&dev_attr_memory_used.attr,
> >  	NULL,
> >  };
> >  
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index e81e9cd..277023d 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -64,22 +64,19 @@ enum zram_pageflags {
> >  struct table {
> >  	unsigned long handle;
> >  	u16 size;	/* object size (excluding header) */
> > -	u8 count;	/* object ref count (not yet used) */
> > -	u8 flags;
> > +	u16 flags;
> >  } __aligned(4);
> >  
> >  struct zram_stats {
> > -	atomic64_t compr_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 num_writes;	/* number of writes */
> > +	atomic64_t num_reads;	/* number of reads */
> > +	atomic64_t pages_stored;	/* no. of pages currently stored */
> > +	atomic64_t compressed_size;	/* compressed size of pages stored */
> > +	atomic64_t pages_zero;		/* no. of zero filled pages */
> > +	atomic64_t failed_reads;	/* no. of failed reads */
> >  	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 */
> > -	atomic_t pages_zero;		/* no. of zero filled pages */
> > -	atomic_t pages_stored;	/* no. of pages currently stored */
> > -	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
> > -	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
> >  };
> >  
> >  struct zram_meta {
> > 
> 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 12:10 PM, Sergey Senozhatsky wrote:
> On (01/14/14 11:38), Jerome Marchand wrote:
>> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
>>> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
>>> `show' functions and reduce code duplication.
>>>
>>> 2) Account and report back to user numbers of failed READ and WRITE
>>> operations.
>>>
>>> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
>>> cause a number of RW sub-requests. zram used to account `good' compressed
>>> sub-queries (with compressed size less than 50% of original size), `bad'
>>> compressed sub-queries (with compressed size greater that 75% of original
>>> size), leaving sub-requests with compression size between 50% and 75% of
>>> original size not accounted and not reported.
>>
>> That's weird: good/bad_compress are accounted, but it seems to me that
>> they are to never used in any way. If so, there is indeed no reason to
>> keep them.
>>
>>
>>> Account each sub-request
>>> compression size so we can calculate real device compression ratio.
>>
>> Your patch doesn't change the way pages_stored and compr[essed]_size
>> are accounted. What does your patch change that allow us to calculate
>> the "real" compression ratio?
>>
>>>
>>> 4) reported zram stats:
>>>   - num_writes  -- number of writes
>>>   - num_reads  -- number of reads
>>>   - pages_stored -- number of pages currently stored
>>>   - compressed_size -- compressed size of pages stored
>>
>> Wouldn't it be more practical to report the original and compressed
>> data sizes using the same units as it is currently done?
>>
> 
> hm, do we really need pages_stored stats? what kind of unseful information it
> shows to end user?.. perhaps, it's better to replace it with accounted passed
> bvec->bv_len (as uncompressed_size).
> 

That's really going to complicates things. We would need to keep track
of which sectors of a particular page has been written to. It's much
easier to keep current page granularity and consider any partial I/O
as an whole page I/O.

> 	-ss
> 
>> Jerome
>>
>>>   - pages_zero  -- number of zero filled pages
>>>   - failed_read -- number of failed reads
>>>   - failed_writes -- can happen when memory is too low
>>>   - invalid_io   -- non-page-aligned I/O requests
>>>   - notify_free  -- number of swap slot free notifications
>>>   - memory_used -- zs pool zs_get_total_size_bytes()
>>>
>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 14:43), Jerome Marchand wrote:
[..]
> >>
> >> That's weird: good/bad_compress are accounted, but it seems to me that
> >> they are to never used in any way. If so, there is indeed no reason to
> >> keep them.
> >>
> >>
> >>> Account each sub-request
> >>> compression size so we can calculate real device compression ratio.
> >>
> >> Your patch doesn't change the way pages_stored and compr[essed]_size
> >> are accounted. What does your patch change that allow us to calculate
> >> the "real" compression ratio?
> >>
> >>>
> >>> 4) reported zram stats:
> >>>   - num_writes  -- number of writes
> >>>   - num_reads  -- number of reads
> >>>   - pages_stored -- number of pages currently stored
> >>>   - compressed_size -- compressed size of pages stored
> >>
> >> Wouldn't it be more practical to report the original and compressed
> >> data sizes using the same units as it is currently done?
> >>
> > 
> > hm, do we really need pages_stored stats? what kind of unseful information it
> > shows to end user?.. perhaps, it's better to replace it with accounted passed
> > bvec->bv_len (as uncompressed_size).
> > 
> 
> That's really going to complicates things. We would need to keep track
> of which sectors of a particular page has been written to. It's much
> easier to keep current page granularity and consider any partial I/O
> as an whole page I/O.
> 

fair enough. thank you.

2/3 and 3/3 were changed according to your comments:
- 2/3 drop READA check
- 3/3 update commit message.

ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?

	-ss

> > 
> >> Jerome
> >>
> >>>   - pages_zero  -- number of zero filled pages
> >>>   - failed_read -- number of failed reads
> >>>   - failed_writes -- can happen when memory is too low
> >>>   - invalid_io   -- non-page-aligned I/O requests
> >>>   - notify_free  -- number of swap slot free notifications
> >>>   - memory_used -- zs pool zs_get_total_size_bytes()
> >>>
> >>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
> On (01/14/14 14:43), Jerome Marchand wrote:
> [..]
>>>>
>>>> That's weird: good/bad_compress are accounted, but it seems to me that
>>>> they are to never used in any way. If so, there is indeed no reason to
>>>> keep them.
>>>>
>>>>
>>>>> Account each sub-request
>>>>> compression size so we can calculate real device compression ratio.
>>>>
>>>> Your patch doesn't change the way pages_stored and compr[essed]_size
>>>> are accounted. What does your patch change that allow us to calculate
>>>> the "real" compression ratio?
>>>>
>>>>>
>>>>> 4) reported zram stats:
>>>>>   - num_writes  -- number of writes
>>>>>   - num_reads  -- number of reads
>>>>>   - pages_stored -- number of pages currently stored
>>>>>   - compressed_size -- compressed size of pages stored
>>>>
>>>> Wouldn't it be more practical to report the original and compressed
>>>> data sizes using the same units as it is currently done?
>>>>
>>>
>>> hm, do we really need pages_stored stats? what kind of unseful information it
>>> shows to end user?.. perhaps, it's better to replace it with accounted passed
>>> bvec->bv_len (as uncompressed_size).
>>>
>>
>> That's really going to complicates things. We would need to keep track
>> of which sectors of a particular page has been written to. It's much
>> easier to keep current page granularity and consider any partial I/O
>> as an whole page I/O.
>>
> 
> fair enough. thank you.
> 
> 2/3 and 3/3 were changed according to your comments:
> - 2/3 drop READA check
> - 3/3 update commit message.
> 
> ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?

The READA thing was my only concern for 2/3, so yes for this one.
Concerning the third patch, I'd like to see what other people think
about which stats we want to report.

> 
> 	-ss
> 
>>>
>>>> Jerome
>>>>
>>>>>   - pages_zero  -- number of zero filled pages
>>>>>   - failed_read -- number of failed reads
>>>>>   - failed_writes -- can happen when memory is too low
>>>>>   - invalid_io   -- non-page-aligned I/O requests
>>>>>   - notify_free  -- number of swap slot free notifications
>>>>>   - memory_used -- zs pool zs_get_total_size_bytes()
>>>>>
>>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>>
>>

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 15:02), Jerome Marchand wrote:
> On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
> > On (01/14/14 14:43), Jerome Marchand wrote:
> > [..]
> >>>>
> >>>> That's weird: good/bad_compress are accounted, but it seems to me that
> >>>> they are to never used in any way. If so, there is indeed no reason to
> >>>> keep them.
> >>>>
> >>>>
> >>>>> Account each sub-request
> >>>>> compression size so we can calculate real device compression ratio.
> >>>>
> >>>> Your patch doesn't change the way pages_stored and compr[essed]_size
> >>>> are accounted. What does your patch change that allow us to calculate
> >>>> the "real" compression ratio?
> >>>>
> >>>>>
> >>>>> 4) reported zram stats:
> >>>>>   - num_writes  -- number of writes
> >>>>>   - num_reads  -- number of reads
> >>>>>   - pages_stored -- number of pages currently stored
> >>>>>   - compressed_size -- compressed size of pages stored
> >>>>
> >>>> Wouldn't it be more practical to report the original and compressed
> >>>> data sizes using the same units as it is currently done?
> >>>>
> >>>
> >>> hm, do we really need pages_stored stats? what kind of unseful information it
> >>> shows to end user?.. perhaps, it's better to replace it with accounted passed
> >>> bvec->bv_len (as uncompressed_size).
> >>>
> >>
> >> That's really going to complicates things. We would need to keep track
> >> of which sectors of a particular page has been written to. It's much
> >> easier to keep current page granularity and consider any partial I/O
> >> as an whole page I/O.
> >>
> > 
> > fair enough. thank you.
> > 
> > 2/3 and 3/3 were changed according to your comments:
> > - 2/3 drop READA check
> > - 3/3 update commit message.
> > 
> > ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?
> 
> The READA thing was my only concern for 2/3, so yes for this one.
> Concerning the third patch, I'd like to see what other people think
> about which stats we want to report.
> 

good. so I hold on for a bit to minimize the traffic and see what other
people think. thank you.

	-ss

> >>>
> >>>> Jerome
> >>>>
> >>>>>   - pages_zero  -- number of zero filled pages
> >>>>>   - failed_read -- number of failed reads
> >>>>>   - failed_writes -- can happen when memory is too low
> >>>>>   - invalid_io   -- non-page-aligned I/O requests
> >>>>>   - notify_free  -- number of swap slot free notifications
> >>>>>   - memory_used -- zs pool zs_get_total_size_bytes()
> >>>>>
> >>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> >>
> >>
> 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 03:09 PM, Sergey Senozhatsky wrote:
> On (01/14/14 15:02), Jerome Marchand wrote:
>> On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
>>> On (01/14/14 14:43), Jerome Marchand wrote:
>>> [..]
>>>>>>
>>>>>> That's weird: good/bad_compress are accounted, but it seems to me that
>>>>>> they are to never used in any way. If so, there is indeed no reason to
>>>>>> keep them.
>>>>>>
>>>>>>
>>>>>>> Account each sub-request
>>>>>>> compression size so we can calculate real device compression ratio.
>>>>>>
>>>>>> Your patch doesn't change the way pages_stored and compr[essed]_size
>>>>>> are accounted. What does your patch change that allow us to calculate
>>>>>> the "real" compression ratio?
>>>>>>
>>>>>>>
>>>>>>> 4) reported zram stats:
>>>>>>>   - num_writes  -- number of writes
>>>>>>>   - num_reads  -- number of reads
>>>>>>>   - pages_stored -- number of pages currently stored
>>>>>>>   - compressed_size -- compressed size of pages stored
>>>>>>
>>>>>> Wouldn't it be more practical to report the original and compressed
>>>>>> data sizes using the same units as it is currently done?
>>>>>>
>>>>>
>>>>> hm, do we really need pages_stored stats? what kind of unseful information it
>>>>> shows to end user?.. perhaps, it's better to replace it with accounted passed
>>>>> bvec->bv_len (as uncompressed_size).
>>>>>
>>>>
>>>> That's really going to complicates things. We would need to keep track
>>>> of which sectors of a particular page has been written to. It's much
>>>> easier to keep current page granularity and consider any partial I/O
>>>> as an whole page I/O.
>>>>
>>>
>>> fair enough. thank you.
>>>
>>> 2/3 and 3/3 were changed according to your comments:
>>> - 2/3 drop READA check
>>> - 3/3 update commit message.
>>>
>>> ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?
>>
>> The READA thing was my only concern for 2/3, so yes for this one.
>> Concerning the third patch, I'd like to see what other people think
>> about which stats we want to report.
>>
> 
> good. so I hold on for a bit to minimize the traffic and see what other
> people think. thank you.
> 
> 	-ss
> 
>>>>>
>>>>>> Jerome
>>>>>>
>>>>>>>   - pages_zero  -- number of zero filled pages
>>>>>>>   - failed_read -- number of failed reads
>>>>>>>   - failed_writes -- can happen when memory is too low
>>>>>>>   - invalid_io   -- non-page-aligned I/O requests
>>>>>>>   - notify_free  -- number of swap slot free notifications
>>>>>>>   - memory_used -- zs pool zs_get_total_size_bytes()

I also wonder if we should add size of meta-data to memory_used,
especially the table size which is probably not always negligible.

Jerome

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

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Minchan Kim]

Hello Sergey,

On Tue, Jan 14, 2014 at 12:37:40PM +0300, Sergey Senozhatsky wrote:
> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> `show' functions and reduce code duplication.
> 
> 2) Account and report back to user numbers of failed READ and WRITE
> operations.
> 
> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> cause a number of RW sub-requests. zram used to account `good' compressed
> sub-queries (with compressed size less than 50% of original size), `bad'
> compressed sub-queries (with compressed size greater that 75% of original
> size), leaving sub-requests with compression size between 50% and 75% of
> original size not accounted and not reported. Account each sub-request
> compression size so we can calculate real device compression ratio.
> 
> 4) reported zram stats:
>   - num_writes  -- number of writes
>   - num_reads  -- number of reads
>   - pages_stored -- number of pages currently stored
>   - compressed_size -- compressed size of pages stored
>   - pages_zero  -- number of zero filled pages
>   - failed_read -- number of failed reads
>   - failed_writes -- can happen when memory is too low
>   - invalid_io   -- non-page-aligned I/O requests
>   - notify_free  -- number of swap slot free notifications
>   - memory_used -- zs pool zs_get_total_size_bytes()
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

So this patch includes some clean up and behavior changes?
Please seaprate them and each patch in behavior change includes
why it's bad or good(ie, motivation).

It could make reviewer/maintainer happy, even you because
some of them could be picked up and other things could be dropped.
Sorry for the bothering you.

Thanks.

> ---
>  drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
>  drivers/block/zram/zram_drv.h |  17 ++---
>  2 files changed, 54 insertions(+), 130 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 2a7682c..8bddaff 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +#define ZRAM_ATTR_RO(name)						\
> +static ssize_t zram_attr_##name##_show(struct device *d,		\
> +				struct device_attribute *attr, char *b)	\
> +{									\
> +	struct zram *zram = dev_to_zram(d);				\
> +	return sprintf(b, "%llu\n",					\
> +		(u64)atomic64_read(&zram->stats.name));			\
> +}									\
> +static struct device_attribute dev_attr_##name =			\
> +	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> +
>  static inline int init_done(struct zram *zram)
>  {
>  	return zram->meta != NULL;
> @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
>  	return (struct zram *)dev_to_disk(dev)->private_data;
>  }
>  
> -static ssize_t disksize_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n", zram->disksize);
> -}
> -
> -static ssize_t initstate_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%u\n", init_done(zram));
> -}
> -
> -static ssize_t num_reads_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.num_reads));
> -}
> -
> -static ssize_t num_writes_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.num_writes));
> -}
> -
> -static ssize_t invalid_io_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.invalid_io));
> -}
> -
> -static ssize_t notify_free_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.notify_free));
> -}
> -
> -static ssize_t zero_pages_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
> -}
> -
> -static ssize_t orig_data_size_show(struct device *dev,
> +static ssize_t memory_used_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> +	u64 val = 0;
>  	struct zram *zram = dev_to_zram(dev);
> +	struct zram_meta *meta = zram->meta;
>  
> -	return sprintf(buf, "%llu\n",
> -		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
> +	down_read(&zram->init_lock);
> +	if (init_done(zram))
> +		val = zs_get_total_size_bytes(meta->mem_pool);
> +	up_read(&zram->init_lock);
> +	return sprintf(buf, "%llu\n", val);
>  }
>  
> -static ssize_t compr_data_size_show(struct device *dev,
> +static ssize_t disksize_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct zram *zram = dev_to_zram(dev);
> -
> -	return sprintf(buf, "%llu\n",
> -			(u64)atomic64_read(&zram->stats.compr_size));
> +	return sprintf(buf, "%llu\n", zram->disksize);
>  }
>  
> -static ssize_t mem_used_total_show(struct device *dev,
> +static ssize_t initstate_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	u64 val = 0;
> +	u32 val = 0;
>  	struct zram *zram = dev_to_zram(dev);
> -	struct zram_meta *meta = zram->meta;
> -
>  	down_read(&zram->init_lock);
> -	if (init_done(zram))
> -		val = zs_get_total_size_bytes(meta->mem_pool);
> +	val = init_done(zram);
>  	up_read(&zram->init_lock);
> -
> -	return sprintf(buf, "%llu\n", val);
> +	return sprintf(buf, "%u\n", val);
>  }
>  
>  /* flag operations needs meta->tb_lock */
> @@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
>  {
>  	struct zram_meta *meta = zram->meta;
>  	unsigned long handle = meta->table[index].handle;
> -	u16 size = meta->table[index].size;
>  
>  	if (unlikely(!handle)) {
>  		/*
> @@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
>  		 */
>  		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
>  			zram_clear_flag(meta, index, ZRAM_ZERO);
> -			atomic_dec(&zram->stats.pages_zero);
> +			atomic64_dec(&zram->stats.pages_zero);
>  		}
>  		return;
>  	}
>  
> -	if (unlikely(size > max_zpage_size))
> -		atomic_dec(&zram->stats.bad_compress);
> -
>  	zs_free(meta->mem_pool, handle);
>  
> -	if (size <= PAGE_SIZE / 2)
> -		atomic_dec(&zram->stats.good_compress);
> -
> -	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
> -	atomic_dec(&zram->stats.pages_stored);
> +	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
> +	atomic64_dec(&zram->stats.pages_stored);
>  
>  	meta->table[index].handle = 0;
>  	meta->table[index].size = 0;
> @@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
>  			  u32 index, int offset, struct bio *bio)
>  {
> -	int ret;
> +	int ret = -EINVAL;
>  	struct page *page;
>  	unsigned char *user_mem, *uncmem = NULL;
>  	struct zram_meta *meta = zram->meta;
> @@ -406,6 +349,8 @@ out_cleanup:
>  	kunmap_atomic(user_mem);
>  	if (is_partial_io(bvec))
>  		kfree(uncmem);
> +	if (ret)
> +		atomic64_inc(&zram->stats.failed_reads);
>  	return ret;
>  }
>  
> @@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		zram_set_flag(meta, index, ZRAM_ZERO);
>  		write_unlock(&zram->meta->tb_lock);
>  
> -		atomic_inc(&zram->stats.pages_zero);
> +		atomic64_inc(&zram->stats.pages_zero);
>  		ret = 0;
>  		goto out;
>  	}
> @@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	}
>  
>  	if (unlikely(clen > max_zpage_size)) {
> -		atomic_inc(&zram->stats.bad_compress);
>  		clen = PAGE_SIZE;
>  		src = NULL;
>  		if (is_partial_io(bvec))
> @@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	write_unlock(&zram->meta->tb_lock);
>  
>  	/* Update stats */
> -	atomic64_add(clen, &zram->stats.compr_size);
> -	atomic_inc(&zram->stats.pages_stored);
> -	if (clen <= PAGE_SIZE / 2)
> -		atomic_inc(&zram->stats.good_compress);
> -
> +	atomic64_add(clen, &zram->stats.compressed_size);
> +	atomic64_inc(&zram->stats.pages_stored);
>  out:
>  	if (locked)
>  		mutex_unlock(&meta->buffer_lock);
> @@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
>  
>  static void zram_init_device(struct zram *zram, struct zram_meta *meta)
>  {
> -	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> -		pr_info(
> -		"There is little point creating a zram of greater than "
> -		"twice the size of memory since we expect a 2:1 compression "
> -		"ratio. Note that zram uses about 0.1%% of the size of "
> -		"the disk when not in use so a huge zram is "
> -		"wasteful.\n"
> -		"\tMemory Size: %lu kB\n"
> -		"\tSize you selected: %llu kB\n"
> -		"Continuing anyway ...\n",
> -		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
> -		);
> -	}
> -
>  	/* zram devices sort of resembles non-rotational disks */
>  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> -
>  	zram->meta = meta;
>  	pr_debug("Initialization done!\n");
>  }
> @@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
>  		disksize_show, disksize_store);
>  static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
>  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> -static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
> -static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
> -static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
> -static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
> -static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
> -static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> -static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
> -static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> +static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
> +
> +ZRAM_ATTR_RO(num_reads);
> +ZRAM_ATTR_RO(num_writes);
> +ZRAM_ATTR_RO(pages_stored);
> +ZRAM_ATTR_RO(invalid_io);
> +ZRAM_ATTR_RO(notify_free);
> +ZRAM_ATTR_RO(pages_zero);
> +ZRAM_ATTR_RO(compressed_size);
>  
>  static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_disksize.attr,
> @@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_reset.attr,
>  	&dev_attr_num_reads.attr,
>  	&dev_attr_num_writes.attr,
> +	&dev_attr_pages_stored.attr,
>  	&dev_attr_invalid_io.attr,
>  	&dev_attr_notify_free.attr,
> -	&dev_attr_zero_pages.attr,
> -	&dev_attr_orig_data_size.attr,
> -	&dev_attr_compr_data_size.attr,
> -	&dev_attr_mem_used_total.attr,
> +	&dev_attr_pages_zero.attr,
> +	&dev_attr_compressed_size.attr,
> +	&dev_attr_memory_used.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index e81e9cd..277023d 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -64,22 +64,19 @@ enum zram_pageflags {
>  struct table {
>  	unsigned long handle;
>  	u16 size;	/* object size (excluding header) */
> -	u8 count;	/* object ref count (not yet used) */
> -	u8 flags;
> +	u16 flags;
>  } __aligned(4);
>  
>  struct zram_stats {
> -	atomic64_t compr_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 num_writes;	/* number of writes */
> +	atomic64_t num_reads;	/* number of reads */
> +	atomic64_t pages_stored;	/* no. of pages currently stored */
> +	atomic64_t compressed_size;	/* compressed size of pages stored */
> +	atomic64_t pages_zero;		/* no. of zero filled pages */
> +	atomic64_t failed_reads;	/* no. of failed reads */
>  	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 */
> -	atomic_t pages_zero;		/* no. of zero filled pages */
> -	atomic_t pages_stored;	/* no. of pages currently stored */
> -	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
> -	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
>  };
>  
>  struct zram_meta {
> -- 
> 1.8.5.2.487.g7559984
> 
> --
> 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 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/15/14 13:24), Minchan Kim wrote:
> Hello Sergey,
> 
> On Tue, Jan 14, 2014 at 12:37:40PM +0300, Sergey Senozhatsky wrote:
> > 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> > `show' functions and reduce code duplication.
> > 
> > 2) Account and report back to user numbers of failed READ and WRITE
> > operations.
> > 
> > 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> > cause a number of RW sub-requests. zram used to account `good' compressed
> > sub-queries (with compressed size less than 50% of original size), `bad'
> > compressed sub-queries (with compressed size greater that 75% of original
> > size), leaving sub-requests with compression size between 50% and 75% of
> > original size not accounted and not reported. Account each sub-request
> > compression size so we can calculate real device compression ratio.
> > 
> > 4) reported zram stats:
> >   - num_writes  -- number of writes
> >   - num_reads  -- number of reads
> >   - pages_stored -- number of pages currently stored
> >   - compressed_size -- compressed size of pages stored
> >   - pages_zero  -- number of zero filled pages
> >   - failed_read -- number of failed reads
> >   - failed_writes -- can happen when memory is too low
> >   - invalid_io   -- non-page-aligned I/O requests
> >   - notify_free  -- number of swap slot free notifications
> >   - memory_used -- zs pool zs_get_total_size_bytes()
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
>

Hello Minchan,
I wouldn't say that it changes behaviour. The only things that is new
is that zram now accounts failed RW requests. the rest is a part of
clean up.

	-ss

> So this patch includes some clean up and behavior changes?
> Please seaprate them and each patch in behavior change includes
> why it's bad or good(ie, motivation).
> 
> It could make reviewer/maintainer happy, even you because
> some of them could be picked up and other things could be dropped.
> Sorry for the bothering you.
> 
> Thanks.
> 
> > ---
> >  drivers/block/zram/zram_drv.c | 167 ++++++++++++------------------------------
> >  drivers/block/zram/zram_drv.h |  17 ++---
> >  2 files changed, 54 insertions(+), 130 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 2a7682c..8bddaff 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_ATTR_RO(name)						\
> > +static ssize_t zram_attr_##name##_show(struct device *d,		\
> > +				struct device_attribute *attr, char *b)	\
> > +{									\
> > +	struct zram *zram = dev_to_zram(d);				\
> > +	return sprintf(b, "%llu\n",					\
> > +		(u64)atomic64_read(&zram->stats.name));			\
> > +}									\
> > +static struct device_attribute dev_attr_##name =			\
> > +	__ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> > +
> >  static inline int init_done(struct zram *zram)
> >  {
> >  	return zram->meta != NULL;
> > @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
> >  	return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> >  
> > -static ssize_t disksize_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n", zram->disksize);
> > -}
> > -
> > -static ssize_t initstate_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%u\n", init_done(zram));
> > -}
> > -
> > -static ssize_t num_reads_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.num_reads));
> > -}
> > -
> > -static ssize_t num_writes_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.num_writes));
> > -}
> > -
> > -static ssize_t invalid_io_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.invalid_io));
> > -}
> > -
> > -static ssize_t notify_free_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.notify_free));
> > -}
> > -
> > -static ssize_t zero_pages_show(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%u\n", atomic_read(&zram->stats.pages_zero));
> > -}
> > -
> > -static ssize_t orig_data_size_show(struct device *dev,
> > +static ssize_t memory_used_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > +	u64 val = 0;
> >  	struct zram *zram = dev_to_zram(dev);
> > +	struct zram_meta *meta = zram->meta;
> >  
> > -	return sprintf(buf, "%llu\n",
> > -		(u64)(atomic_read(&zram->stats.pages_stored)) << PAGE_SHIFT);
> > +	down_read(&zram->init_lock);
> > +	if (init_done(zram))
> > +		val = zs_get_total_size_bytes(meta->mem_pool);
> > +	up_read(&zram->init_lock);
> > +	return sprintf(buf, "%llu\n", val);
> >  }
> >  
> > -static ssize_t compr_data_size_show(struct device *dev,
> > +static ssize_t disksize_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> >  	struct zram *zram = dev_to_zram(dev);
> > -
> > -	return sprintf(buf, "%llu\n",
> > -			(u64)atomic64_read(&zram->stats.compr_size));
> > +	return sprintf(buf, "%llu\n", zram->disksize);
> >  }
> >  
> > -static ssize_t mem_used_total_show(struct device *dev,
> > +static ssize_t initstate_show(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > -	u64 val = 0;
> > +	u32 val = 0;
> >  	struct zram *zram = dev_to_zram(dev);
> > -	struct zram_meta *meta = zram->meta;
> > -
> >  	down_read(&zram->init_lock);
> > -	if (init_done(zram))
> > -		val = zs_get_total_size_bytes(meta->mem_pool);
> > +	val = init_done(zram);
> >  	up_read(&zram->init_lock);
> > -
> > -	return sprintf(buf, "%llu\n", val);
> > +	return sprintf(buf, "%u\n", val);
> >  }
> >  
> >  /* flag operations needs meta->tb_lock */
> > @@ -293,7 +243,6 @@ static void zram_free_page(struct zram *zram, size_t index)
> >  {
> >  	struct zram_meta *meta = zram->meta;
> >  	unsigned long handle = meta->table[index].handle;
> > -	u16 size = meta->table[index].size;
> >  
> >  	if (unlikely(!handle)) {
> >  		/*
> > @@ -302,21 +251,15 @@ static void zram_free_page(struct zram *zram, size_t index)
> >  		 */
> >  		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
> >  			zram_clear_flag(meta, index, ZRAM_ZERO);
> > -			atomic_dec(&zram->stats.pages_zero);
> > +			atomic64_dec(&zram->stats.pages_zero);
> >  		}
> >  		return;
> >  	}
> >  
> > -	if (unlikely(size > max_zpage_size))
> > -		atomic_dec(&zram->stats.bad_compress);
> > -
> >  	zs_free(meta->mem_pool, handle);
> >  
> > -	if (size <= PAGE_SIZE / 2)
> > -		atomic_dec(&zram->stats.good_compress);
> > -
> > -	atomic64_sub(meta->table[index].size, &zram->stats.compr_size);
> > -	atomic_dec(&zram->stats.pages_stored);
> > +	atomic64_sub(meta->table[index].size, &zram->stats.compressed_size);
> > +	atomic64_dec(&zram->stats.pages_stored);
> >  
> >  	meta->table[index].handle = 0;
> >  	meta->table[index].size = 0;
> > @@ -362,7 +305,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> >  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> >  			  u32 index, int offset, struct bio *bio)
> >  {
> > -	int ret;
> > +	int ret = -EINVAL;
> >  	struct page *page;
> >  	unsigned char *user_mem, *uncmem = NULL;
> >  	struct zram_meta *meta = zram->meta;
> > @@ -406,6 +349,8 @@ out_cleanup:
> >  	kunmap_atomic(user_mem);
> >  	if (is_partial_io(bvec))
> >  		kfree(uncmem);
> > +	if (ret)
> > +		atomic64_inc(&zram->stats.failed_reads);
> >  	return ret;
> >  }
> >  
> > @@ -459,7 +404,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  		zram_set_flag(meta, index, ZRAM_ZERO);
> >  		write_unlock(&zram->meta->tb_lock);
> >  
> > -		atomic_inc(&zram->stats.pages_zero);
> > +		atomic64_inc(&zram->stats.pages_zero);
> >  		ret = 0;
> >  		goto out;
> >  	}
> > @@ -478,7 +423,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  	}
> >  
> >  	if (unlikely(clen > max_zpage_size)) {
> > -		atomic_inc(&zram->stats.bad_compress);
> >  		clen = PAGE_SIZE;
> >  		src = NULL;
> >  		if (is_partial_io(bvec))
> > @@ -516,11 +460,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  	write_unlock(&zram->meta->tb_lock);
> >  
> >  	/* Update stats */
> > -	atomic64_add(clen, &zram->stats.compr_size);
> > -	atomic_inc(&zram->stats.pages_stored);
> > -	if (clen <= PAGE_SIZE / 2)
> > -		atomic_inc(&zram->stats.good_compress);
> > -
> > +	atomic64_add(clen, &zram->stats.compressed_size);
> > +	atomic64_inc(&zram->stats.pages_stored);
> >  out:
> >  	if (locked)
> >  		mutex_unlock(&meta->buffer_lock);
> > @@ -586,23 +527,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)
> >  
> >  static void zram_init_device(struct zram *zram, struct zram_meta *meta)
> >  {
> > -	if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> > -		pr_info(
> > -		"There is little point creating a zram of greater than "
> > -		"twice the size of memory since we expect a 2:1 compression "
> > -		"ratio. Note that zram uses about 0.1%% of the size of "
> > -		"the disk when not in use so a huge zram is "
> > -		"wasteful.\n"
> > -		"\tMemory Size: %lu kB\n"
> > -		"\tSize you selected: %llu kB\n"
> > -		"Continuing anyway ...\n",
> > -		(totalram_pages << PAGE_SHIFT) >> 10, zram->disksize >> 10
> > -		);
> > -	}
> > -
> >  	/* zram devices sort of resembles non-rotational disks */
> >  	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> > -
> >  	zram->meta = meta;
> >  	pr_debug("Initialization done!\n");
> >  }
> > @@ -774,14 +700,15 @@ static DEVICE_ATTR(disksize, S_IRUGO | S_IWUSR,
> >  		disksize_show, disksize_store);
> >  static DEVICE_ATTR(initstate, S_IRUGO, initstate_show, NULL);
> >  static DEVICE_ATTR(reset, S_IWUSR, NULL, reset_store);
> > -static DEVICE_ATTR(num_reads, S_IRUGO, num_reads_show, NULL);
> > -static DEVICE_ATTR(num_writes, S_IRUGO, num_writes_show, NULL);
> > -static DEVICE_ATTR(invalid_io, S_IRUGO, invalid_io_show, NULL);
> > -static DEVICE_ATTR(notify_free, S_IRUGO, notify_free_show, NULL);
> > -static DEVICE_ATTR(zero_pages, S_IRUGO, zero_pages_show, NULL);
> > -static DEVICE_ATTR(orig_data_size, S_IRUGO, orig_data_size_show, NULL);
> > -static DEVICE_ATTR(compr_data_size, S_IRUGO, compr_data_size_show, NULL);
> > -static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
> > +static DEVICE_ATTR(memory_used, S_IRUGO, memory_used_show, NULL);
> > +
> > +ZRAM_ATTR_RO(num_reads);
> > +ZRAM_ATTR_RO(num_writes);
> > +ZRAM_ATTR_RO(pages_stored);
> > +ZRAM_ATTR_RO(invalid_io);
> > +ZRAM_ATTR_RO(notify_free);
> > +ZRAM_ATTR_RO(pages_zero);
> > +ZRAM_ATTR_RO(compressed_size);
> >  
> >  static struct attribute *zram_disk_attrs[] = {
> >  	&dev_attr_disksize.attr,
> > @@ -789,12 +716,12 @@ static struct attribute *zram_disk_attrs[] = {
> >  	&dev_attr_reset.attr,
> >  	&dev_attr_num_reads.attr,
> >  	&dev_attr_num_writes.attr,
> > +	&dev_attr_pages_stored.attr,
> >  	&dev_attr_invalid_io.attr,
> >  	&dev_attr_notify_free.attr,
> > -	&dev_attr_zero_pages.attr,
> > -	&dev_attr_orig_data_size.attr,
> > -	&dev_attr_compr_data_size.attr,
> > -	&dev_attr_mem_used_total.attr,
> > +	&dev_attr_pages_zero.attr,
> > +	&dev_attr_compressed_size.attr,
> > +	&dev_attr_memory_used.attr,
> >  	NULL,
> >  };
> >  
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index e81e9cd..277023d 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -64,22 +64,19 @@ enum zram_pageflags {
> >  struct table {
> >  	unsigned long handle;
> >  	u16 size;	/* object size (excluding header) */
> > -	u8 count;	/* object ref count (not yet used) */
> > -	u8 flags;
> > +	u16 flags;
> >  } __aligned(4);
> >  
> >  struct zram_stats {
> > -	atomic64_t compr_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 num_writes;	/* number of writes */
> > +	atomic64_t num_reads;	/* number of reads */
> > +	atomic64_t pages_stored;	/* no. of pages currently stored */
> > +	atomic64_t compressed_size;	/* compressed size of pages stored */
> > +	atomic64_t pages_zero;		/* no. of zero filled pages */
> > +	atomic64_t failed_reads;	/* no. of failed reads */
> >  	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 */
> > -	atomic_t pages_zero;		/* no. of zero filled pages */
> > -	atomic_t pages_stored;	/* no. of pages currently stored */
> > -	atomic_t good_compress;	/* % of pages with compression ratio<=50% */
> > -	atomic_t bad_compress;	/* % of pages with compression ratio>=75% */
> >  };
> >  
> >  struct zram_meta {
> > -- 
> > 1.8.5.2.487.g7559984
> > 
> > --
> > 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 0/3] zram stats rework and code cleanup

[Sergey Senozhatsky]

On (01/14/14 12:37), Sergey Senozhatsky wrote:
> This patchset includes code clean up and zram stats rework.
> 

forgot to mention. this patchset based on top of Minchan's
patchset.

	-ss

> Sergey Senozhatsky (3):
>   zram: drop `init_done' struct zram member
>   zram: do not pass rw argument to __zram_make_request()
>   zram: rework reported to end-user zram statistics
> 
>  drivers/block/zram/zram_drv.c | 213 ++++++++++++++----------------------------
>  drivers/block/zram/zram_drv.h |  18 ++--
>  2 files changed, 76 insertions(+), 155 deletions(-)
> 
> -- 
> 1.8.5.2.487.g7559984
>