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

Sergey Senozhatsky

unread,

Jan 14, 2014, 4:50:02 AM1/14/14

to

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

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

Sergey Senozhatsky

unread,

Jan 14, 2014, 4:50:02 AM1/14/14

to

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.se...@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;

Sergey Senozhatsky

unread,

Jan 14, 2014, 4:50:02 AM1/14/14

to

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.se...@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 {

Sergey Senozhatsky

unread,

Jan 14, 2014, 4:50:02 AM1/14/14

to

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.se...@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;
/*

Sergey Senozhatsky

unread,

Jan 14, 2014, 4:50:03 AM1/14/14

to

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

Jerome Marchand

unread,

Jan 14, 2014, 5:40:01 AM1/14/14

to

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

Jerome Marchand

unread,

Jan 14, 2014, 6:10:01 AM1/14/14

to

On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:

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

Jerome

Jerome Marchand

unread,

Jan 14, 2014, 6:10:02 AM1/14/14

to

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.se...@gmail.com>

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

Sergey Senozhatsky

unread,

Jan 14, 2014, 6:10:03 AM1/14/14

to

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.

Sergey Senozhatsky

unread,

Jan 14, 2014, 6:20:01 AM1/14/14

to

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

Sergey Senozhatsky

unread,

Jan 14, 2014, 6:20:02 AM1/14/14

to

On (01/14/14 11:38), Jerome Marchand wrote:

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

Sergey Senozhatsky

unread,

Jan 14, 2014, 6:40:02 AM1/14/14

to

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.se...@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;

Jerome Marchand

unread,

Jan 14, 2014, 7:20:02 AM1/14/14

to

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 Marchand

unread,

Jan 14, 2014, 7:30:01 AM1/14/14

to

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.

Sergey Senozhatsky

unread,

Jan 14, 2014, 7:40:02 AM1/14/14

to

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

Jerome Marchand

unread,

Jan 14, 2014, 8:50:01 AM1/14/14

to

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.se...@gmail.com>

Sergey Senozhatsky

unread,

Jan 14, 2014, 9:00:02 AM1/14/14

to

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?

Jerome Marchand

unread,

Jan 14, 2014, 9:10:02 AM1/14/14

to

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.

Sergey Senozhatsky

unread,

Jan 14, 2014, 9:20:03 AM1/14/14

to

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

Jerome Marchand

unread,

Jan 14, 2014, 9:30:03 AM1/14/14

to

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

Minchan Kim

unread,

Jan 14, 2014, 9:00:02 PM1/14/14

to

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.se...@gmail.com>

Acked-by: Minchan Kim <min...@kernel.org>

--
Kind regards,
Minchan Kim

Minchan Kim

unread,

Jan 14, 2014, 9:20:01 PM1/14/14

to

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.

--
Kind regards,
Minchan Kim

Minchan Kim

unread,

Jan 14, 2014, 11:30:02 PM1/14/14

to

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.se...@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.

--
Kind regards,
Minchan Kim

Sergey Senozhatsky

unread,

Jan 15, 2014, 4:20:02 AM1/15/14

to

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

Sergey Senozhatsky

unread,

Jan 15, 2014, 5:20:04 AM1/15/14

to

This patchset includes zram stats clean up and enhancements.

Sergey Senozhatsky (4):

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

zram: report failed read and writes stats

drivers/block/zram/zram_drv.c | 210 ++++++++++++++----------------------------
drivers/block/zram/zram_drv.h | 18 ++--
2 files changed, 74 insertions(+), 154 deletions(-)

--
1.8.5.3.493.gb139ac2

Sergey Senozhatsky

unread,

Jan 15, 2014, 5:20:04 AM1/15/14

to

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.

Acked-by: Minchan Kim <min...@kernel.org>
Acked-by: Jerome Marchand <jmar...@redhat.com>

Signed-off-by: Sergey Senozhatsky <sergey.se...@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;
/*

Sergey Senozhatsky

unread,

Jan 15, 2014, 5:20:04 AM1/15/14

to

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 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: do not check for READA return code. noted by Jerome Marchand.

Acked-by: Minchan Kim <min...@kernel.org>
Acked-by: Jerome Marchand <jmar...@redhat.com>
Signed-off-by: Sergey Senozhatsky <sergey.se...@gmail.com>
---

drivers/block/zram/zram_drv.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index c323e05..b0bcb7e 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -533,14 +533,18 @@ 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 == 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 +674,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 +699,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 +737,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;

Sergey Senozhatsky

unread,

Jan 15, 2014, 5:30:01 AM1/15/14

to

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

2) 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. zram already accounts each
sub-request's compression size so we can calculate real device compression
ratio.

Acked-by: Minchan Kim <min...@kernel.org>
Acked-by: Jerome Marchand <jmar...@redhat.com>
Signed-off-by: Sergey Senozhatsky <sergey.se...@gmail.com>
---

drivers/block/zram/zram_drv.c | 163 ++++++++++++------------------------------
drivers/block/zram/zram_drv.h | 17 ++---
2 files changed, 51 insertions(+), 129 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index b0bcb7e..6eb2155 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)
-{