zram: free meta table in zram_meta_free
Subsystems (3)
Minchan Kim
maintainerPATCH 1/2 zram: free meta table in zram_meta_free
From: Ganesh Mahendran <[email protected]>
zram_meta_alloc() and zram_meta_free() are a pair.
In zram_meta_alloc(), meta table is allocated. So it it better to free it in zram_meta_free().
--- drivers/block/zram/zram_drv.c | 30 ++++++++++++++---------------- drivers/block/zram/zram_drv.h | 1 + 2 files changed, 15 insertions(+), 16 deletions(-)
drivers/block/zram/zram_drv.c +14 -16
@@ -309,6 +309,18 @@ static inline int valid_io_request(struct zram *zram,static void zram_meta_free (struct zram_meta *meta) { +size_t index; + +/* Free all pages that are still in this zram device */ +for (index =0 ; index < meta->num_pages; index++) { +unsigned long handle = meta->table[index].handle; + +if (!handle) +continue ; + +zs_free (meta->mem_pool, handle); + } +zs_destroy_pool (meta->mem_pool);vfree (meta->table);kfree (meta);
@@ -316,15 +328,14 @@ static void zram_meta_free(struct zram_meta *meta)static struct zram_meta *zram_meta_alloc (int device_id,u64 disksize) { -size_t num_pages;char pool_name[8 ];struct zram_meta *meta =kmalloc (sizeof (*meta),GFP_KERNEL );if (!meta)return NULL ; - num_pages = disksize >>PAGE_SHIFT ; - meta->table =vzalloc (num_pages *sizeof (*meta->table)); + meta->num_pages = disksize >>PAGE_SHIFT ; + meta->table =vzalloc (meta->num_pages *sizeof (*meta->table));if (!meta->table) {pr_err ("Error allocating zram address table\n" ); goto out_error;
@@ -706,9 +717,6 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { -size_t index; -struct zram_meta *meta; -down_write (&zram->init_lock); zram->limit_pages =0 ;
@@ -718,16 +726,6 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } - meta = zram->meta; -/* 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; -if (!handle) -continue ; - -zs_free (meta->mem_pool, handle); - } -zcomp_destroy (zram->comp); zram->max_comp_streams =1 ;
drivers/block/zram/zram_drv.h +1 -0
@@ -96,6 +96,7 @@ struct zram_stats {struct zram_meta {struct zram_table_entry *table;struct zs_pool *mem_pool; +size_t num_pages; };struct zram {
Signature · 7 lines
1.9.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerPATCH 2/2 zram: remove init_lock in zram_make_request
Admin could reset zram during I/O operation going on so we have used zram->init_lock as read-side lock in I/O path to prevent sudden zram meta freeing.
However, the init_lock is really troublesome.
We can't do call zram_meta_alloc under init_lock due to lockdep splat because zram_rw_page is one of the function under reclaim path and hold it as read_lock while other places in process context hold it as write_lock. So, we have used allocation out of the lock to avoid lockdep warn but it's not good for readability and fainally, I met another lockdep splat between init_lock and cpu_hotpulug from kmem_cache_destroy during wokring zsmalloc compaction. :(
Yes, the ideal is to remove horrible init_lock of zram in rw path. This patch removes it in rw path and instead, put init_done bool variable to check initialization done with smp_[wmb|rmb] and srcu_[un]read_lock to prevent sudden zram meta freeing during I/O operation.
--- drivers/block/zram/zram_drv.c | 85 +++++++++++++++++++++++++++++++------------ drivers/block/zram/zram_drv.h | 5 +++ 2 files changed, 66 insertions(+), 24 deletions(-)
drivers/block/zram/zram_drv.c +61 -24
@@ -32,6 +32,7 @@#include <linux/string.h> #include <linux/vmalloc.h> #include <linux/err.h> +#include <linux/srcu.h> #include "zram_drv.h"
@@ -53,9 +54,31 @@ static ssize_t name##_show(struct device *d, \ } \static DEVICE_ATTR_RO (name); -static inline int init_done (struct zram *zram) +static inline bool init_done (struct zram *zram) { -return zram->meta !=NULL ; +/* +* init_done can be used without holding zram->init_lock in +* read/write handler(ie, zram_make_request) but we should make sure +* that zram->init_done should set up after meta initialization is +* done. Look at setup_init_done. +*/ +bool ret = zram->init_done; + +if (ret) +smp_rmb ();/* pair with setup_init_done */ + +return ret; +} + +static inline void setup_init_done (struct zram *zram,bool flag) +{ +/* +* Store operation of struct zram fields should complete +* before zram->init_done set up because zram_bvec_rw +* doesn't hold an zram->init_lock. +*/ +smp_wmb (); + zram->init_done = flag; }static inline struct zram *dev_to_zram (struct device *dev)
@@ -326,6 +349,10 @@ static void zram_meta_free(struct zram_meta *meta)kfree (meta); } +static void rcu_zram_do_nothing (struct rcu_head *unused) +{ +} +static struct zram_meta *zram_meta_alloc (int device_id,u64 disksize) {char pool_name[8 ];
@@ -726,11 +753,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } -zcomp_destroy (zram->comp); zram->max_comp_streams =1 ; -zram_meta_free (zram->meta); - zram->meta =NULL ;/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats));
@@ -738,8 +762,12 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)if (reset_capacity)set_capacity (zram->disk,0 ); +setup_init_done (zram, false); +call_srcu (&zram->srcu, &zram->rcu, rcu_zram_do_nothing); +synchronize_srcu (&zram->srcu); +zram_meta_free (zram->meta); +zcomp_destroy (zram->comp);up_write (&zram->init_lock); -/* * Revalidate disk out of the init_lock to avoid lockdep splat. * It's okay because disk's capacity is protected by init_lock
@@ -762,10 +790,19 @@ static ssize_t disksize_store(struct device *dev,if (!disksize)return -EINVAL ; +down_write (&zram->init_lock); +if (init_done (zram)) { +pr_info ("Cannot change disksize for initialized device\n" ); +up_write (&zram->init_lock); +return -EBUSY ; + } + disksize =PAGE_ALIGN (disksize); meta =zram_meta_alloc (zram->disk->first_minor, disksize); -if (!meta) +if (!meta) { +up_write (&zram->init_lock);return -ENOMEM ; + } comp =zcomp_create (zram->compressor, zram->max_comp_streams);if (IS_ERR (comp)) {
@@ -775,17 +812,11 @@ static ssize_t disksize_store(struct device *dev, goto out_free_meta; } -down_write (&zram->init_lock); -if (init_done (zram)) { -pr_info ("Cannot change disksize for initialized device\n" ); - err = -EBUSY ; - goto out_destroy_comp; - } - zram->meta = meta; zram->comp = comp; zram->disksize = disksize;set_capacity (zram->disk, zram->disksize >>SECTOR_SHIFT ); +setup_init_done (zram, true);up_write (&zram->init_lock);/*
@@ -797,10 +828,8 @@ static ssize_t disksize_store(struct device *dev,return len; -out_destroy_comp: -up_write (&zram->init_lock); -zcomp_destroy (comp); out_free_meta: +up_write (&zram->init_lock);zram_meta_free (meta);return err; }
@@ -905,9 +934,10 @@ out:*/ static void zram_make_request (struct request_queue *queue,struct bio *bio) { +int idx;struct zram *zram = queue->queuedata; -down_read (&zram->init_lock); + idx =srcu_read_lock (&zram->srcu);if (unlikely (!init_done (zram))) goto error;
@@ -918,12 +948,12 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) }__zram_make_request (zram, bio); -up_read (&zram->init_lock); +srcu_read_unlock (&zram->srcu, idx);return ; error: -up_read (&zram->init_lock); +srcu_read_unlock (&zram->srcu, idx);bio_io_error (bio); }
@@ -945,18 +975,20 @@ static void zram_slot_free_notify(struct block_device *bdev,static int zram_rw_page (struct block_device *bdev,sector_t sector,struct page *page,int rw) { -int offset, err; +int offset, err, idx;u32 index;struct zram *zram;struct bio_vec bv; zram = bdev->bd_disk->private_data; + idx =srcu_read_lock (&zram->srcu); +if (!valid_io_request (zram, sector,PAGE_SIZE )) {atomic64_inc (&zram->stats.invalid_io); +srcu_read_unlock (&zram->srcu, idx);return -EINVAL ; } -down_read (&zram->init_lock);if (unlikely (!init_done (zram))) { err = -EIO ; goto out_unlock;
@@ -971,7 +1003,7 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, err =zram_bvec_rw (zram, &bv, index, offset, rw); out_unlock: -up_read (&zram->init_lock); +srcu_read_unlock (&zram->srcu, idx);/* * If I/O fails, just return error(ie, non-zero) without * calling page_endio.
@@ -1041,6 +1073,11 @@ static int create_device(struct zram *zram, int device_id)init_rwsem (&zram->init_lock); +if (init_srcu_struct (&zram->srcu)) { +pr_err ("Error initialize srcu for device %d\n" , device_id); + goto out; + } + zram->queue =blk_alloc_queue (GFP_KERNEL );if (!zram->queue) { pr_err("Error allocating disk queue for device %d\n",
@@ -1125,8 +1162,8 @@ static void destroy_device(struct zram *zram)del_gendisk (zram->disk);put_disk (zram->disk); -blk_cleanup_queue (zram->queue); +cleanup_srcu_struct (&zram->srcu); }static int __init zram_init (void )
drivers/block/zram/zram_drv.h +5 -0
@@ -105,8 +105,13 @@ struct zram {struct gendisk *disk;struct zcomp *comp; +struct srcu_struct srcu; +struct rcu_head rcu; +/* Prevent concurrent execution of device init, reset and R/W request */ struct rw_semaphore init_lock; +bool init_done; +/* * This is the limit on amount of *uncompressed* worth of data * we can store in a disk.
Signature · 7 lines
1.9.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 42 lines · Minchan Kim
Admin could reset zram during I/O operation going on so we have used
zram->init_lockas read-side lock in I/O path to prevent sudden zram meta freeing.However, the
init_lockis really troublesome.
We can't do callzram_meta_allocunderinit_lockdue to lockdep splat becausezram_rw_pageis one of the function under reclaim path and hold it asread_lockwhile other places in process context hold it aswrite_lock. So, we have used allocation out of the lock to avoid lockdep warn but it's not good for readability and fainally, I met another lockdep splat betweeninit_lockandcpu_hotpulugfromkmem_cache_destroyduring wokring zsmalloc compaction. :(Yes, the ideal is to remove horrible
init_lockof zram in rw path. This patch removes it in rw path and instead, putinit_donebool variable to check initialization done with smp_[wmb|rmb] and srcu_[un]read_lock to prevent sudden zram meta freeing during I/O operation.Signed-off-by Minchan Kim <[email protected]>--- drivers/block/zram/zram_drv.c | 85 +++++++++++++++++++++++++++++++------------ drivers/block/zram/zram_drv.h | 5 +++ 2 files changed, 66 insertions(+), 24 deletions(-)drivers/block/zram/zram_drv.c +1
-1@@ -32,6 +32,7 @@#include <linux/string.h> #include <linux/vmalloc.h> #include <linux/err.h> +#include <linux/srcu.h> #include "zram_drv.h" @@ -53,9 +54,31 @@ static ssize_t name##_show(struct device *d, \ } \static DEVICE_ATTR_RO (name); -static inline int init_done (struct zram *zram)
drivers/block/zram/zram_drv.c static ssize_t name##_show(struct device *d, \ · line 65
Quoted 10 lines
drivers/block/zram/zram_drv.c +8
-1+static inline bool init_done (struct zram *zram) { -return zram->meta !=NULL ; +/* +* init_done can be used without holding zram->init_lock in +* read/write handler(ie, zram_make_request) but we should make sure +* that zram->init_done should set up after meta initialization is +* done. Look at setup_init_done. +*/ +bool ret = zram->init_done;
I don't like re-introduced ->init_done.
another idea... how about using zram->disksize == 0' instead of ->init_done' (previously `->meta != NULL')? should do the trick.
and I'm not sure I get this rmb...
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 7 lines · Sergey Senozhatsky
Quoted 7 lines
drivers/block/zram/zram_drv.c +4
-2-static inline int init_done (struct zram *zram) +static inline bool init_done (struct zram *zram) { -return zram->meta !=NULL ; +/* +* init_done can be used without holding zram->init_lock in +* read/write handler(ie, zram_make_request) but we should make sure
Quoted 9 lines
drivers/block/zram/zram_drv.c static ssize_t name##_show(struct device *d, \ · line 65
Quoted 4 lines
drivers/block/zram/zram_drv.c +4
-0+* that zram->init_done should set up after meta initialization is +* done. Look at setup_init_done. +*/ +bool ret = zram->init_done;I don't like re-introduced ->init_done.
another idea... how about usingzram->disksize == 0' instead of->init_done' (previously `->meta != NULL')? should do the trick.
a typo, I meant `->disksize != 0'.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
In reply to Sergey SenozhatskyQuoted 42 lines · Sergey Senozhatsky
Quoted 41 lines · Minchan Kim
Admin could reset zram during I/O operation going on so we have used
zram->init_lockas read-side lock in I/O path to prevent sudden zram meta freeing.However, the
init_lockis really troublesome.
We can't do callzram_meta_allocunderinit_lockdue to lockdep splat becausezram_rw_pageis one of the function under reclaim path and hold it asread_lockwhile other places in process context hold it aswrite_lock. So, we have used allocation out of the lock to avoid lockdep warn but it's not good for readability and fainally, I met another lockdep splat betweeninit_lockandcpu_hotpulugfromkmem_cache_destroyduring wokring zsmalloc compaction. :(Yes, the ideal is to remove horrible
init_lockof zram in rw path. This patch removes it in rw path and instead, putinit_donebool variable to check initialization done with smp_[wmb|rmb] and srcu_[un]read_lock to prevent sudden zram meta freeing during I/O operation.Signed-off-by Minchan Kim <[email protected]>--- drivers/block/zram/zram_drv.c | 85 +++++++++++++++++++++++++++++++------------ drivers/block/zram/zram_drv.h | 5 +++ 2 files changed, 66 insertions(+), 24 deletions(-)drivers/block/zram/zram_drv.c +1
-0@@ -32,6 +32,7 @@#include <linux/string.h> #include <linux/vmalloc.h> #include <linux/err.h> +#include <linux/srcu.h> #include "zram_drv.h" @@ -53,9 +54,31 @@ static ssize_t name##_show(struct device *d, \ } \static DEVICE_ATTR_RO (name);
Quoted 15 lines
drivers/block/zram/zram_drv.c static ssize_t name##_show(struct device *d, \ · line 65
Quoted 11 lines
drivers/block/zram/zram_drv.c +8
-2-static inline int init_done (struct zram *zram) +static inline bool init_done (struct zram *zram) { -return zram->meta !=NULL ; +/* +* init_done can be used without holding zram->init_lock in +* read/write handler(ie, zram_make_request) but we should make sure +* that zram->init_done should set up after meta initialization is +* done. Look at setup_init_done. +*/ +bool ret = zram->init_done;I don't like re-introduced ->init_done.
another idea... how about usingzram->disksize == 0' instead of->init_done' (previously `->meta != NULL')? should do the trick.
It could be.
Quoted 3 lines
and I'm not sure I get this rmb...
What makes you not sure?
I think it's clear and common pattern for smp_[wmb|rmb]. :)
Quoted 2 lines
-ss
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 8 lines · Minchan Kim
Quoted 3 lines · Sergey Senozhatsky
I don't like re-introduced ->init_done.
another idea... how about usingzram->disksize == 0' instead of->init_done' (previously `->meta != NULL')? should do the trick.It could be.
care to change it?
Quoted 7 lines
Quoted 3 lines
and I'm not sure I get this rmb...
What makes you not sure?
I think it's clear and common pattern for smp_[wmb|rmb]. :)
well... what that "if (ret)" gives? it's almost always true, because the device is initialized during read/write operations (in 99.99% of cases).
-ss
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 11 lines · Sergey Senozhatsky
Quoted 8 lines · Minchan Kim
Quoted 3 lines · Sergey Senozhatsky
I don't like re-introduced ->init_done.
another idea... how about usingzram->disksize == 0' instead of->init_done' (previously `->meta != NULL')? should do the trick.It could be.
care to change it?
Will try!
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
and I'm not sure I get this rmb...
Quoted 7 lines
Quoted 3 lines
What makes you not sure?
I think it's clear and common pattern for smp_[wmb|rmb]. :)well... what that "if (ret)" gives? it's almost always true, because the device is initialized during read/write operations (in 99.99% of cases).
If it was your concern, I'm happy to remove the check.(ie, actually, I realized that after I push the button to send). Thanks!
Quoted 2 lines
-ss
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 10 lines · Minchan Kim
Quoted 9 lines · Sergey Senozhatsky
Quoted 8 lines · Minchan Kim
Quoted 3 lines · Sergey Senozhatsky
I don't like re-introduced ->init_done.
another idea... how about usingzram->disksize == 0' instead of->init_done' (previously `->meta != NULL')? should do the trick.It could be.
Quoted 7 lines
Quoted 1 line
care to change it?
Will try!
If it was your concern, I'm happy to remove the check.(ie, actually, I realized that after I push the button to send). Thanks!
Thanks a lot, Minchan.
and, guys, sorry for previous html email (I'm sure I toggled the "plain text" mode in gmail web-interface, but somehow it has different meaning in gmail world).
I'm still concerned about performance numbers that I see on my x86_64. it's not always, but mostly slower. I'll give it another try (disable lockdep, etc.), but if we lose 10% on average then, sorry, I'm not so positive about srcu change and will tend to vote for your initial commit that simply moved meta free() out of init_lock and left locking as is (lockdep warning would have been helpful there, because otherwise it just looked like we change code w/o any reason).
what do you thunk?
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 34 lines · Sergey Senozhatsky
Quoted 10 lines · Minchan Kim
Quoted 9 lines · Sergey Senozhatsky
Quoted 8 lines · Minchan Kim
Quoted 3 lines · Sergey Senozhatsky
I don't like re-introduced ->init_done.
another idea... how about usingzram->disksize == 0' instead of->init_done' (previously `->meta != NULL')? should do the trick.It could be.
Quoted 7 lines
Quoted 1 line
care to change it?
Will try!
If it was your concern, I'm happy to remove the check.(ie, actually, I realized that after I push the button to send). Thanks!
Thanks a lot, Minchan.
and, guys, sorry for previous html email (I'm sure I toggled the "plain text" mode in gmail web-interface, but somehow it has different meaning in gmail world).
I'm still concerned about performance numbers that I see on my
x86_64. it's not always, but mostly slower. I'll give it another try (disable lockdep, etc.), but if we lose 10% on average then, sorry, I'm not so positive about srcu change and will tend to vote for your initial commit that simply moved metafree()out ofinit_lockand left locking as is (lockdep warning would have been helpful there, because otherwise it just looked like we change code w/o any reason).
Quoted 1 line
what do you thunk?
Surely I agreee with you. If it suffers from 10% performance regression, it's absolutely no go.
However, I believe it should be no loss because that's one of the reason from RCU birth which should be really win in read-side lock path compared to other locking.
Please test it with dd or something for block-based test for removing noise from FS. I also will test it to confirm that with real machine.
Thanks for the review!
Quoted 2 lines
-ss
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 13 lines · Minchan Kim
Quoted 9 lines
I'm still concerned about performance numbers that I see on my
x86_64. it's not always, but mostly slower. I'll give it another try (disable lockdep, etc.), but if we lose 10% on average then, sorry, I'm not so positive about srcu change and will tend to vote for your initial commit that simply moved metafree()out ofinit_lockand left locking as is (lockdep warning would have been helpful there, because otherwise it just looked like we change code w/o any reason).what do you thunk?
Surely I agreee with you. If it suffers from 10% performance regression, it's absolutely no go.
Quoted 7 lines
However, I believe it should be no loss because that's one of the reason from RCU birth which should be really win in read-side lock path compared to other locking.
Please test it with dd or something for block-based test for removing noise from FS. I also will test it to confirm that with real machine.
do you test with a single dd thread/process? just dd if=foo of=bar -c... or you start N `dd &' processes?
for a single writer there should be no difference, no doubt. I'm more interested in multi-writer/multi-reader/mixed use cases.
the options that I use are: iozone -t 3 -R -r 16K -s 60M -I +Z and -I is:
-I Use VxFS VX_DIRECT, O_DIRECT,or O_DIRECTIO for all file operations
with O_DIRECT I don't think there is a lot of noise, but I'll try to use different benchmarks a bit later.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 18 lines · Sergey Senozhatsky
Quoted 17 lines · Minchan Kim
Quoted 9 lines
I'm still concerned about performance numbers that I see on my
x86_64. it's not always, but mostly slower. I'll give it another try (disable lockdep, etc.), but if we lose 10% on average then, sorry, I'm not so positive about srcu change and will tend to vote for your initial commit that simply moved metafree()out ofinit_lockand left locking as is (lockdep warning would have been helpful there, because otherwise it just looked like we change code w/o any reason).what do you thunk?
Surely I agreee with you. If it suffers from 10% performance regression, it's absolutely no go.
However, I believe it should be no loss because that's one of the reason from RCU birth which should be really win in read-side lock path compared to other locking.
Quoted 6 lines
Quoted 3 lines
Please test it with dd or something for block-based test for removing noise from FS. I also will test it to confirm that with real machine.
do you test with a single dd thread/process? just dd if=foo of=bar -c... or you start N `dd &' processes?
I tested it with multiple dd processes.
Quoted 11 lines
for a single writer there should be no difference, no doubt. I'm more interested in multi-writer/multi-reader/mixed use cases.
the options that I use are: iozone -t 3 -R -r 16K -s 60M -I +Z and -I is:
-I Use VxFSVX_DIRECT, O_DIRECT,orO_DIRECTIOfor all file operationswith
O_DIRECTI don't think there is a lot of noise, but I'll try to use different benchmarks a bit later.
As you told, the data was not stable.
Anyway, when I read down_read implementation, it's one atomic instruction. Hmm, it seems te be better for srcu_read_lock which does more things. But I guessed most of overhead are from [de]compression, memcpy, clear_page That's why I guessed we don't have measurable difference from that. What's the data pattern if you use iozone? I guess it's really simple pattern compressor can do fast. I used /dev/sda for dd write so more realistic data. Anyway, if we has 10% regression even if the data is simple, I never want to merge it.
I will test it carefully and if it turns out lots regression, surely, I will not go with this and send the original patch again.
Thanks.
Quoted 2 lines
-ss
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 3 lines · Minchan Kim
As you told, the data was not stable.
yes. fread test was always slower, and the rest was mostly slower.
Quoted 3 lines
Anyway, when I read
down_readimplementation, it's one atomic instruction. Hmm, it seems te be better forsrcu_read_lockwhich does more things.
srcu looks havier, agree.
Quoted 3 lines
But I guessed most of overhead are from [de]compression, memcpy,
clear_pageThat's why I guessed we don't have measurable difference from that. What's the data pattern if you use iozone?
by "data pattern" you mean usage scenario? well, I usually use zram for `make -jX', where X=[4..N]. so N concurrent read-write ops scenario.
-ss
Quoted 5 lines
I guess it's really simple pattern compressor can do fast. I used
/dev/sdafor dd write so more realistic data. Anyway, if we has 10% regression even if the data is simple, I never want to merge it.
I will test it carefully and if it turns out lots regression, surely, I will not go with this and send the original patch again.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Hello, Sergey
Quoted 11 lines · Sergey Senozhatsky
Quoted 3 lines · Minchan Kim
As you told, the data was not stable.
yes. fread test was always slower, and the rest was mostly slower.
Quoted 3 lines
Anyway, when I read
down_readimplementation, it's one atomic instruction. Hmm, it seems te be better forsrcu_read_lockwhich does more things.srcu looks havier, agree.
ffffffff8172c350 <down_read>:
ffffffff8172c350: e8 7b 3f 00 00 callq ffffffff817302d0 <__fentry__> ffffffff8172c355: 55 push %rbp ffffffff8172c356: 48 89 e5 mov %rsp,%rbp ffffffff8172c359: 53 push %rbx ffffffff8172c35a: 48 89 fb mov %rdi,%rbx ffffffff8172c35d: 48 83 ec 08 sub $0x8,%rsp ffffffff8172c361: e8 9a e0 ff ff callq ffffffff8172a400 <_cond_resched> ffffffff8172c366: 48 89 d8 mov %rbx,%rax ffffffff8172c369: f0 48 ff 00 lock incq (%rax) ffffffff8172c36d: 79 05 jns ffffffff8172c374 <down_read+0x24> ffffffff8172c36f: e8 5c e7 c4 ff callq ffffffff8137aad0 <call_rwsem_down_read_failed> ffffffff8172c374: 48 83 c4 08 add $0x8,%rsp ffffffff8172c378: 5b pop %rbx ffffffff8172c379: 5d pop %rbp ffffffff8172c37a: c3 retq
ffffffff810eeec0 <__srcu_read_lock>:
ffffffff810eeec0: e8 0b 14 64 00 callq ffffffff817302d0 <__fentry__> ffffffff810eeec5: 48 8b 07 mov (%rdi),%rax ffffffff810eeec8: 55 push %rbp ffffffff810eeec9: 48 89 e5 mov %rsp,%rbp ffffffff810eeecc: 83 e0 01 and $0x1,%eax ffffffff810eeecf: 48 63 d0 movslq %eax,%rdx ffffffff810eeed2: 48 8b 4f 08 mov 0x8(%rdi),%rcx ffffffff810eeed6: 65 48 ff 04 d1 incq %gs:(%rcx,%rdx,8) ffffffff810eeedb: 0f ae f0 mfence ffffffff810eeede: 48 83 c2 02 add $0x2,%rdx ffffffff810eeee2: 48 8b 4f 08 mov 0x8(%rdi),%rcx ffffffff810eeee6: 65 48 ff 04 d1 incq %gs:(%rcx,%rdx,8) ffffffff810eeeeb: 5d pop %rbp ffffffff810eeeec: c3 retq
Yes, __srcu_read_lock is a little bit heavier but the number of instruction are not too much difference to make difference 10%. A culprit is __cond_resched but I don't think, either because our test was CPU intensive soS I don't think schedule latency affects total bandwidth.
More cuprit is your data pattern.
It seems you didn't use scramble_buffers=0, zero_buffers in fio so that fio fills random data pattern so zram bandwidth could be different by compression/decompression ratio.
I did test your fio script adding above options with my 4 CPU real machine (NOTE, ubuntu fio is old so that it doesn't work well above two options so I should update fio recently which solves it perfectly)
Another thing about fio is it seems loops option works with write test with overwrite=1 options while read test doesn't work so that I should use perf stat -r options to verify stdev.
In addition, I passed first test to remove noise as creating files and increased testsize as 1G from 400m
1) randread
= vanilla =
Performance counter stats for 'fio test-fio-randread.txt' (10 runs):
4713.879241 task-clock (msec) # 3.160 CPUs utilized ( +- 0.62% )
1,131 context-switches # 0.240 K/sec ( +- 2.83% )
23 cpu-migrations # 0.005 K/sec ( +- 4.40% )
15,767 page-faults # 0.003 M/sec ( +- 0.03% )
15,134,497,088 cycles # 3.211 GHz ( +- 0.15% ) [83.36%]
10,763,665,604 stalled-cycles-frontend # 71.12% frontend cycles idle ( +- 0.22% ) [83.34%]
6,896,294,076 stalled-cycles-backend # 45.57% backend cycles idle ( +- 0.29% ) [66.67%]
9,898,608,791 instructions # 0.65 insns per cycle
# 1.09 stalled cycles per insn ( +- 0.07% ) [83.33%]
1,852,167,485 branches # 392.918 M/sec ( +- 0.07% ) [83.34%]
14,864,143 branch-misses # 0.80% of all branches ( +- 0.16% ) [83.34%]1.491813361 seconds time elapsed ( +- 0.62% )
= srcu =
Performance counter stats for 'fio test-fio-randread.txt' (10 runs):
4752.790715 task-clock (msec) # 3.166 CPUs utilized ( +- 0.48% )
1,179 context-switches # 0.248 K/sec ( +- 1.56% )
26 cpu-migrations # 0.005 K/sec ( +- 3.91% )
15,764 page-faults # 0.003 M/sec ( +- 0.02% )
15,263,869,915 cycles # 3.212 GHz ( +- 0.25% ) [83.32%]
10,935,658,177 stalled-cycles-frontend # 71.64% frontend cycles idle ( +- 0.38% ) [83.33%]
7,067,290,320 stalled-cycles-backend # 46.30% backend cycles idle ( +- 0.46% ) [66.64%]
9,896,513,423 instructions # 0.65 insns per cycle
# 1.11 stalled cycles per insn ( +- 0.07% ) [83.33%]
1,847,612,285 branches # 388.743 M/sec ( +- 0.07% ) [83.38%]
14,814,815 branch-misses # 0.80% of all branches ( +- 0.24% ) [83.37%]1.501284082 seconds time elapsed ( +- 0.50% )
srcu is worse as 0.63% but the difference is really marginal.
2) randwrite
= vanilla =
Performance counter stats for 'fio test-fio-randwrite.txt' (10 runs):
6283.823490 task-clock (msec) # 3.332 CPUs utilized ( +- 0.44% )
1,536 context-switches # 0.245 K/sec ( +- 2.10% )
25 cpu-migrations # 0.004 K/sec ( +- 3.79% )
15,914 page-faults # 0.003 M/sec ( +- 0.02% )
20,408,942,915 cycles # 3.248 GHz ( +- 0.40% ) [83.34%]
14,398,424,739 stalled-cycles-frontend # 70.55% frontend cycles idle ( +- 0.62% ) [83.36%]
9,513,822,555 stalled-cycles-backend # 46.62% backend cycles idle ( +- 0.62% ) [66.65%]
13,507,376,783 instructions # 0.66 insns per cycle
# 1.07 stalled cycles per insn ( +- 0.05% ) [83.36%]
3,155,423,934 branches # 502.150 M/sec ( +- 0.05% ) [83.34%]
18,381,090 branch-misses # 0.58% of all branches ( +- 0.16% ) [83.34%]1.885926070 seconds time elapsed ( +- 0.61% )
= srcu =
Performance counter stats for 'fio test-fio-randwrite.txt' (10 runs):
6152.997119 task-clock (msec) # 3.304 CPUs utilized ( +- 0.29% )
1,422 context-switches # 0.231 K/sec ( +- 3.45% )
28 cpu-migrations # 0.004 K/sec ( +- 7.47% )
15,921 page-faults # 0.003 M/sec ( +- 0.02% )
19,862,315,430 cycles # 3.228 GHz ( +- 0.09% ) [83.33%]
13,872,541,761 stalled-cycles-frontend # 69.84% frontend cycles idle ( +- 0.12% ) [83.34%]
9,074,883,552 stalled-cycles-backend # 45.69% backend cycles idle ( +- 0.19% ) [66.71%]
13,494,854,651 instructions # 0.68 insns per cycle
# 1.03 stalled cycles per insn ( +- 0.03% ) [83.37%]
3,148,938,955 branches # 511.773 M/sec ( +- 0.04% ) [83.33%]
17,701,249 branch-misses # 0.56% of all branches ( +- 0.23% ) [83.34%]1.862543230 seconds time elapsed ( +- 0.35% )
srcu is better as 1.24% is better.
3) randrw
= vanilla =
Performance counter stats for 'fio test-fio-randrw.txt' (10 runs):
5609.976477 task-clock (msec) # 3.249 CPUs utilized ( +- 0.34% )
1,407 context-switches # 0.251 K/sec ( +- 0.96% )
25 cpu-migrations # 0.004 K/sec ( +- 5.37% )
15,906 page-faults # 0.003 M/sec ( +- 0.05% )
18,090,560,346 cycles # 3.225 GHz ( +- 0.35% ) [83.36%]
12,885,393,954 stalled-cycles-frontend # 71.23% frontend cycles idle ( +- 0.53% ) [83.33%]
8,570,185,547 stalled-cycles-backend # 47.37% backend cycles idle ( +- 0.59% ) [66.67%]
11,771,620,352 instructions # 0.65 insns per cycle
# 1.09 stalled cycles per insn ( +- 0.05% ) [83.35%]
2,508,014,871 branches # 447.063 M/sec ( +- 0.05% ) [83.34%]
18,585,638 branch-misses # 0.74% of all branches ( +- 0.23% ) [83.35%]1.726691239 seconds time elapsed ( +- 0.40% )
= srcu =
5475.312828 task-clock (msec) # 3.246 CPUs utilized ( +- 0.59% )
1,399 context-switches # 0.255 K/sec ( +- 1.46% )
24 cpu-migrations # 0.004 K/sec ( +- 6.27% )
15,916 page-faults # 0.003 M/sec ( +- 0.04% )
17,583,197,041 cycles # 3.211 GHz ( +- 0.11% ) [83.33%]
12,352,657,985 stalled-cycles-frontend # 70.25% frontend cycles idle ( +- 0.16% ) [83.33%]
8,173,164,212 stalled-cycles-backend # 46.48% backend cycles idle ( +- 0.19% ) [66.70%]
11,780,176,340 instructions # 0.67 insns per cycle
# 1.05 stalled cycles per insn ( +- 0.05% ) [83.36%]
2,506,722,383 branches # 457.823 M/sec ( +- 0.06% ) [83.35%]
18,436,877 branch-misses # 0.74% of all branches ( +- 0.18% ) [83.32%]1.686877512 seconds time elapsed ( +- 0.43% )
srcu is better as 2.3%
Srcu is better than down_read but I don't believe either because when I did perf record, [up|down]_read and srcu_read_[lock|unlock] is really minor (about 0.5%) so that I think it's really marginal. (for example, if we removes srcu_read_[un]lock totally, we just enhance about 1%) So, I don't think it's worth.
Okay, if you concerns on the data still, how about this? Even, it would be smaller instructions than [up|down]_read so I guess it could remove your performance concern. But I don't believe it could make significant difference, either.
Hope it addresses your concern.
Thanks.
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Hello Minchan,
excellent analysis!
Quoted 9 lines · Minchan Kim
Yes,
__srcu_read_lockis a little bit heavier but the number of instruction are not too much difference to make difference 10%. A culprit is__cond_reschedbut I don't think, either because our test was CPU intensive soS I don't think schedule latency affects total bandwidth.More cuprit is your data pattern.
It seems you didn't use scramble_buffers=0,zero_buffersin fio so that fio fills random data pattern so zram bandwidth could be different by compression/decompression ratio.
Completely agree.
Shame on me. gotten so used to iozone (iozone uses same data pattern 0xA5, this is +Z option what for), so I didn't even think about data pattern in fio. sorry.
Quoted 8 lines
1) randread
srcu is worse as 0.63% but the difference is really marginal.2) randwrite
srcu is better as 1.24% is better.3) randrw
srcu is better as 2.3%
hm, interesting. I'll re-check.
Quoted 1 line
Okay, if you concerns on the data still, how about this?
I'm not so upset to lose 0.6234187%. my concerns were about iozone's 10% different (which looks a bit worse).
I'll review your patch. Thanks for your effort.
Quoted 7 lines
Quoted 3 lines
by "data pattern" you mean usage scenario? well, I usually use zram for `make -jX', where X=[4..N]. so N concurrent read-write ops scenario.
What I meant is what data fills I/O buffer, which is really important to evaluate zram because the compression/decompression speeds relys on it.
I see. I never test it with `make' anyway, only iozone +Z.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
In reply to Minchan KimHello Minchan,
the idea looks good and this is something I was trying to do, except that I used kref.
some review nitpicks are below. I also posted modified version of your patch so that will save some time.
Review
Quoted 4 lines
static inline int
init_done(struct zram *zram)
{(quoted diff) +1
-1- return zram->meta != NULL; + return zram->disksize != 0;
we don't set ->disksize to 0 when create device. and I think it's better to use refcount here, but set it to 0 during device creation. (see the patch below)
Review
Quoted 6 lines
(quoted diff) +6
-0+static inline bool zram_meta_get(struct zram_meta *meta) +{ + if (!atomic_inc_not_zero(&meta->refcount)) + return false; + return true; +}
I've changed it to likely case first: `if recount return true'
Review
Quoted 4 lines
static void
zram_reset_device(struct zram *zram, bool reset_capacity){(quoted diff) +2
-0+ struct zram_meta *meta; + u64 disksize;
not needed. (see the patch below).
Review static void zram_reset_device(struct zram *zram, bool reset_capacity) · line 755
Quoted 11 lines
(quoted diff) +3
-0+ down_write(&zram->init_lock); zram->limit_pages = 0;@@ -728,14 +750,20 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity) return; } + meta = zram->meta; + zcomp_destroy(zram->comp);
we can't destoy zcomp before we see IO completion.
Review static void zram_reset_device(struct zram *zram, bool reset_capacity) · line 756
Quoted 1 line
(quoted diff) +0
-0zram->max_comp_streams = 1;
we better keep original comp_streams number before we see IO completion. we don't know how many RW ops we have, so completion can happen earlier.
Review static void zram_reset_device(struct zram *zram, bool reset_capacity) · line 760
Quoted 6 lines
(quoted diff) +4
-2- zram_meta_free(zram->meta, zram->disksize); - zram->meta = NULL; + disksize = zram->disksize; + zram_meta_put(meta); + /* Read/write handler will not handle further I/O operation. */ + zram->disksize = 0;
I keep it on its current position. (see below)
Quoted 12 lines
(quoted diff) +3
-1+ wait_for_completion(&meta->complete); + /* I/O operation under all of CPU are done so let's free */ + zram_meta_free(zram->meta, disksize); /* Reset stats */ memset(&zram->stats, 0, sizeof(zram->stats)); - zram->disksize = 0; if (reset_capacity) set_capacity(zram->disk, 0);@@ -908,23 +936,25 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) {
Review static void zram_make_request(struct request_queue *queue, struct bio *bio) · line 944
Quoted 10 lines
(quoted diff) +4
-2struct zram *zram = queue->queuedata; - down_read(&zram->init_lock); - if (unlikely(!init_done(zram))) + if (unlikely(!zram_meta_get(zram->meta))) goto error; + if (unlikely(!init_done(zram))) + goto put_meta; +
here and later:
we can't take zram_meta_get() first and then check for init_done(zram), because ->meta can be NULL, so it fill be ->NULL->refcount.
let's keep ->completion and ->refcount in zram and rename zram_meta_[get|put] to zram_[get|put].
please review a bit modified version of your patch.
/* the patch also reogranizes a bit order of struct zram members, to move member that we use more often together and to avoid paddings. nothing critical here. */
next action items are:
-- we actually can now switch from init_lock in some _show() fucntion to zram_get()/zram_put()
-- address that theoretical and very unlikely in real live race condition of umount-reset vs. mount-rw.
no concerns about performance of this version -- we probably will not get any faster than that.
thanks a lot for your effort!
---
drivers/block/zram/zram_drv.c | 82 ++++++++++++++++++++++++++++++------------- drivers/block/zram/zram_drv.h | 17 ++++----- 2 files changed, 66 insertions(+), 33 deletions(-)
drivers/block/zram/zram_drv.c +57 -25
@@ -44,7 +44,7 @@ static const char *default_compressor = "lzo";static unsigned int num_devices =1 ;#define ZRAM_ATTR_RO (name) \ -static ssize_t name##_show(struct device *d, \ +static ssize_t name##_show(struct device *d, \ struct device_attribute *attr, char *b) \ { \ struct zram *zram = dev_to_zram(d); \
@@ -55,7 +55,7 @@ static DEVICE_ATTR_RO(name);static inline int init_done (struct zram *zram) { -return zram->meta !=NULL ; +return atomic_read (&zram->refcount); }static inline struct zram *dev_to_zram (struct device *dev)
@@ -358,6 +358,23 @@ out_error:return NULL ; } +static inline bool zram_get (struct zram *zram) +{ +if (atomic_inc_not_zero (&zram->refcount)) +return true; +return false; +} + +/* +* We want to free zram_meta in process context to avoid +* deadlock between reclaim path and any other locks +*/ +static inline void zram_put (struct zram *zram) +{ +if (atomic_dec_and_test (&zram->refcount)) +complete (&zram->io_done); +} +static void update_position (u32 *index,int *offset,struct bio_vec *bvec) {if (*offset + bvec->bv_len >=PAGE_SIZE )
@@ -719,6 +736,9 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { +struct zram_meta *meta; +struct zcomp *comp; +down_write (&zram->init_lock); zram->limit_pages =0 ;
@@ -728,14 +748,21 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } -zcomp_destroy (zram->comp); - zram->max_comp_streams =1 ; -zram_meta_free (zram->meta, zram->disksize); - zram->meta =NULL ; + meta = zram->meta; + comp = zram->comp; +/* ->refcount will go down to 0 eventually */ +zram_put (zram); + +wait_for_completion (&zram->io_done); +/* I/O operation under all of CPU are done so let's free */ +zram_meta_free (meta, disksize); +zcomp_destroy (comp); +/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats)); - zram->disksize =0 ; + zram->max_comp_streams =1 ; +if (reset_capacity)set_capacity (zram->disk,0 );
@@ -783,6 +810,8 @@ static ssize_t disksize_store(struct device *dev, goto out_destroy_comp; } +init_completion (&zram->io_done); +atomic_set (&zram->refcount,1 ); zram->meta = meta; zram->comp = comp; zram->disksize = disksize;
@@ -795,7 +824,6 @@ static ssize_t disksize_store(struct device *dev,* so that revalidate_disk always sees up-to-date capacity. */ revalidate_disk (zram->disk); -return len; out_destroy_comp:
@@ -908,23 +936,24 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) {struct zram *zram = queue->queuedata; -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) +if (unlikely (!zram_get (zram))) goto error; +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, bio->bi_iter.bi_sector, bio->bi_iter.bi_size)) {atomic64_inc (&zram->stats.invalid_io); - goto error; + goto put_zram; }__zram_make_request (zram, bio); -up_read (&zram->init_lock); - +zram_put (zram);return ; - +put_zram: +zram_put (zram); error: -up_read (&zram->init_lock);bio_io_error (bio); }
@@ -946,21 +975,22 @@ static void zram_slot_free_notify(struct block_device *bdev,static int zram_rw_page (struct block_device *bdev,sector_t sector,struct page *page,int rw) { -int offset, err; +int offset, err = -EIO ;u32 index;struct zram *zram;struct bio_vec bv; zram = bdev->bd_disk->private_data; +if (unlikely (!zram_get (zram))) + goto out; + +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, sector,PAGE_SIZE )) {atomic64_inc (&zram->stats.invalid_io); -return -EINVAL ; - } - -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) { - err = -EIO ; - goto out_unlock; + err = -EINVAL ; + goto put_zram; } index = sector >>SECTORS_PER_PAGE_SHIFT ;
@@ -971,8 +1001,9 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, bv.bv_offset =0 ; err =zram_bvec_rw (zram, &bv, index, offset, rw); -out_unlock: -up_read (&zram->init_lock); +put_zram: +zram_put (zram); +out:/* * If I/O fails, just return error(ie, non-zero) without * calling page_endio.
@@ -1041,6 +1072,7 @@ static int create_device(struct zram *zram, int device_id)int ret = -ENOMEM ;init_rwsem (&zram->init_lock); +atomic_set (&zram->refcount,0 ); zram->queue =blk_alloc_queue (GFP_KERNEL );if (!zram->queue) {
drivers/block/zram/zram_drv.h +9 -8
@@ -100,24 +100,25 @@ struct zram_meta {struct zram {struct zram_meta *meta; +struct zcomp *comp;struct request_queue *queue;struct gendisk *disk; -struct zcomp *comp; -/* Prevent concurrent execution of device init, reset and R/W request */ struct rw_semaphore init_lock;/* -* This is the limit on amount of *uncompressed* worth of data -* we can store in a disk. +* the number of pages zram can consume for storing compressed data */ -u64 disksize;/* bytes */ +unsigned long limit_pages; +atomic_t refcount;int max_comp_streams; +struct zram_stats stats; +struct completion io_done;/* notify IO under all of cpu are done */ /* -* the number of pages zram can consume for storing compressed data +* This is the limit on amount of *uncompressed* worth of data +* we can store in a disk. */ -unsigned long limit_pages; - +u64 disksize;/* bytes */ char compressor[10 ]; };#endif
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Sorry, guys! my bad. the patch I sent to you is incomplete. pelase review this one.
Review
Quoted 2 lines · Sergey Senozhatsky
(quoted diff) +1
-0+ zram_meta_free(meta, disksize);
that was my in-mail last second stupid modification and I didn't compile tested it. I hit send button and then realized that your patch didn't move ->meta and ->comp free out of ->init_lock.
So this patch does it.
thanks.
---
drivers/block/zram/zram_drv.c | 84 +++++++++++++++++++++++++++++-------------- drivers/block/zram/zram_drv.h | 17 ++++----- 2 files changed, 67 insertions(+), 34 deletions(-)
drivers/block/zram/zram_drv.c +58 -26
@@ -44,7 +44,7 @@ static const char *default_compressor = "lzo";static unsigned int num_devices =1 ;#define ZRAM_ATTR_RO (name) \ -static ssize_t name##_show(struct device *d, \ +static ssize_t name##_show(struct device *d, \ struct device_attribute *attr, char *b) \ { \ struct zram *zram = dev_to_zram(d); \
@@ -55,7 +55,7 @@ static DEVICE_ATTR_RO(name);static inline int init_done (struct zram *zram) { -return zram->meta !=NULL ; +return atomic_read (&zram->refcount); }static inline struct zram *dev_to_zram (struct device *dev)
@@ -358,6 +358,23 @@ out_error:return NULL ; } +static inline bool zram_get (struct zram *zram) +{ +if (atomic_inc_not_zero (&zram->refcount)) +return true; +return false; +} + +/* +* We want to free zram_meta in process context to avoid +* deadlock between reclaim path and any other locks +*/ +static inline void zram_put (struct zram *zram) +{ +if (atomic_dec_and_test (&zram->refcount)) +complete (&zram->io_done); +} +static void update_position (u32 *index,int *offset,struct bio_vec *bvec) {if (*offset + bvec->bv_len >=PAGE_SIZE )
@@ -719,6 +736,10 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { +struct zram_meta *meta; +struct zcomp *comp; +u64 disksize; +down_write (&zram->init_lock); zram->limit_pages =0 ;
@@ -728,19 +749,25 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } -zcomp_destroy (zram->comp); - zram->max_comp_streams =1 ; -zram_meta_free (zram->meta, zram->disksize); - zram->meta =NULL ; + meta = zram->meta; + comp = zram->comp; + disksize = zram->disksize; +/* ->refcount will go down to 0 eventually */ +zram_put (zram); +wait_for_completion (&zram->io_done); +/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats)); - zram->disksize =0 ; + zram->max_comp_streams =1 ; +if (reset_capacity)set_capacity (zram->disk,0 );up_write (&zram->init_lock); - +/* I/O operation under all of CPU are done so let's free */ +zram_meta_free (meta, disksize); +zcomp_destroy (comp);/* * Revalidate disk out of the init_lock to avoid lockdep splat. * It's okay because disk's capacity is protected by init_lock
@@ -783,6 +810,8 @@ static ssize_t disksize_store(struct device *dev, goto out_destroy_comp; } +init_completion (&zram->io_done); +atomic_set (&zram->refcount,1 ); zram->meta = meta; zram->comp = comp; zram->disksize = disksize;
@@ -795,7 +824,6 @@ static ssize_t disksize_store(struct device *dev,* so that revalidate_disk always sees up-to-date capacity. */ revalidate_disk (zram->disk); -return len; out_destroy_comp:
@@ -908,23 +936,24 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) {struct zram *zram = queue->queuedata; -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) +if (unlikely (!zram_get (zram))) goto error; +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, bio->bi_iter.bi_sector, bio->bi_iter.bi_size)) {atomic64_inc (&zram->stats.invalid_io); - goto error; + goto put_zram; }__zram_make_request (zram, bio); -up_read (&zram->init_lock); - +zram_put (zram);return ; - +put_zram: +zram_put (zram); error: -up_read (&zram->init_lock);bio_io_error (bio); }
@@ -946,21 +975,22 @@ static void zram_slot_free_notify(struct block_device *bdev,static int zram_rw_page (struct block_device *bdev,sector_t sector,struct page *page,int rw) { -int offset, err; +int offset, err = -EIO ;u32 index;struct zram *zram;struct bio_vec bv; zram = bdev->bd_disk->private_data; +if (unlikely (!zram_get (zram))) + goto out; + +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, sector,PAGE_SIZE )) {atomic64_inc (&zram->stats.invalid_io); -return -EINVAL ; - } - -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) { - err = -EIO ; - goto out_unlock; + err = -EINVAL ; + goto put_zram; } index = sector >>SECTORS_PER_PAGE_SHIFT ;
@@ -971,8 +1001,9 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, bv.bv_offset =0 ; err =zram_bvec_rw (zram, &bv, index, offset, rw); -out_unlock: -up_read (&zram->init_lock); +put_zram: +zram_put (zram); +out:/* * If I/O fails, just return error(ie, non-zero) without * calling page_endio.
@@ -1041,6 +1072,7 @@ static int create_device(struct zram *zram, int device_id)int ret = -ENOMEM ;init_rwsem (&zram->init_lock); +atomic_set (&zram->refcount,0 ); zram->queue =blk_alloc_queue (GFP_KERNEL );if (!zram->queue) {
drivers/block/zram/zram_drv.h +9 -8
@@ -100,24 +100,25 @@ struct zram_meta {struct zram {struct zram_meta *meta; +struct zcomp *comp;struct request_queue *queue;struct gendisk *disk; -struct zcomp *comp; -/* Prevent concurrent execution of device init, reset and R/W request */ struct rw_semaphore init_lock;/* -* This is the limit on amount of *uncompressed* worth of data -* we can store in a disk. +* the number of pages zram can consume for storing compressed data */ -u64 disksize;/* bytes */ +unsigned long limit_pages; +atomic_t refcount;int max_comp_streams; +struct zram_stats stats; +struct completion io_done;/* notify IO under all of cpu are done */ /* -* the number of pages zram can consume for storing compressed data +* This is the limit on amount of *uncompressed* worth of data +* we can store in a disk. */ -unsigned long limit_pages; - +u64 disksize;/* bytes */ char compressor[10 ]; };#endif
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 34 lines · Sergey Senozhatsky
Sorry, guys! my bad. the patch I sent to you is incomplete. pelase review this one.
drivers/block/zram/zram_drv.c static void zram_reset_device(struct zram *zram, bool reset_capacity) · line 769
Quoted 2 lines · Sergey Senozhatsky
drivers/block/zram/zram_drv.c +1
-0+zram_meta_free (meta, disksize);that was my in-mail last second stupid modification and I didn't compile tested it. I hit send button and then realized that your patch didn't move ->meta and ->comp free out of ->init_lock.
So this patch does it.
thanks.
---
drivers/block/zram/zram_drv.c | 84 +++++++++++++++++++++++++++++-------------- drivers/block/zram/zram_drv.h | 17 ++++----- 2 files changed, 67 insertions(+), 34 deletions(-)drivers/block/zram/zram_drv.c +1
-1@@ -44,7 +44,7 @@ static const char *default_compressor = "lzo";static unsigned int num_devices =1 ;#define ZRAM_ATTR_RO (name) \ -static ssize_t name##_show(struct device *d, \ +static ssize_t name##_show(struct device *d, \ struct device_attribute *attr, char *b) \ { \ struct zram *zram = dev_to_zram(d); \
drivers/block/zram/zram_drv.c static DEVICE_ATTR_RO(name); · line 58
Quoted 5 lines
drivers/block/zram/zram_drv.c +1
-1@@ -55,7 +55,7 @@ static DEVICE_ATTR_RO(name);static inline int init_done (struct zram *zram) { -return zram->meta !=NULL ; +return atomic_read (&zram->refcount);
As I said previous mail, it could make livelock so I want to use disksize in here to prevent further I/O handling.
Quoted 13 lines
drivers/block/zram/zram_drv.c +6
-0}static inline struct zram *dev_to_zram (struct device *dev)@@ -358,6 +358,23 @@ out_error:return NULL ; } +static inline bool zram_get (struct zram *zram) +{ +if (atomic_inc_not_zero (&zram->refcount)) +return true; +return false; +}
drivers/block/zram/zram_drv.c out_error: · line 376
Quoted 10 lines
drivers/block/zram/zram_drv.c +10
-0+ +/* +* We want to free zram_meta in process context to avoid +* deadlock between reclaim path and any other locks +*/ +static inline void zram_put (struct zram *zram) +{ +if (atomic_dec_and_test (&zram->refcount)) +complete (&zram->io_done); +}
Although I suggested this complete, it might be rather overkill(pz, understand me it was work in midnight. :))
Instead, we could use just atomic_dec in here and
use wait_event(event, atomic_read(&zram->refcount) == 0) in reset.
Otherwise, looks good to me. I will cook up based on this version and test/send if you don't have any strong objection until that.
Thanks for the review, Sergey!
Quoted 186 lines
drivers/block/zram/zram_drv.c +40
-24+static void update_position (u32 *index,int *offset,struct bio_vec *bvec) {if (*offset + bvec->bv_len >=PAGE_SIZE )@@ -719,6 +736,10 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { +struct zram_meta *meta; +struct zcomp *comp; +u64 disksize; +down_write (&zram->init_lock); zram->limit_pages =0 ;@@ -728,19 +749,25 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } -zcomp_destroy (zram->comp); - zram->max_comp_streams =1 ; -zram_meta_free (zram->meta, zram->disksize); - zram->meta =NULL ; + meta = zram->meta; + comp = zram->comp; + disksize = zram->disksize; +/* ->refcount will go down to 0 eventually */ +zram_put (zram); +wait_for_completion (&zram->io_done); +/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats)); - zram->disksize =0 ; + zram->max_comp_streams =1 ; +if (reset_capacity)set_capacity (zram->disk,0 );up_write (&zram->init_lock); - +/* I/O operation under all of CPU are done so let's free */ +zram_meta_free (meta, disksize); +zcomp_destroy (comp);/* * Revalidate disk out of the init_lock to avoid lockdep splat. * It's okay because disk's capacity is protected by init_lock @@ -783,6 +810,8 @@ static ssize_t disksize_store(struct device *dev, goto out_destroy_comp; } +init_completion (&zram->io_done); +atomic_set (&zram->refcount,1 ); zram->meta = meta; zram->comp = comp; zram->disksize = disksize;@@ -795,7 +824,6 @@ static ssize_t disksize_store(struct device *dev,* so that revalidate_disk always sees up-to-date capacity. */ revalidate_disk (zram->disk); -return len; out_destroy_comp:@@ -908,23 +936,24 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) {struct zram *zram = queue->queuedata; -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) +if (unlikely (!zram_get (zram))) goto error; +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, bio->bi_iter.bi_sector, bio->bi_iter.bi_size)) {atomic64_inc (&zram->stats.invalid_io); - goto error; + goto put_zram; }__zram_make_request (zram, bio); -up_read (&zram->init_lock); - +zram_put (zram);return ; - +put_zram: +zram_put (zram); error: -up_read (&zram->init_lock);bio_io_error (bio); }@@ -946,21 +975,22 @@ static void zram_slot_free_notify(struct block_device *bdev,static int zram_rw_page (struct block_device *bdev,sector_t sector,struct page *page,int rw) { -int offset, err; +int offset, err = -EIO ;u32 index;struct zram *zram;struct bio_vec bv; zram = bdev->bd_disk->private_data; +if (unlikely (!zram_get (zram))) + goto out; + +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, sector,PAGE_SIZE )) {atomic64_inc (&zram->stats.invalid_io); -return -EINVAL ; - } - -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) { - err = -EIO ; - goto out_unlock; + err = -EINVAL ; + goto put_zram; } index = sector >>SECTORS_PER_PAGE_SHIFT ;@@ -971,8 +1001,9 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, bv.bv_offset =0 ; err =zram_bvec_rw (zram, &bv, index, offset, rw); -out_unlock: -up_read (&zram->init_lock); +put_zram: +zram_put (zram); +out:/* * If I/O fails, just return error(ie, non-zero) without * calling page_endio. @@ -1041,6 +1072,7 @@ static int create_device(struct zram *zram, int device_id)int ret = -ENOMEM ;init_rwsem (&zram->init_lock); +atomic_set (&zram->refcount,0 ); zram->queue =blk_alloc_queue (GFP_KERNEL );if (!zram->queue) {drivers/block/zram/zram_drv.h +9
-8@@ -100,24 +100,25 @@ struct zram_meta {struct zram {struct zram_meta *meta; +struct zcomp *comp;struct request_queue *queue;struct gendisk *disk; -struct zcomp *comp; -/* Prevent concurrent execution of device init, reset and R/W request */ struct rw_semaphore init_lock;/* -* This is the limit on amount of *uncompressed* worth of data -* we can store in a disk. +* the number of pages zram can consume for storing compressed data */ -u64 disksize;/* bytes */ +unsigned long limit_pages; +atomic_t refcount;int max_comp_streams; +struct zram_stats stats; +struct completion io_done;/* notify IO under all of cpu are done */ /* -* the number of pages zram can consume for storing compressed data +* This is the limit on amount of *uncompressed* worth of data +* we can store in a disk. */ -unsigned long limit_pages; - +u64 disksize;/* bytes */ char compressor[10 ]; };#endif
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 7 lines · Minchan Kim
drivers/block/zram/zram_drv.c static DEVICE_ATTR_RO(name); · line 58
Quoted 4 lines
static inline int
init_done(struct zram *zram)
{drivers/block/zram/zram_drv.c +1
-1-return zram->meta !=NULL ; +return atomic_read (&zram->refcount);As I said previous mail, it could make livelock so I want to use disksize in here to prevent further I/O handling.
just as I said in my previous email -- is this live lock really possible? we need to umount device to continue with reset. and umount will kill IOs out of our way.
the other reset caller is __exit zram_exit(). but once again, I don't expect this function being executed on mounted device and module being in use.
Quoted 11 lines
drivers/block/zram/zram_drv.c out_error: · line 376
Quoted 5 lines
drivers/block/zram/zram_drv.c +5
-0+static inline void zram_put (struct zram *zram) +{ +if (atomic_dec_and_test (&zram->refcount)) +complete (&zram->io_done); +}Although I suggested this complete, it might be rather overkill(pz, understand me it was work in midnight. :))
Instead, we could use justatomic_decin here and
usewait_event(event, atomic_read(&zram->refcount) == 0)in reset.
yes, I think it can do the trick.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 30 lines · Sergey Senozhatsky
Quoted 7 lines · Minchan Kim
drivers/block/zram/zram_drv.c static DEVICE_ATTR_RO(name); · line 58
Quoted 4 lines
static inline int
init_done(struct zram *zram)
{drivers/block/zram/zram_drv.c +1
-1-return zram->meta !=NULL ; +return atomic_read (&zram->refcount);As I said previous mail, it could make livelock so I want to use disksize in here to prevent further I/O handling.
just as I said in my previous email -- is this live lock really possible? we need to umount device to continue with reset. and umount will kill IOs out of our way.
the other reset caller is
__exitzram_exit(). but once again, I don't expect this function being executed on mounted device and module being in use.Quoted 11 lines
drivers/block/zram/zram_drv.c out_error: · line 376
Quoted 5 lines
drivers/block/zram/zram_drv.c +5
-0+static inline void zram_put (struct zram *zram) +{ +if (atomic_dec_and_test (&zram->refcount)) +complete (&zram->io_done); +}Although I suggested this complete, it might be rather overkill(pz, understand me it was work in midnight. :))
Instead, we could use justatomic_decin here and
usewait_event(event, atomic_read(&zram->refcount) == 0)in reset.
Quoted 1 line
yes, I think it can do the trick.
Hey, it's not a trick. It suits for the our goal well. Completion was too much, I think.
Quoted 2 lines
-ss
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 31 lines · Minchan Kim
Quoted 30 lines · Sergey Senozhatsky
Quoted 7 lines · Minchan Kim
drivers/block/zram/zram_drv.c static DEVICE_ATTR_RO(name); · line 58
Quoted 4 lines
static inline int
init_done(struct zram *zram)
{drivers/block/zram/zram_drv.c +1
-1-return zram->meta !=NULL ; +return atomic_read (&zram->refcount);As I said previous mail, it could make livelock so I want to use disksize in here to prevent further I/O handling.
just as I said in my previous email -- is this live lock really possible? we need to umount device to continue with reset. and umount will kill IOs out of our way.
the other reset caller is
__exitzram_exit(). but once again, I don't expect this function being executed on mounted device and module being in use.Quoted 11 lines
drivers/block/zram/zram_drv.c out_error: · line 376
Quoted 5 lines
drivers/block/zram/zram_drv.c +5
-0+static inline void zram_put (struct zram *zram) +{ +if (atomic_dec_and_test (&zram->refcount)) +complete (&zram->io_done); +}Although I suggested this complete, it might be rather overkill(pz, understand me it was work in midnight. :))
Instead, we could use justatomic_decin here and
usewait_event(event, atomic_read(&zram->refcount) == 0)in reset.
Quoted 5 lines
Quoted 1 line
yes, I think it can do the trick.
Hey, it's not a trick. It suits for the our goal well. Completion was too much, I think.
sure :)
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
In reply to Sergey SenozhatskyHey Sergey,
Quoted 7 lines · Sergey Senozhatsky
Hello Minchan,
the idea looks good and this is something I was trying to do, except that I used kref.
some review nitpicks are below. I also posted modified version of your patch so that will save some time.
Thanks a lot!
Quoted 10 lines
Review
Quoted 6 lines
static inline int
init_done(struct zram *zram)
{(quoted diff) +1
-1- return zram->meta != NULL; + return zram->disksize != 0;we don't set ->disksize to 0 when create device. and I think it's better to use refcount here, but set it to 0 during device creation. (see the patch below)
There was a reason I didn't use refcount there.
I should have written down it.
We need something to prevent further I/O handling on other CPUs. Otherwise, it's livelock. For example, new 'A' I/O rw path on CPU 1 can see non-zero refcount if another CPU is going on rw. Then, another new 'B' I/O rw path on CPU 2 can see non-zero refcount if A I/O is going on. Then, another new 'C' I/O rw path on CPU 3 can see non-zero refcount if B I/O is going on. Finally, 'A' IO is done on CPU 1 and next I/O 'D' on CPU 1 can see non-zero refcount because 'C' on CPU 3 is going on. Infinite loop.
Quoted 9 lines
drivers/block/zram/zram_drv.c out_error: · line 366
Quoted 7 lines
drivers/block/zram/zram_drv.c +6
-0+static inline bool zram_meta_get (struct zram_meta *meta) +{ +if (!atomic_inc_not_zero (&meta->refcount)) +return false; +return true; +}I've changed it to likely case first: `if recount return true'
Thanks!
Quoted 16 lines
Review
Quoted 5 lines
static void
zram_reset_device(struct zram *zram, bool reset_capacity){(quoted diff) +2
-0+ struct zram_meta *meta; + u64 disksize;not needed. (see the patch below).
Quoted 8 lines
drivers/block/zram/zram_drv.c +1
-0+down_write (&zram->init_lock); zram->limit_pages =0 ;@@ -728,14 +750,20 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; }
Quoted 5 lines
drivers/block/zram/zram_drv.c static void zram_reset_device(struct zram *zram, bool reset_capacity)
Quoted 3 lines
drivers/block/zram/zram_drv.c +2
-0+ meta = zram->meta; +zcomp_destroy (zram->comp);we can't destoy zcomp before we see IO completion.
True.
Quoted 5 lines
Review static void zram_reset_device(struct zram *zram, bool reset_capacity) · line 756
Quoted 2 lines
(quoted diff) +0
-0zram->max_comp_streams = 1;we better keep original
comp_streamsnumber before we see IO completion. we don't know how many RW ops we have, so completion can happen earlier.
Yeb.
Quoted 29 lines
Review static void zram_reset_device(struct zram *zram, bool reset_capacity) · line 760
Quoted 7 lines
(quoted diff) +4
-2- zram_meta_free(zram->meta, zram->disksize); - zram->meta = NULL; + disksize = zram->disksize; + zram_meta_put(meta); + /* Read/write handler will not handle further I/O operation. */ + zram->disksize = 0;I keep it on its current position. (see below)
Quoted 19 lines
drivers/block/zram/zram_drv.c +4
-3+wait_for_completion (&meta->complete); +/* I/O operation under all of CPU are done so let's free */ +zram_meta_free (zram->meta, disksize);/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats)); - zram->disksize =0 ;if (reset_capacity)set_capacity (zram->disk,0 );@@ -908,23 +936,25 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) {struct zram *zram = queue->queuedata; -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) +if (unlikely (!zram_meta_get (zram->meta))) goto error;
Quoted 7 lines
drivers/block/zram/zram_drv.c static void zram_make_request(struct request_queue *queue, struct bio *bio) · line 944
Quoted 3 lines
drivers/block/zram/zram_drv.c +3
-0+if (unlikely (!init_done (zram))) + goto put_meta; +here and later:
we can't takezram_meta_get()first and then check forinit_done(zram), because ->meta can be NULL, so it fill be ->NULL->refcount.
True.
Actually, it was totally RFC I forgot adding the tag in the night but I can't escape from my shame with the escuse. Thanks!
Quoted 3 lines
let's keep ->completion and ->refcount in zram and rename zram_meta_[get|put] to zram_[get|put].
Good idea but still want to name it as zram_meta_get/put because zram_get naming might confuse struct zram's refcount rather than zram_meta. :)
Quoted 252 lines
please review a bit modified version of your patch.
/* the patch also reogranizes a bit order of struct zram members, to move member that we use more often together and to avoid paddings. nothing critical here. */
next action items are:
-- we actually can now switch frominit_lockin some_show()fucntion to zram_get()/zram_put()
-- address that theoretical and very unlikely in real live race condition of umount-reset vs. mount-rw.no concerns about performance of this version -- we probably will not get any faster than that.
thanks a lot for your effort!
---
drivers/block/zram/zram_drv.c | 82 ++++++++++++++++++++++++++++++------------- drivers/block/zram/zram_drv.h | 17 ++++----- 2 files changed, 66 insertions(+), 33 deletions(-)drivers/block/zram/zram_drv.c +57
-25@@ -44,7 +44,7 @@ static const char *default_compressor = "lzo";static unsigned int num_devices =1 ;#define ZRAM_ATTR_RO (name) \ -static ssize_t name##_show(struct device *d, \ +static ssize_t name##_show(struct device *d, \ struct device_attribute *attr, char *b) \ { \ struct zram *zram = dev_to_zram(d); \@@ -55,7 +55,7 @@ static DEVICE_ATTR_RO(name);static inline int init_done (struct zram *zram) { -return zram->meta !=NULL ; +return atomic_read (&zram->refcount); }static inline struct zram *dev_to_zram (struct device *dev)@@ -358,6 +358,23 @@ out_error:return NULL ; } +static inline bool zram_get (struct zram *zram) +{ +if (atomic_inc_not_zero (&zram->refcount)) +return true; +return false; +} + +/* +* We want to free zram_meta in process context to avoid +* deadlock between reclaim path and any other locks +*/ +static inline void zram_put (struct zram *zram) +{ +if (atomic_dec_and_test (&zram->refcount)) +complete (&zram->io_done); +} +static void update_position (u32 *index,int *offset,struct bio_vec *bvec) {if (*offset + bvec->bv_len >=PAGE_SIZE )@@ -719,6 +736,9 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { +struct zram_meta *meta; +struct zcomp *comp; +down_write (&zram->init_lock); zram->limit_pages =0 ;@@ -728,14 +748,21 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } -zcomp_destroy (zram->comp); - zram->max_comp_streams =1 ; -zram_meta_free (zram->meta, zram->disksize); - zram->meta =NULL ; + meta = zram->meta; + comp = zram->comp; +/* ->refcount will go down to 0 eventually */ +zram_put (zram); + +wait_for_completion (&zram->io_done); +/* I/O operation under all of CPU are done so let's free */ +zram_meta_free (meta, disksize); +zcomp_destroy (comp); +/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats)); - zram->disksize =0 ; + zram->max_comp_streams =1 ; +if (reset_capacity)set_capacity (zram->disk,0 );@@ -783,6 +810,8 @@ static ssize_t disksize_store(struct device *dev, goto out_destroy_comp; } +init_completion (&zram->io_done); +atomic_set (&zram->refcount,1 ); zram->meta = meta; zram->comp = comp; zram->disksize = disksize;@@ -795,7 +824,6 @@ static ssize_t disksize_store(struct device *dev,* so that revalidate_disk always sees up-to-date capacity. */ revalidate_disk (zram->disk); -return len; out_destroy_comp:@@ -908,23 +936,24 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) {struct zram *zram = queue->queuedata; -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) +if (unlikely (!zram_get (zram))) goto error; +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, bio->bi_iter.bi_sector, bio->bi_iter.bi_size)) {atomic64_inc (&zram->stats.invalid_io); - goto error; + goto put_zram; }__zram_make_request (zram, bio); -up_read (&zram->init_lock); - +zram_put (zram);return ; - +put_zram: +zram_put (zram); error: -up_read (&zram->init_lock);bio_io_error (bio); }@@ -946,21 +975,22 @@ static void zram_slot_free_notify(struct block_device *bdev,static int zram_rw_page (struct block_device *bdev,sector_t sector,struct page *page,int rw) { -int offset, err; +int offset, err = -EIO ;u32 index;struct zram *zram;struct bio_vec bv; zram = bdev->bd_disk->private_data; +if (unlikely (!zram_get (zram))) + goto out; + +if (unlikely (!init_done (zram))) + goto put_zram; +if (!valid_io_request (zram, sector,PAGE_SIZE )) {atomic64_inc (&zram->stats.invalid_io); -return -EINVAL ; - } - -down_read (&zram->init_lock); -if (unlikely (!init_done (zram))) { - err = -EIO ; - goto out_unlock; + err = -EINVAL ; + goto put_zram; } index = sector >>SECTORS_PER_PAGE_SHIFT ;@@ -971,8 +1001,9 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, bv.bv_offset =0 ; err =zram_bvec_rw (zram, &bv, index, offset, rw); -out_unlock: -up_read (&zram->init_lock); +put_zram: +zram_put (zram); +out:/* * If I/O fails, just return error(ie, non-zero) without * calling page_endio. @@ -1041,6 +1072,7 @@ static int create_device(struct zram *zram, int device_id)int ret = -ENOMEM ;init_rwsem (&zram->init_lock); +atomic_set (&zram->refcount,0 ); zram->queue =blk_alloc_queue (GFP_KERNEL );if (!zram->queue) {drivers/block/zram/zram_drv.h +9
-8@@ -100,24 +100,25 @@ struct zram_meta {struct zram {struct zram_meta *meta; +struct zcomp *comp;struct request_queue *queue;struct gendisk *disk; -struct zcomp *comp; -/* Prevent concurrent execution of device init, reset and R/W request */ struct rw_semaphore init_lock;/* -* This is the limit on amount of *uncompressed* worth of data -* we can store in a disk. +* the number of pages zram can consume for storing compressed data */ -u64 disksize;/* bytes */ +unsigned long limit_pages; +atomic_t refcount;int max_comp_streams; +struct zram_stats stats; +struct completion io_done;/* notify IO under all of cpu are done */ /* -* the number of pages zram can consume for storing compressed data +* This is the limit on amount of *uncompressed* worth of data +* we can store in a disk. */ -unsigned long limit_pages; - +u64 disksize;/* bytes */ char compressor[10 ]; };#endif
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Hello Minchan,
Quoted 12 lines · Minchan Kim
Quoted 8 lines
Review
Quoted 4 lines
static inline int
init_done(struct zram *zram)
{(quoted diff) +1
-1- return zram->meta != NULL; + return zram->disksize != 0;we don't set ->disksize to 0 when create device. and I think it's better to use refcount here, but set it to 0 during device creation. (see the patch below)
There was a reason I didn't use refcount there.
I should have written down it.
Quoted 8 lines
We need something to prevent further I/O handling on other CPUs. Otherwise, it's livelock. For example, new 'A' I/O rw path on CPU 1 can see non-zero refcount if another CPU is going on rw. Then, another new 'B' I/O rw path on CPU 2 can see non-zero refcount if A I/O is going on. Then, another new 'C' I/O rw path on CPU 3 can see non-zero refcount if B I/O is going on. Finally, 'A' IO is done on CPU 1 and next I/O 'D' on CPU 1 can see non-zero refcount because 'C' on CPU 3 is going on. Infinite loop.
sure, I did think about this. and I actually didn't find any reason not to use ->refcount there. if user wants to reset the device, he first should umount it to make bdev->bd_holders check happy. and that's where IOs will be failed. so it makes sense to switch to ->refcount there, IMHO.
Quoted 7 lines
Quoted 3 lines
here and later:
we can't takezram_meta_get()first and then check forinit_done(zram), because ->meta can be NULL, so it fill be ->NULL->refcount.True.
Actually, it was totally RFC I forgot adding the tag in the night but I can't escape from my shame with the escuse. Thanks!
no problem at all. you were throwing solutions all week long.
Quoted 7 lines
Quoted 4 lines
let's keep ->completion and ->refcount in zram and rename zram_meta_[get|put] to zram_[get|put].
Good idea but still want to name it as zram_meta_get/put because
zram_getnaming might confuse struct zram's refcount rather thanzram_meta. :)
no objections. but I assume we agreed to keep ->io_done completion and ->refcount in zram.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 24 lines · Sergey Senozhatsky
Hello Minchan,
Quoted 12 lines · Minchan Kim
Quoted 8 lines
Review
Quoted 4 lines
static inline int
init_done(struct zram *zram)
{(quoted diff) +1
-1- return zram->meta != NULL; + return zram->disksize != 0;we don't set ->disksize to 0 when create device. and I think it's better to use refcount here, but set it to 0 during device creation. (see the patch below)
There was a reason I didn't use refcount there.
I should have written down it.Quoted 8 lines
We need something to prevent further I/O handling on other CPUs. Otherwise, it's livelock. For example, new 'A' I/O rw path on CPU 1 can see non-zero refcount if another CPU is going on rw. Then, another new 'B' I/O rw path on CPU 2 can see non-zero refcount if A I/O is going on. Then, another new 'C' I/O rw path on CPU 3 can see non-zero refcount if B I/O is going on. Finally, 'A' IO is done on CPU 1 and next I/O 'D' on CPU 1 can see non-zero refcount because 'C' on CPU 3 is going on. Infinite loop.
Quoted 4 lines
sure, I did think about this. and I actually didn't find any reason not to use ->refcount there. if user wants to reset the device, he first should umount it to make
bdev->bd_holderscheck happy. and that's where IOs will be failed. so it makes sense to switch to ->refcount there, IMHO.
If we use zram as block device itself(not a fs or swap) and open the block device as !FMODE_EXCL, bd_holders will be void.
Another topic: As I didn't see enough fs/block_dev.c bd_holders in zram would be mess. I guess we need to study hotplug of device and implement it for zram reset rather than strange own konb. It should go TODO. :(
Quoted 24 lines
Quoted 9 lines
Quoted 5 lines
here and later:
we can't takezram_meta_get()first and then check forinit_done(zram), because ->meta can be NULL, so it fill be ->NULL->refcount.True.
Actually, it was totally RFC I forgot adding the tag in the night but I can't escape from my shame with the escuse. Thanks!no problem at all. you were throwing solutions all week long.
Quoted 7 lines
Quoted 4 lines
let's keep ->completion and ->refcount in zram and rename zram_meta_[get|put] to zram_[get|put].
Good idea but still want to name it as zram_meta_get/put because
zram_getnaming might confuse struct zram's refcount rather thanzram_meta. :)no objections. but I assume we agreed to keep ->io_done completion and ->refcount in zram.
-ss
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 8 lines · Minchan Kim
Quoted 4 lines
sure, I did think about this. and I actually didn't find any reason not to use ->refcount there. if user wants to reset the device, he first should umount it to make
bdev->bd_holderscheck happy. and that's where IOs will be failed. so it makes sense to switch to ->refcount there, IMHO.If we use zram as block device itself(not a fs or swap) and open the block device as !FMODE_EXCL,
bd_holderswill be void.
hm.
I don't mind to use ->disksize there, but personally I'd maybe prefer to use ->refcount, which just looks less hacky. zram's most common use cases are coming from ram swap device or ram device with fs. so it looks a bit like we care about some corner case here.
just my opinion, no objections against ->disksize != 0.
I need to check fs/block_dev. can we switch away from ->bd_holders?
Quoted 3 lines
Another topic: As I didn't see enough
fs/block_dev.cbd_holdersin zram would be mess. I guess we need to study hotplug of device and implement it for zram reset rather than strange own konb. It should go TODO. :(
ok, need to investigate this later.
let's land current activities first.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 10 lines · Sergey Senozhatsky
Quoted 8 lines · Minchan Kim
Quoted 4 lines
sure, I did think about this. and I actually didn't find any reason not to use ->refcount there. if user wants to reset the device, he first should umount it to make
bdev->bd_holderscheck happy. and that's where IOs will be failed. so it makes sense to switch to ->refcount there, IMHO.If we use zram as block device itself(not a fs or swap) and open the block device as !FMODE_EXCL,
bd_holderswill be void.
Quoted 5 lines
hm.
I don't mind to use ->disksize there, but personally I'd maybe prefer to use ->refcount, which just looks less hacky. zram's most common use cases are coming from ram swap device or ram device with fs. so it looks a bit like we care about some corner case here.
Maybe, but I always test zram with dd so it's not a corner case for me. :)
Quoted 2 lines
just my opinion, no objections against ->disksize != 0.
Thanks. It's a draft for v2. Please review.
BTW, you pointed out race between bdev_open/close and reset and it's cleary bug although it's rare in real practice.
So, I want to fix it earlier than this patch and mark it as -stable if we can fix it easily like Ganesh's work.
If it gets landing, we could make this patch rebased on it.
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
the patch mosly looks good, except for one place:
Review static ssize_t disksize_store(struct device *dev, · line 816
Quoted 6 lines · Minchan Kim
(quoted diff) +2
-0@@ -783,6 +812,8 @@ static ssize_t disksize_store(struct device *dev, goto out_destroy_comp; } + init_waitqueue_head(&zram->io_done); + zram_meta_get(zram);
it was
+ init_completion(&zram->io_done);
+ atomic_set(&zram->refcount, 1);
I think we need to replace zram_meta_get(zram) with atomic_set(&zram->refcount, 1).
->refcount is 0 by default and atomic_inc_not_zero(&zram->refcount) will not increment it here, nor anywhere else.
Quoted 8 lines
(quoted diff) +0
-1zram->meta = meta; zram->comp = comp; zram->disksize = disksize;@@ -838,8 +869,8 @@ static ssize_t reset_store(struct device *dev, /* Make sure all pending I/O is finished */ fsync_bdev(bdev); bdput(bdev); -
[..]
Review static int create_device(struct zram *zram, int device_id) · line 1078
Quoted 5 lines
(quoted diff) +1
-0@@ -1041,6 +1075,7 @@ static int create_device(struct zram *zram, int device_id) int ret = -ENOMEM; init_rwsem(&zram->init_lock); + atomic_set(&zram->refcount, 0);
sorry, I forgot that zram is kzalloc()-ated. so we can drop
atomic_set(&zram->refcount, 0)
Review static int create_device(struct zram *zram, int device_id) · line 1080
Quoted 2 lines
(quoted diff) +0
-0zram->queue = blk_alloc_queue(GFP_KERNEL); if (!zram->queue) {
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 35 lines · Sergey Senozhatsky
the patch mosly looks good, except for one place:
Review static ssize_t disksize_store(struct device *dev, · line 816
Quoted 6 lines · Minchan Kim
(quoted diff) +2
-0@@ -783,6 +812,8 @@ static ssize_t disksize_store(struct device *dev, goto out_destroy_comp; } + init_waitqueue_head(&zram->io_done); + zram_meta_get(zram);it was
(quoted diff) +2
-0+ init_completion(&zram->io_done); + atomic_set(&zram->refcount, 1);I think we need to replace
zram_meta_get(zram)withatomic_set(&zram->refcount, 1).->refcount is 0 by default and
atomic_inc_not_zero(&zram->refcount)will not increment it here, nor anywhere else.Quoted 8 lines
(quoted diff) +0
-1zram->meta = meta; zram->comp = comp; zram->disksize = disksize;@@ -838,8 +869,8 @@ static ssize_t reset_store(struct device *dev, /* Make sure all pending I/O is finished */ fsync_bdev(bdev); bdput(bdev); -[..]
Quoted 3 lines
(quoted diff) +0
-0@@ -1041,6 +1075,7 @@ static int create_device(struct zram *zram, int device_id) int ret = -ENOMEM;
Quoted 7 lines
Review static int create_device(struct zram *zram, int device_id) · line 1078
Quoted 2 lines
(quoted diff) +1
-0init_rwsem(&zram->init_lock); + atomic_set(&zram->refcount, 0);sorry, I forgot that zram is kzalloc()-ated. so we can drop
atomic_set(&zram->refcount, 0)
Everything are fixed. Ready to send a patch. But before sending, hope we fix umount race issue first.
Thanks a lot, Sergey!
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 3 lines · Minchan Kim
Everything are fixed. Ready to send a patch. But before sending, hope we fix umount race issue first.
Thanks a lot, Minchan!
OK, surely I don't mind to fix the umount race first, I just didn't want to interrupt you in the middle of locking rework. the race is hardly possible, but still.
I didn't review Ganesh's patch yet, I'll try to find some time to take a look later this day.
I'm also planning to send a small `struct zram' cleanup patch by the end of this day.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerRe: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
In reply to Minchan KimQuoted 146 lines · Minchan Kim
Quoted 10 lines · Sergey Senozhatsky
Quoted 8 lines · Minchan Kim
Quoted 4 lines
sure, I did think about this. and I actually didn't find any reason not to use ->refcount there. if user wants to reset the device, he first should umount it to make
bdev->bd_holderscheck happy. and that's where IOs will be failed. so it makes sense to switch to ->refcount there, IMHO.If we use zram as block device itself(not a fs or swap) and open the block device as !FMODE_EXCL,
bd_holderswill be void.Quoted 5 lines
hm.
I don't mind to use ->disksize there, but personally I'd maybe prefer to use ->refcount, which just looks less hacky. zram's most common use cases are coming from ram swap device or ram device with fs. so it looks a bit like we care about some corner case here.Maybe, but I always test zram with dd so it's not a corner case for me. :)
Quoted 2 lines
just my opinion, no objections against ->disksize != 0.
Thanks. It's a draft for v2. Please review.
BTW, you pointed out race between bdev_open/close and reset and it's cleary bug although it's rare in real practice.
So, I want to fix it earlier than this patch and mark it as -stable if we can fix it easily like Ganesh's work.
If it gets landing, we could make this patch rebased on it.From
699502b4e0c84b3d7b33f8754cf1c0109b16c012Mon Sep 17 00:00:00 2001
From: Minchan Kim <[email protected]>
Date: Mon, 2 Feb 2015 10:36:28 +0900
Subject: [PATCH v2] zram: removeinit_lockinzram_make_requestAdmin could reset zram during I/O operation going on so we have used
zram->init_lockas read-side lock in I/O path to prevent sudden zram meta freeing.However, the
init_lockis really troublesome.
We can't do callzram_meta_allocunderinit_lockdue to lockdep splat becausezram_rw_pageis one of the function under reclaim path and hold it asread_lockwhile other places in process context hold it aswrite_lock. So, we have used allocation out of the lock to avoid lockdep warn but it's not good for readability and fainally, I met another lockdep splat betweeninit_lockandcpu_hotplugfromkmem_cache_destroyduring working zsmalloc compaction. :(Yes, the ideal is to remove horrible
init_lockof zram in rw path. This patch removes it in rw path and instead, add atomic refcount for meta lifetime management and completion to free meta in process context. It's important to free meta in process context because some of resource destruction needs mutex lock, which could be held if we releases the resource in reclaim context so it's deadlock, again.Signed-off-by Sergey Senozhatsky <[email protected]>Signed-off-by Minchan Kim <[email protected]>--- drivers/block/zram/zram_drv.c | 85 ++++++++++++++++++++++++++++++------------- drivers/block/zram/zram_drv.h | 20 +++++----- 2 files changed, 71 insertions(+), 34 deletions(-)drivers/block/zram/zram_drv.c +36
-7@@ -55,7 +55,7 @@ static DEVICE_ATTR_RO(name);static inline int init_done (struct zram *zram) { -return zram->meta !=NULL ; +return zram->disksize !=0 ; }static inline struct zram *dev_to_zram (struct device *dev)@@ -358,6 +358,18 @@ out_error:return NULL ; } +static inline bool zram_meta_get (struct zram *zram) +{ +if (atomic_inc_not_zero (&zram->refcount)) +return true; +return false; +} + +static inline void zram_meta_put (struct zram *zram) +{ +atomic_dec (&zram->refcount); +} +static void update_position (u32 *index,int *offset,struct bio_vec *bvec) {if (*offset + bvec->bv_len >=PAGE_SIZE )@@ -719,6 +731,10 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { +struct zram_meta *meta; +struct zcomp *comp; +u64 disksize; +down_write (&zram->init_lock); zram->limit_pages =0 ;@@ -728,19 +744,32 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } -zcomp_destroy (zram->comp); - zram->max_comp_streams =1 ; -zram_meta_free (zram->meta, zram->disksize); - zram->meta =NULL ; + meta = zram->meta; + comp = zram->comp; + disksize = zram->disksize; + zram->disksize =0 ; +/* +* ->refcount will go down to 0 eventually and rw handler cannot +* handle further I/O by init_done checking. +*/ +zram_meta_put (zram); +/* +* We want to free zram_meta in process context to avoid +* deadlock between reclaim path and any other locks +*/ +wait_event (zram->io_done,atomic_read (&zram->refcount) ==0 ); +/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats)); + zram->max_comp_streams =1 ; - zram->disksize =0 ;if (reset_capacity)set_capacity (zram->disk,0 );up_write (&zram->init_lock); - +/* I/O operation under all of CPU are done so let's free */ +zram_meta_free (meta, disksize); +zcomp_destroy (comp);/* * Revalidate disk out of the init_lock to avoid lockdep splat. * It's okay because disk's capacity is protected by init_lock
drivers/block/zram/zram_drv.c static ssize_t disksize_store(struct device *dev, · line 816
Quoted 5 lines
drivers/block/zram/zram_drv.c +2
-0@@ -783,6 +812,8 @@ static ssize_t disksize_store(struct device *dev, goto out_destroy_comp; } +init_waitqueue_head (&zram->io_done); +zram_meta_get (zram);
Argh, It should be
atomic_set(&zram->refcount, 1);
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
In reply to Minchan KimQuoted 3 lines · Minchan Kim
I tested it with multiple dd processes.
Hello,
fio test (http://manpages.ubuntu.com/manpages/natty/man1/fio.1.html) configs are attached, so we can run fio on different h/w (works against zram with fs mounted at /mnt/)
for i in ./test-fio-*; do fio ./$i; rm bg*; done
in short
randread
BASE READ: io=1600.0MB, aggrb=3747.8MB/s, minb=959250KB/s, maxb=982.82MB/s, mint=407msec, maxt=427msec
SRCU READ: io=1600.0MB, aggrb=3782.6MB/s, minb=968321KB/s, maxb=977.11MB/s, mint=409msec, maxt=423msec
random read/write
BASE READ: io=820304KB, aggrb=1296.3MB/s, minb=331838KB/s, maxb=333456KB/s, mint=615msec, maxt=618msec
SRCU READ: io=820304KB, aggrb=1261.6MB/s, minb=322954KB/s, maxb=335639KB/s, mint=611msec, maxt=635msec
BASE WRITE: io=818096KB, aggrb=1292.8MB/s, minb=330944KB/s, maxb=332559KB/s, mint=615msec, maxt=618msec SRCU WRITE: io=818096KB, aggrb=1258.2MB/s, minb=322085KB/s, maxb=334736KB/s, mint=611msec, maxt=635msec
random write
BASE WRITE: io=1600.0MB, aggrb=692184KB/s, minb=173046KB/s, maxb=174669KB/s, mint=2345msec, maxt=2367msec SRCU WRITE: io=1600.0MB, aggrb=672577KB/s, minb=168144KB/s, maxb=174149KB/s, mint=2352msec, maxt=2436msec
detailed per-process metrics:
************* BASE *************
$ for i in ./test-fio-*; do fio ./$i; rm bg*; done bgreader: (g=0): rw=randread, bs=4K-4K/4K-4K/4K-4K, ioengine=libaio, iodepth=32 ... fio-2.2.5 Starting 4 processes bgreader: Laying out IO file(s) (1 file(s) / 400MB) bgreader: Laying out IO file(s) (1 file(s) / 400MB) bgreader: Laying out IO file(s) (1 file(s) / 400MB) bgreader: Laying out IO file(s) (1 file(s) / 400MB)
bgreader: (groupid=0, jobs=1): err= 0: pid=10792: Thu Jan 29 19:43:30 2015
read : io=409600KB, bw=959251KB/s, iops=239812, runt= 427msec
slat (usec): min=2, max=28, avg= 2.43, stdev= 0.62
clat (usec): min=1, max=151, avg=122.97, stdev= 4.99
lat (usec): min=4, max=174, avg=125.52, stdev= 5.08
clat percentiles (usec):
| 1.00th=[ 106], 5.00th=[ 116], 10.00th=[ 121], 20.00th=[ 122],
| 30.00th=[ 122], 40.00th=[ 123], 50.00th=[ 123], 60.00th=[ 124],
| 70.00th=[ 124], 80.00th=[ 124], 90.00th=[ 125], 95.00th=[ 127],
| 99.00th=[ 143], 99.50th=[ 143], 99.90th=[ 145], 99.95th=[ 145],
| 99.99th=[ 147]
lat (usec) : 2=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.97%
cpu : usr=39.11%, sys=55.04%, ctx=82, majf=0, minf=38
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgreader: (groupid=0, jobs=1): err= 0: pid=10793: Thu Jan 29 19:43:30 2015
read : io=409600KB, bw=982254KB/s, iops=245563, runt= 417msec
slat (usec): min=2, max=60, avg= 2.42, stdev= 0.56
clat (usec): min=1, max=187, avg=123.17, stdev= 4.02
lat (usec): min=3, max=189, avg=125.71, stdev= 4.09
clat percentiles (usec):
| 1.00th=[ 105], 5.00th=[ 121], 10.00th=[ 121], 20.00th=[ 122],
| 30.00th=[ 122], 40.00th=[ 123], 50.00th=[ 123], 60.00th=[ 124],
| 70.00th=[ 124], 80.00th=[ 125], 90.00th=[ 126], 95.00th=[ 127],
| 99.00th=[ 133], 99.50th=[ 135], 99.90th=[ 141], 99.95th=[ 175],
| 99.99th=[ 187]
lat (usec) : 2=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.97%
cpu : usr=40.53%, sys=55.88%, ctx=43, majf=0, minf=39
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgreader: (groupid=0, jobs=1): err= 0: pid=10794: Thu Jan 29 19:43:30 2015
read : io=409600KB, bw=982.82MB/s, iops=251597, runt= 407msec
slat (usec): min=2, max=50, avg= 2.43, stdev= 0.58
clat (usec): min=2, max=174, avg=123.07, stdev= 3.97
lat (usec): min=5, max=197, avg=125.62, stdev= 4.02
clat percentiles (usec):
| 1.00th=[ 106], 5.00th=[ 120], 10.00th=[ 121], 20.00th=[ 122],
| 30.00th=[ 122], 40.00th=[ 123], 50.00th=[ 123], 60.00th=[ 123],
| 70.00th=[ 124], 80.00th=[ 125], 90.00th=[ 126], 95.00th=[ 129],
| 99.00th=[ 135], 99.50th=[ 137], 99.90th=[ 145], 99.95th=[ 153],
| 99.99th=[ 169]
lat (usec) : 4=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.98%
cpu : usr=41.03%, sys=57.74%, ctx=42, majf=0, minf=37
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgreader: (groupid=0, jobs=1): err= 0: pid=10795: Thu Jan 29 19:43:30 2015
read : io=409600KB, bw=980.41MB/s, iops=250980, runt= 408msec
slat (usec): min=2, max=357, avg= 2.45, stdev= 1.32
clat (usec): min=1, max=498, avg=124.23, stdev=12.09
lat (usec): min=4, max=501, avg=126.80, stdev=12.33
clat percentiles (usec):
| 1.00th=[ 108], 5.00th=[ 121], 10.00th=[ 122], 20.00th=[ 122],
| 30.00th=[ 123], 40.00th=[ 123], 50.00th=[ 124], 60.00th=[ 124],
| 70.00th=[ 124], 80.00th=[ 125], 90.00th=[ 126], 95.00th=[ 127],
| 99.00th=[ 139], 99.50th=[ 141], 99.90th=[ 274], 99.95th=[ 350],
| 99.99th=[ 498]
lat (usec) : 2=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.61%, 500=0.37%
cpu : usr=41.18%, sys=58.09%, ctx=86, majf=0, minf=38
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32Run status group 0 (all jobs):
READ: io=1600.0MB, aggrb=3747.8MB/s, minb=959250KB/s, maxb=982.82MB/s, mint=407msec, maxt=427msec
Disk stats (read/write):
zram0: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00%
bgupdater: (g=0): rw=randrw, bs=4K-4K/4K-4K/4K-4K, ioengine=libaio, iodepth=32
...
fio-2.2.5
Starting 4 processes
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: (groupid=0, jobs=1): err= 0: pid=10803: Thu Jan 29 19:43:33 2015
read : io=205076KB, bw=332916KB/s, iops=83228, runt= 616msec
slat (usec): min=2, max=38, avg= 2.53, stdev= 0.71
clat (usec): min=18, max=912, avg=187.23, stdev=18.71
lat (usec): min=22, max=915, avg=189.88, stdev=18.74
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 181], 40.00th=[ 183], 50.00th=[ 187], 60.00th=[ 191],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 203], 95.00th=[ 209],
| 99.00th=[ 223], 99.50th=[ 233], 99.90th=[ 258], 99.95th=[ 290],
| 99.99th=[ 908]
bw (KB /s): min=332504, max=332504, per=25.05%, avg=332504.00, stdev= 0.00
write: io=204524KB, bw=332019KB/s, iops=83004, runt= 616msec
slat (usec): min=4, max=731, avg= 6.21, stdev= 3.42
clat (usec): min=1, max=910, avg=187.36, stdev=17.80
lat (usec): min=8, max=918, avg=193.71, stdev=18.16
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 181], 40.00th=[ 185], 50.00th=[ 187], 60.00th=[ 191],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 203], 95.00th=[ 209],
| 99.00th=[ 223], 99.50th=[ 231], 99.90th=[ 258], 99.95th=[ 282],
| 99.99th=[ 908]
bw (KB /s): min=332544, max=332544, per=25.12%, avg=332544.00, stdev= 0.00
lat (usec) : 2=0.01%, 20=0.01%, 50=0.01%, 100=0.01%, 250=99.80%
lat (usec) : 500=0.15%, 1000=0.03%
cpu : usr=30.36%, sys=68.99%, ctx=185, majf=0, minf=7
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgupdater: (groupid=0, jobs=1): err= 0: pid=10804: Thu Jan 29 19:43:33 2015
read : io=205076KB, bw=331838KB/s, iops=82959, runt= 618msec
slat (usec): min=2, max=99, avg= 2.54, stdev= 0.88
clat (usec): min=13, max=4676, avg=187.64, stdev=78.01
lat (usec): min=16, max=4678, avg=190.29, stdev=78.01
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 181], 40.00th=[ 183], 50.00th=[ 187], 60.00th=[ 189],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 201], 95.00th=[ 207],
| 99.00th=[ 221], 99.50th=[ 239], 99.90th=[ 342], 99.95th=[ 462],
| 99.99th=[ 4704]
bw (KB /s): min=330248, max=330248, per=24.88%, avg=330248.00, stdev= 0.00
write: io=204524KB, bw=330945KB/s, iops=82736, runt= 618msec
slat (usec): min=4, max=4458, avg= 6.24, stdev=19.77
clat (usec): min=2, max=4673, avg=187.90, stdev=80.44
lat (usec): min=6, max=4680, avg=194.28, stdev=82.87
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 181], 40.00th=[ 183], 50.00th=[ 187], 60.00th=[ 189],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 201], 95.00th=[ 207],
| 99.00th=[ 219], 99.50th=[ 233], 99.90th=[ 342], 99.95th=[ 462],
| 99.99th=[ 4640]
bw (KB /s): min=330328, max=330328, per=24.95%, avg=330328.00, stdev= 0.00
lat (usec) : 4=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.67%, 500=0.28%
lat (msec) : 10=0.03%
cpu : usr=30.10%, sys=68.45%, ctx=67, majf=0, minf=10
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgupdater: (groupid=0, jobs=1): err= 0: pid=10805: Thu Jan 29 19:43:33 2015
read : io=205076KB, bw=332916KB/s, iops=83228, runt= 616msec
slat (usec): min=2, max=4810, avg= 2.63, stdev=21.50
clat (usec): min=14, max=5018, avg=187.75, stdev=99.05
lat (usec): min=16, max=5020, avg=190.49, stdev=101.38
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 175],
| 30.00th=[ 179], 40.00th=[ 183], 50.00th=[ 185], 60.00th=[ 189],
| 70.00th=[ 191], 80.00th=[ 195], 90.00th=[ 201], 95.00th=[ 207],
| 99.00th=[ 219], 99.50th=[ 229], 99.90th=[ 262], 99.95th=[ 956],
| 99.99th=[ 5024]
bw (KB /s): min=331056, max=331056, per=24.94%, avg=331056.00, stdev= 0.00
write: io=204524KB, bw=332019KB/s, iops=83004, runt= 616msec
slat (usec): min=4, max=49, avg= 6.11, stdev= 1.01
clat (usec): min=2, max=5018, avg=186.97, stdev=70.14
lat (usec): min=6, max=5024, avg=193.22, stdev=70.15
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 179], 40.00th=[ 183], 50.00th=[ 187], 60.00th=[ 189],
| 70.00th=[ 193], 80.00th=[ 195], 90.00th=[ 201], 95.00th=[ 207],
| 99.00th=[ 217], 99.50th=[ 227], 99.90th=[ 255], 99.95th=[ 948],
| 99.99th=[ 5024]
bw (KB /s): min=331320, max=331320, per=25.03%, avg=331320.00, stdev= 0.00
lat (usec) : 4=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.83%, 500=0.09%, 1000=0.03%
lat (msec) : 10=0.03%
cpu : usr=30.52%, sys=68.34%, ctx=70, majf=0, minf=8
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgupdater: (groupid=0, jobs=1): err= 0: pid=10806: Thu Jan 29 19:43:33 2015
read : io=205076KB, bw=333457KB/s, iops=83364, runt= 615msec
slat (usec): min=2, max=40, avg= 2.53, stdev= 0.69
clat (usec): min=18, max=287, avg=186.73, stdev=12.94
lat (usec): min=21, max=289, avg=189.38, stdev=12.98
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 181], 40.00th=[ 183], 50.00th=[ 187], 60.00th=[ 189],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 203], 95.00th=[ 207],
| 99.00th=[ 221], 99.50th=[ 229], 99.90th=[ 258], 99.95th=[ 270],
| 99.99th=[ 286]
bw (KB /s): min=332680, max=332680, per=25.06%, avg=332680.00, stdev= 0.00
write: io=204524KB, bw=332559KB/s, iops=83139, runt= 615msec
slat (usec): min=4, max=28, avg= 6.17, stdev= 1.02
clat (usec): min=2, max=285, avg=186.95, stdev=12.80
lat (usec): min=8, max=294, avg=193.26, stdev=12.89
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 181], 40.00th=[ 185], 50.00th=[ 187], 60.00th=[ 191],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 203], 95.00th=[ 209],
| 99.00th=[ 221], 99.50th=[ 227], 99.90th=[ 251], 99.95th=[ 262],
| 99.99th=[ 274]
bw (KB /s): min=332672, max=332672, per=25.13%, avg=332672.00, stdev= 0.00
lat (usec) : 4=0.01%, 20=0.01%, 50=0.01%, 100=0.01%, 250=99.85%
lat (usec) : 500=0.14%
cpu : usr=30.73%, sys=69.11%, ctx=63, majf=0, minf=7
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32Run status group 0 (all jobs):
READ: io=820304KB, aggrb=1296.3MB/s, minb=331838KB/s, maxb=333456KB/s, mint=615msec, maxt=618msec
WRITE: io=818096KB, aggrb=1292.8MB/s, minb=330944KB/s, maxb=332559KB/s, mint=615msec, maxt=618msec
Disk stats (read/write):
zram0: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00%
bgwriter: (g=0): rw=randwrite, bs=4K-4K/4K-4K/4K-4K, ioengine=libaio, iodepth=32
...
fio-2.2.5
Starting 4 processes
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
Jobs: 4 (f=4)
bgwriter: (groupid=0, jobs=1): err= 0: pid=10814: Thu Jan 29 19:43:35 2015
write: io=409600KB, bw=174150KB/s, iops=43537, runt= 2352msec
slat (usec): min=11, max=3866, avg=20.72, stdev=14.58
clat (usec): min=2, max=5074, avg=712.72, stdev=92.15
lat (usec): min=20, max=5094, avg=733.63, stdev=93.77
clat percentiles (usec):
| 1.00th=[ 612], 5.00th=[ 676], 10.00th=[ 684], 20.00th=[ 700],
| 30.00th=[ 700], 40.00th=[ 708], 50.00th=[ 708], 60.00th=[ 716],
| 70.00th=[ 716], 80.00th=[ 724], 90.00th=[ 732], 95.00th=[ 740],
| 99.00th=[ 820], 99.50th=[ 1004], 99.90th=[ 1640], 99.95th=[ 2128],
| 99.99th=[ 5088]
bw (KB /s): min=170904, max=174808, per=24.97%, avg=172816.00, stdev=1847.92
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.24%
lat (usec) : 750=96.28%, 1000=2.94%
lat (msec) : 2=0.46%, 4=0.03%, 10=0.03%
cpu : usr=10.67%, sys=88.52%, ctx=267, majf=0, minf=8
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgwriter: (groupid=0, jobs=1): err= 0: pid=10815: Thu Jan 29 19:43:35 2015
write: io=409600KB, bw=173046KB/s, iops=43261, runt= 2367msec
slat (usec): min=12, max=1459, avg=20.87, stdev=10.16
clat (usec): min=2, max=2804, avg=717.25, stdev=69.33
lat (usec): min=22, max=2825, avg=738.31, stdev=70.84
clat percentiles (usec):
| 1.00th=[ 628], 5.00th=[ 676], 10.00th=[ 692], 20.00th=[ 700],
| 30.00th=[ 708], 40.00th=[ 708], 50.00th=[ 716], 60.00th=[ 716],
| 70.00th=[ 724], 80.00th=[ 732], 90.00th=[ 740], 95.00th=[ 748],
| 99.00th=[ 812], 99.50th=[ 1080], 99.90th=[ 1800], 99.95th=[ 1992],
| 99.99th=[ 2640]
bw (KB /s): min=171160, max=172768, per=24.82%, avg=171786.00, stdev=761.48
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.11%
lat (usec) : 750=95.99%, 1000=3.25%
lat (msec) : 2=0.60%, 4=0.04%
cpu : usr=10.65%, sys=88.64%, ctx=502, majf=0, minf=9
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgwriter: (groupid=0, jobs=1): err= 0: pid=10816: Thu Jan 29 19:43:35 2015
write: io=409600KB, bw=174224KB/s, iops=43555, runt= 2351msec
slat (usec): min=12, max=280, avg=20.67, stdev= 2.58
clat (usec): min=2, max=1116, avg=712.42, stdev=31.10
lat (usec): min=20, max=1145, avg=733.30, stdev=31.89
clat percentiles (usec):
| 1.00th=[ 628], 5.00th=[ 676], 10.00th=[ 684], 20.00th=[ 700],
| 30.00th=[ 708], 40.00th=[ 708], 50.00th=[ 716], 60.00th=[ 716],
| 70.00th=[ 724], 80.00th=[ 724], 90.00th=[ 740], 95.00th=[ 748],
| 99.00th=[ 772], 99.50th=[ 788], 99.90th=[ 1020], 99.95th=[ 1048],
| 99.99th=[ 1080]
bw (KB /s): min=171632, max=174888, per=25.00%, avg=173026.00, stdev=1359.10
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.11%
lat (usec) : 750=94.50%, 1000=5.19%
lat (msec) : 2=0.19%
cpu : usr=10.85%, sys=88.77%, ctx=472, majf=0, minf=8
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgwriter: (groupid=0, jobs=1): err= 0: pid=10817: Thu Jan 29 19:43:35 2015
write: io=409600KB, bw=174670KB/s, iops=43667, runt= 2345msec
slat (usec): min=11, max=6296, avg=20.66, stdev=24.99
clat (usec): min=2, max=5509, avg=708.98, stdev=90.54
lat (usec): min=22, max=7099, avg=729.83, stdev=94.22
clat percentiles (usec):
| 1.00th=[ 604], 5.00th=[ 676], 10.00th=[ 684], 20.00th=[ 700],
| 30.00th=[ 700], 40.00th=[ 708], 50.00th=[ 708], 60.00th=[ 716],
| 70.00th=[ 716], 80.00th=[ 724], 90.00th=[ 732], 95.00th=[ 740],
| 99.00th=[ 756], 99.50th=[ 772], 99.90th=[ 940], 99.95th=[ 1912],
| 99.99th=[ 5536]
bw (KB /s): min=170792, max=176064, per=25.05%, avg=173360.00, stdev=2157.10
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.32%
lat (usec) : 750=97.68%, 1000=1.92%
lat (msec) : 2=0.03%, 10=0.03%
cpu : usr=10.79%, sys=88.53%, ctx=252, majf=0, minf=6
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32Run status group 0 (all jobs):
WRITE: io=1600.0MB, aggrb=692184KB/s, minb=173046KB/s, maxb=174669KB/s, mint=2345msec, maxt=2367msec
Disk stats (read/write):
zram0: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00%
==========================================================================================================
************* SRCU *************
$ for i in ./test-fio-*; do fio ./$i; rm bg*; done bgreader: (g=0): rw=randread, bs=4K-4K/4K-4K/4K-4K, ioengine=libaio, iodepth=32 ... fio-2.2.5 Starting 4 processes bgreader: Laying out IO file(s) (1 file(s) / 400MB) bgreader: Laying out IO file(s) (1 file(s) / 400MB) bgreader: Laying out IO file(s) (1 file(s) / 400MB) bgreader: Laying out IO file(s) (1 file(s) / 400MB)
bgreader: (groupid=0, jobs=1): err= 0: pid=11578: Thu Jan 29 19:45:01 2015
read : io=409600KB, bw=986988KB/s, iops=246746, runt= 415msec
slat (usec): min=2, max=708, avg= 2.48, stdev= 2.29
clat (usec): min=2, max=839, avg=124.69, stdev=16.01
lat (usec): min=4, max=842, avg=127.29, stdev=16.29
clat percentiles (usec):
| 1.00th=[ 121], 5.00th=[ 122], 10.00th=[ 122], 20.00th=[ 122],
| 30.00th=[ 123], 40.00th=[ 123], 50.00th=[ 123], 60.00th=[ 123],
| 70.00th=[ 124], 80.00th=[ 124], 90.00th=[ 125], 95.00th=[ 131],
| 99.00th=[ 139], 99.50th=[ 223], 99.90th=[ 243], 99.95th=[ 282],
| 99.99th=[ 836]
lat (usec) : 4=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.90%, 500=0.05%, 1000=0.03%
cpu : usr=40.48%, sys=57.11%, ctx=46, majf=0, minf=39
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgreader: (groupid=0, jobs=1): err= 0: pid=11579: Thu Jan 29 19:45:01 2015
read : io=409600KB, bw=968322KB/s, iops=242080, runt= 423msec
slat (usec): min=2, max=88, avg= 2.45, stdev= 0.67
clat (usec): min=1, max=213, avg=123.41, stdev= 4.23
lat (usec): min=3, max=216, avg=125.97, stdev= 4.30
clat percentiles (usec):
| 1.00th=[ 108], 5.00th=[ 121], 10.00th=[ 122], 20.00th=[ 122],
| 30.00th=[ 123], 40.00th=[ 123], 50.00th=[ 123], 60.00th=[ 124],
| 70.00th=[ 124], 80.00th=[ 124], 90.00th=[ 125], 95.00th=[ 126],
| 99.00th=[ 139], 99.50th=[ 139], 99.90th=[ 145], 99.95th=[ 195],
| 99.99th=[ 213]
lat (usec) : 2=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.97%
cpu : usr=39.24%, sys=55.79%, ctx=85, majf=0, minf=39
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgreader: (groupid=0, jobs=1): err= 0: pid=11580: Thu Jan 29 19:45:01 2015
read : io=409600KB, bw=984615KB/s, iops=246153, runt= 416msec
slat (usec): min=2, max=51, avg= 2.42, stdev= 0.63
clat (usec): min=2, max=171, avg=123.01, stdev= 3.46
lat (usec): min=4, max=174, avg=125.55, stdev= 3.50
clat percentiles (usec):
| 1.00th=[ 120], 5.00th=[ 121], 10.00th=[ 121], 20.00th=[ 122],
| 30.00th=[ 122], 40.00th=[ 122], 50.00th=[ 122], 60.00th=[ 123],
| 70.00th=[ 123], 80.00th=[ 123], 90.00th=[ 124], 95.00th=[ 126],
| 99.00th=[ 141], 99.50th=[ 143], 99.90th=[ 145], 99.95th=[ 155],
| 99.99th=[ 171]
lat (usec) : 4=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.97%
cpu : usr=40.38%, sys=56.25%, ctx=82, majf=0, minf=37
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgreader: (groupid=0, jobs=1): err= 0: pid=11581: Thu Jan 29 19:45:01 2015
read : io=409600KB, bw=977.11MB/s, iops=250366, runt= 409msec
slat (usec): min=2, max=236, avg= 2.46, stdev= 0.98
clat (usec): min=1, max=370, avg=124.55, stdev=11.16
lat (usec): min=4, max=372, avg=127.12, stdev=11.37
clat percentiles (usec):
| 1.00th=[ 107], 5.00th=[ 121], 10.00th=[ 122], 20.00th=[ 122],
| 30.00th=[ 123], 40.00th=[ 123], 50.00th=[ 124], 60.00th=[ 124],
| 70.00th=[ 124], 80.00th=[ 125], 90.00th=[ 125], 95.00th=[ 126],
| 99.00th=[ 175], 99.50th=[ 223], 99.90th=[ 225], 99.95th=[ 278],
| 99.99th=[ 370]
lat (usec) : 2=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.92%, 500=0.06%
cpu : usr=41.56%, sys=57.95%, ctx=48, majf=0, minf=38
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=102400/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32Run status group 0 (all jobs):
READ: io=1600.0MB, aggrb=3782.6MB/s, minb=968321KB/s, maxb=977.11MB/s, mint=409msec, maxt=423msec
Disk stats (read/write):
zram0: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00%
bgupdater: (g=0): rw=randrw, bs=4K-4K/4K-4K/4K-4K, ioengine=libaio, iodepth=32
...
fio-2.2.5
Starting 4 processes
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: Laying out IO file(s) (1 file(s) / 400MB)
bgupdater: (groupid=0, jobs=1): err= 0: pid=11592: Thu Jan 29 19:45:04 2015
read : io=205076KB, bw=334545KB/s, iops=83636, runt= 613msec
slat (usec): min=2, max=73, avg= 2.56, stdev= 0.73
clat (usec): min=19, max=310, avg=186.14, stdev=13.49
lat (usec): min=22, max=312, avg=188.81, stdev=13.55
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 179], 40.00th=[ 183], 50.00th=[ 187], 60.00th=[ 189],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 201], 95.00th=[ 207],
| 99.00th=[ 229], 99.50th=[ 251], 99.90th=[ 266], 99.95th=[ 274],
| 99.99th=[ 298]
bw (KB /s): min=333496, max=333496, per=25.82%, avg=333496.00, stdev= 0.00
write: io=204524KB, bw=333644KB/s, iops=83411, runt= 613msec
slat (usec): min=4, max=30, avg= 6.10, stdev= 1.01
clat (usec): min=2, max=307, avg=186.32, stdev=13.35
lat (usec): min=8, max=313, avg=192.56, stdev=13.47
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 177],
| 30.00th=[ 181], 40.00th=[ 183], 50.00th=[ 187], 60.00th=[ 189],
| 70.00th=[ 193], 80.00th=[ 197], 90.00th=[ 201], 95.00th=[ 207],
| 99.00th=[ 227], 99.50th=[ 249], 99.90th=[ 266], 99.95th=[ 274],
| 99.99th=[ 298]
bw (KB /s): min=333368, max=333368, per=25.88%, avg=333368.00, stdev= 0.00
lat (usec) : 4=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=99.47%, 500=0.51%
cpu : usr=30.51%, sys=69.00%, ctx=68, majf=0, minf=7
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgupdater: (groupid=0, jobs=1): err= 0: pid=11593: Thu Jan 29 19:45:04 2015
read : io=205076KB, bw=334545KB/s, iops=83636, runt= 613msec
slat (usec): min=2, max=40, avg= 2.53, stdev= 0.65
clat (usec): min=53, max=3694, avg=186.31, stdev=65.12
lat (usec): min=55, max=3697, avg=188.96, stdev=65.13
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 175],
| 30.00th=[ 179], 40.00th=[ 181], 50.00th=[ 185], 60.00th=[ 187],
| 70.00th=[ 191], 80.00th=[ 195], 90.00th=[ 201], 95.00th=[ 207],
| 99.00th=[ 231], 99.50th=[ 249], 99.90th=[ 266], 99.95th=[ 342],
| 99.99th=[ 3696]
bw (KB /s): min=335528, max=335528, per=25.97%, avg=335528.00, stdev= 0.00
write: io=204524KB, bw=333644KB/s, iops=83411, runt= 613msec
slat (usec): min=4, max=160, avg= 6.08, stdev= 1.22
clat (usec): min=2, max=3693, avg=186.31, stdev=59.38
lat (usec): min=44, max=3699, avg=192.54, stdev=59.41
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 175],
| 30.00th=[ 179], 40.00th=[ 183], 50.00th=[ 185], 60.00th=[ 189],
| 70.00th=[ 191], 80.00th=[ 195], 90.00th=[ 201], 95.00th=[ 205],
| 99.00th=[ 225], 99.50th=[ 247], 99.90th=[ 262], 99.95th=[ 346],
| 99.99th=[ 3696]
bw (KB /s): min=335376, max=335376, per=26.03%, avg=335376.00, stdev= 0.00
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=99.55%, 500=0.41%
lat (msec) : 4=0.03%
cpu : usr=30.34%, sys=68.68%, ctx=64, majf=0, minf=9
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgupdater: (groupid=0, jobs=1): err= 0: pid=11594: Thu Jan 29 19:45:04 2015
read : io=205076KB, bw=322954KB/s, iops=80738, runt= 635msec
slat (usec): min=2, max=9087, avg= 2.75, stdev=41.31
clat (usec): min=13, max=9287, avg=193.88, stdev=237.09
lat (usec): min=15, max=9293, avg=196.75, stdev=240.69
clat percentiles (usec):
| 1.00th=[ 143], 5.00th=[ 163], 10.00th=[ 169], 20.00th=[ 175],
| 30.00th=[ 179], 40.00th=[ 181], 50.00th=[ 185], 60.00th=[ 189],
| 70.00th=[ 191], 80.00th=[ 195], 90.00th=[ 201], 95.00th=[ 209],
| 99.00th=[ 255], 99.50th=[ 350], 99.90th=[ 3536], 99.95th=[ 8256],
| 99.99th=[ 9280]
bw (KB /s): min=324248, max=324248, per=25.10%, avg=324248.00, stdev= 0.00
write: io=204524KB, bw=322085KB/s, iops=80521, runt= 635msec
slat (usec): min=3, max=3360, avg= 6.20, stdev=15.91
clat (usec): min=1, max=9286, avg=192.34, stdev=210.71
lat (usec): min=6, max=9293, avg=198.68, stdev=211.36
clat percentiles (usec):
| 1.00th=[ 145], 5.00th=[ 163], 10.00th=[ 169], 20.00th=[ 175],
| 30.00th=[ 179], 40.00th=[ 183], 50.00th=[ 185], 60.00th=[ 189],
| 70.00th=[ 191], 80.00th=[ 195], 90.00th=[ 203], 95.00th=[ 209],
| 99.00th=[ 253], 99.50th=[ 346], 99.90th=[ 1032], 99.95th=[ 8256],
| 99.99th=[ 9280]
bw (KB /s): min=324656, max=324656, per=25.20%, avg=324656.00, stdev= 0.00
lat (usec) : 2=0.01%, 10=0.01%, 20=0.01%, 50=0.01%, 100=0.01%
lat (usec) : 250=98.84%, 500=0.93%, 750=0.05%
lat (msec) : 2=0.03%, 4=0.06%, 10=0.06%
cpu : usr=29.29%, sys=66.14%, ctx=91, majf=0, minf=7
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgupdater: (groupid=0, jobs=1): err= 0: pid=11595: Thu Jan 29 19:45:04 2015
read : io=205076KB, bw=335640KB/s, iops=83909, runt= 611msec
slat (usec): min=2, max=58, avg= 2.56, stdev= 0.82
clat (usec): min=17, max=470, avg=185.51, stdev=15.09
lat (usec): min=20, max=472, avg=188.18, stdev=15.16
clat percentiles (usec):
| 1.00th=[ 157], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 175],
| 30.00th=[ 179], 40.00th=[ 181], 50.00th=[ 185], 60.00th=[ 187],
| 70.00th=[ 191], 80.00th=[ 195], 90.00th=[ 201], 95.00th=[ 209],
| 99.00th=[ 233], 99.50th=[ 251], 99.90th=[ 270], 99.95th=[ 330],
| 99.99th=[ 462]
bw (KB /s): min=334544, max=334544, per=25.90%, avg=334544.00, stdev= 0.00
write: io=204524KB, bw=334736KB/s, iops=83684, runt= 611msec
slat (usec): min=4, max=90, avg= 6.07, stdev= 1.19
clat (usec): min=2, max=468, avg=185.70, stdev=14.68
lat (usec): min=8, max=474, avg=191.91, stdev=14.84
clat percentiles (usec):
| 1.00th=[ 159], 5.00th=[ 167], 10.00th=[ 171], 20.00th=[ 175],
| 30.00th=[ 179], 40.00th=[ 183], 50.00th=[ 185], 60.00th=[ 189],
| 70.00th=[ 191], 80.00th=[ 195], 90.00th=[ 203], 95.00th=[ 209],
| 99.00th=[ 231], 99.50th=[ 249], 99.90th=[ 266], 99.95th=[ 274],
| 99.99th=[ 462]
bw (KB /s): min=334440, max=334440, per=25.96%, avg=334440.00, stdev= 0.00
lat (usec) : 4=0.01%, 20=0.01%, 50=0.01%, 100=0.01%, 250=99.48%
lat (usec) : 500=0.51%
cpu : usr=30.44%, sys=68.58%, ctx=188, majf=0, minf=7
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=51269/w=51131/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32Run status group 0 (all jobs):
READ: io=820304KB, aggrb=1261.6MB/s, minb=322954KB/s, maxb=335639KB/s, mint=611msec, maxt=635msec
WRITE: io=818096KB, aggrb=1258.2MB/s, minb=322085KB/s, maxb=334736KB/s, mint=611msec, maxt=635msec
Disk stats (read/write):
zram0: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00%
bgwriter: (g=0): rw=randwrite, bs=4K-4K/4K-4K/4K-4K, ioengine=libaio, iodepth=32
...
fio-2.2.5
Starting 4 processes
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
bgwriter: Laying out IO file(s) (1 file(s) / 400MB)
Jobs: 4 (f=4)
bgwriter: (groupid=0, jobs=1): err= 0: pid=11605: Thu Jan 29 19:45:06 2015
write: io=409600KB, bw=168144KB/s, iops=42036, runt= 2436msec
slat (usec): min=10, max=5615, avg=21.50, stdev=37.53
clat (usec): min=2, max=9808, avg=738.51, stdev=364.25
lat (usec): min=40, max=9827, avg=760.22, stdev=371.10
clat percentiles (usec):
| 1.00th=[ 612], 5.00th=[ 668], 10.00th=[ 684], 20.00th=[ 692],
| 30.00th=[ 700], 40.00th=[ 700], 50.00th=[ 708], 60.00th=[ 708],
| 70.00th=[ 716], 80.00th=[ 724], 90.00th=[ 732], 95.00th=[ 756],
| 99.00th=[ 1576], 99.50th=[ 3056], 99.90th=[ 6816], 99.95th=[ 9408],
| 99.99th=[ 9792]
bw (KB /s): min=157408, max=176360, per=24.65%, avg=165770.00, stdev=8401.50
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.20%
lat (usec) : 750=94.24%, 1000=4.14%
lat (msec) : 2=0.63%, 4=0.45%, 10=0.33%
cpu : usr=10.26%, sys=85.22%, ctx=982, majf=0, minf=7
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgwriter: (groupid=0, jobs=1): err= 0: pid=11606: Thu Jan 29 19:45:06 2015
write: io=409600KB, bw=173928KB/s, iops=43481, runt= 2355msec
slat (usec): min=12, max=1173, avg=20.76, stdev= 7.22
clat (usec): min=2, max=2247, avg=713.64, stdev=49.05
lat (usec): min=22, max=2270, avg=734.58, stdev=50.26
clat percentiles (usec):
| 1.00th=[ 620], 5.00th=[ 668], 10.00th=[ 684], 20.00th=[ 692],
| 30.00th=[ 700], 40.00th=[ 708], 50.00th=[ 708], 60.00th=[ 716],
| 70.00th=[ 716], 80.00th=[ 724], 90.00th=[ 740], 95.00th=[ 756],
| 99.00th=[ 868], 99.50th=[ 924], 99.90th=[ 1368], 99.95th=[ 1608],
| 99.99th=[ 2192]
bw (KB /s): min=170448, max=175504, per=25.63%, avg=172406.00, stdev=2166.56
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.13%
lat (usec) : 750=92.77%, 1000=6.92%
lat (msec) : 2=0.17%, 4=0.01%
cpu : usr=10.66%, sys=88.66%, ctx=528, majf=0, minf=8
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgwriter: (groupid=0, jobs=1): err= 0: pid=11607: Thu Jan 29 19:45:06 2015
write: io=409600KB, bw=172900KB/s, iops=43224, runt= 2369msec
slat (usec): min=9, max=3458, avg=20.88, stdev=19.13
clat (usec): min=2, max=4190, avg=718.03, stdev=114.69
lat (usec): min=22, max=4210, avg=739.09, stdev=116.47
clat percentiles (usec):
| 1.00th=[ 620], 5.00th=[ 668], 10.00th=[ 684], 20.00th=[ 692],
| 30.00th=[ 700], 40.00th=[ 700], 50.00th=[ 708], 60.00th=[ 708],
| 70.00th=[ 716], 80.00th=[ 724], 90.00th=[ 732], 95.00th=[ 756],
| 99.00th=[ 1064], 99.50th=[ 1080], 99.90th=[ 2024], 99.95th=[ 3952],
| 99.99th=[ 4192]
bw (KB /s): min=168600, max=173752, per=25.49%, avg=171458.00, stdev=2129.26
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.14%
lat (usec) : 750=94.29%, 1000=2.69%
lat (msec) : 2=2.74%, 4=0.09%, 10=0.03%
cpu : usr=10.55%, sys=87.38%, ctx=373, majf=0, minf=8
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32
bgwriter: (groupid=0, jobs=1): err= 0: pid=11608: Thu Jan 29 19:45:06 2015
write: io=409600KB, bw=174150KB/s, iops=43537, runt= 2352msec
slat (usec): min=9, max=5199, avg=20.79, stdev=26.32
clat (usec): min=2, max=5915, avg=712.94, stdev=149.06
lat (usec): min=20, max=5936, avg=733.91, stdev=151.50
clat percentiles (usec):
| 1.00th=[ 628], 5.00th=[ 668], 10.00th=[ 684], 20.00th=[ 692],
| 30.00th=[ 700], 40.00th=[ 708], 50.00th=[ 708], 60.00th=[ 716],
| 70.00th=[ 716], 80.00th=[ 724], 90.00th=[ 732], 95.00th=[ 740],
| 99.00th=[ 780], 99.50th=[ 836], 99.90th=[ 2512], 99.95th=[ 5536],
| 99.99th=[ 5920]
bw (KB /s): min=170872, max=174024, per=25.68%, avg=172714.00, stdev=1386.45
lat (usec) : 4=0.01%, 50=0.01%, 100=0.01%, 250=0.01%, 500=0.10%
lat (usec) : 750=97.10%, 1000=2.55%
lat (msec) : 2=0.08%, 4=0.07%, 10=0.09%
cpu : usr=10.50%, sys=88.56%, ctx=260, majf=0, minf=6
IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=100.0%, >=64=0.0%
submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0%
complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.1%, 64=0.0%, >=64=0.0%
issued : total=r=0/w=102400/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
latency : target=0, window=0, percentile=100.00%, depth=32Run status group 0 (all jobs):
WRITE: io=1600.0MB, aggrb=672577KB/s, minb=168144KB/s, maxb=174149KB/s, mint=2352msec, maxt=2436msec
Disk stats (read/write):
zram0: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00%
-ss
[global]
size=400m
directory=/mnt/
ioengine=libaio
iodepth=32
direct=1
invalidate=1
numjobs=4
loops=4
[bgreader]
rw=randread
[global]
size=400m
directory=/mnt/
ioengine=libaio
iodepth=32
direct=1
invalidate=1
numjobs=4
loops=5
[bgupdater]
rw=randrw
[global]
size=400m
directory=/mnt/
ioengine=libaio
iodepth=32
direct=1
invalidate=1
numjobs=4
loops=5
[bgwriter]
rw=randwrite
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
In reply to Minchan KimHello, Minchan:
2015-01-28 16:15 GMT+08:00 Minchan Kim <[email protected]>:
Quoted 3 lines
Admin could reset zram during I/O operation going on so we have used
zram->init_lockas read-side lock in I/O path to prevent sudden zram meta freeing.
When I/O operation is running, that means the /dev/zram0 is mounted or swaped on. Then the device could not be reset by below code:
/* Do not reset an active device! */
if (bdev->bd_holders) {
ret = -EBUSY;
goto out;
}So the zram->init_lock in I/O path is to check whether the device has been initialized(echo xxx > /sys/block/zram/disk_size).
Is that right?
Quoted 3 lines
However, the
init_lockis really troublesome.
We can't do callzram_meta_allocunderinit_lockdue to lockdep splat
I do not know much about the lockdep.
enabled the CONFIG_LOCKDEP, But from the test, there is no warning printed. Could you please tell me how this happened?
Thanks.
Quoted 89 lines
because
zram_rw_pageis one of the function under reclaim path and hold it asread_lockwhile other places in process context hold it aswrite_lock. So, we have used allocation out of the lock to avoid lockdep warn but it's not good for readability and fainally, I met another lockdep splat betweeninit_lockandcpu_hotpulugfromkmem_cache_destroyduring wokring zsmalloc compaction. :(Yes, the ideal is to remove horrible
init_lockof zram in rw path. This patch removes it in rw path and instead, putinit_donebool variable to check initialization done with smp_[wmb|rmb] and srcu_[un]read_lock to prevent sudden zram meta freeing during I/O operation.Signed-off-by Minchan Kim <[email protected]>--- drivers/block/zram/zram_drv.c | 85 +++++++++++++++++++++++++++++++------------ drivers/block/zram/zram_drv.h | 5 +++ 2 files changed, 66 insertions(+), 24 deletions(-)drivers/block/zram/zram_drv.c +29
-5@@ -32,6 +32,7 @@#include <linux/string.h> #include <linux/vmalloc.h> #include <linux/err.h> +#include <linux/srcu.h> #include "zram_drv.h" @@ -53,9 +54,31 @@ static ssize_t name##_show(struct device *d, \ } \static DEVICE_ATTR_RO (name); -static inline int init_done (struct zram *zram) +static inline bool init_done (struct zram *zram) { -return zram->meta !=NULL ; +/* +* init_done can be used without holding zram->init_lock in +* read/write handler(ie, zram_make_request) but we should make sure +* that zram->init_done should set up after meta initialization is +* done. Look at setup_init_done. +*/ +bool ret = zram->init_done; + +if (ret) +smp_rmb ();/* pair with setup_init_done */ + +return ret; +} + +static inline void setup_init_done (struct zram *zram,bool flag) +{ +/* +* Store operation of struct zram fields should complete +* before zram->init_done set up because zram_bvec_rw +* doesn't hold an zram->init_lock. +*/ +smp_wmb (); + zram->init_done = flag; }static inline struct zram *dev_to_zram (struct device *dev)@@ -326,6 +349,10 @@ static void zram_meta_free(struct zram_meta *meta)kfree (meta); } +static void rcu_zram_do_nothing (struct rcu_head *unused) +{ +} +static struct zram_meta *zram_meta_alloc (int device_id,u64 disksize) {char pool_name[8 ];@@ -726,11 +753,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } -zcomp_destroy (zram->comp); zram->max_comp_streams =1 ; -zram_meta_free (zram->meta); - zram->meta =NULL ;/* Reset stats */ memset (&zram->stats,0 ,sizeof (zram->stats));
drivers/block/zram/zram_drv.c static void zram_reset_device(struct zram *zram, bool reset_capacity) · line 763
Quoted 2 lines
drivers/block/zram/zram_drv.c +0
-0@@ -738,8 +762,12 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)if (reset_capacity)set_capacity (zram->disk,0 );
Should this be after synchronize_srcu()?
Quoted 163 lines
drivers/block/zram/zram_drv.c +32
-19+setup_init_done (zram, false); +call_srcu (&zram->srcu, &zram->rcu, rcu_zram_do_nothing); +synchronize_srcu (&zram->srcu); +zram_meta_free (zram->meta); +zcomp_destroy (zram->comp);up_write (&zram->init_lock); -/* * Revalidate disk out of the init_lock to avoid lockdep splat. * It's okay because disk's capacity is protected by init_lock @@ -762,10 +790,19 @@ static ssize_t disksize_store(struct device *dev,if (!disksize)return -EINVAL ; +down_write (&zram->init_lock); +if (init_done (zram)) { +pr_info ("Cannot change disksize for initialized device\n" ); +up_write (&zram->init_lock); +return -EBUSY ; + } + disksize =PAGE_ALIGN (disksize); meta =zram_meta_alloc (zram->disk->first_minor, disksize); -if (!meta) +if (!meta) { +up_write (&zram->init_lock);return -ENOMEM ; + } comp =zcomp_create (zram->compressor, zram->max_comp_streams);if (IS_ERR (comp)) {@@ -775,17 +812,11 @@ static ssize_t disksize_store(struct device *dev, goto out_free_meta; } -down_write (&zram->init_lock); -if (init_done (zram)) { -pr_info ("Cannot change disksize for initialized device\n" ); - err = -EBUSY ; - goto out_destroy_comp; - } - zram->meta = meta; zram->comp = comp; zram->disksize = disksize;set_capacity (zram->disk, zram->disksize >>SECTOR_SHIFT ); +setup_init_done (zram, true);up_write (&zram->init_lock);/* @@ -797,10 +828,8 @@ static ssize_t disksize_store(struct device *dev,return len; -out_destroy_comp: -up_write (&zram->init_lock); -zcomp_destroy (comp); out_free_meta: +up_write (&zram->init_lock);zram_meta_free (meta);return err; }@@ -905,9 +934,10 @@ out:*/ static void zram_make_request (struct request_queue *queue,struct bio *bio) { +int idx;struct zram *zram = queue->queuedata; -down_read (&zram->init_lock); + idx =srcu_read_lock (&zram->srcu);if (unlikely (!init_done (zram))) goto error;@@ -918,12 +948,12 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) }__zram_make_request (zram, bio); -up_read (&zram->init_lock); +srcu_read_unlock (&zram->srcu, idx);return ; error: -up_read (&zram->init_lock); +srcu_read_unlock (&zram->srcu, idx);bio_io_error (bio); }@@ -945,18 +975,20 @@ static void zram_slot_free_notify(struct block_device *bdev,static int zram_rw_page (struct block_device *bdev,sector_t sector,struct page *page,int rw) { -int offset, err; +int offset, err, idx;u32 index;struct zram *zram;struct bio_vec bv; zram = bdev->bd_disk->private_data; + idx =srcu_read_lock (&zram->srcu); +if (!valid_io_request (zram, sector,PAGE_SIZE )) {atomic64_inc (&zram->stats.invalid_io); +srcu_read_unlock (&zram->srcu, idx);return -EINVAL ; } -down_read (&zram->init_lock);if (unlikely (!init_done (zram))) { err = -EIO ; goto out_unlock;@@ -971,7 +1003,7 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, err =zram_bvec_rw (zram, &bv, index, offset, rw); out_unlock: -up_read (&zram->init_lock); +srcu_read_unlock (&zram->srcu, idx);/* * If I/O fails, just return error(ie, non-zero) without * calling page_endio. @@ -1041,6 +1073,11 @@ static int create_device(struct zram *zram, int device_id)init_rwsem (&zram->init_lock); +if (init_srcu_struct (&zram->srcu)) { +pr_err ("Error initialize srcu for device %d\n" , device_id); + goto out; + } + zram->queue =blk_alloc_queue (GFP_KERNEL );if (!zram->queue) { pr_err("Error allocating disk queue for device %d\n",@@ -1125,8 +1162,8 @@ static void destroy_device(struct zram *zram)del_gendisk (zram->disk);put_disk (zram->disk); -blk_cleanup_queue (zram->queue); +cleanup_srcu_struct (&zram->srcu); }static int __init zram_init (void )drivers/block/zram/zram_drv.h +5
-1@@ -105,8 +105,13 @@ struct zram {struct gendisk *disk;struct zcomp *comp; +struct srcu_struct srcu; +struct rcu_head rcu; +/* Prevent concurrent execution of device init, reset and R/W request */ struct rw_semaphore init_lock; +bool init_done; +/* * This is the limit on amount of *uncompressed* worth of data * we can store in a disk. --
Quoted 2 lines
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 8 lines · Ganesh Mahendran
Quoted 3 lines
Admin could reset zram during I/O operation going on so we have used
zram->init_lockas read-side lock in I/O path to prevent sudden zram meta freeing.When I/O operation is running, that means the
/dev/zram0is mounted or swaped on. Then the device could not be reset by below code:
Quoted 9 lines
/* Do not reset an active device! */ if (bdev->bd_holders) { ret = -EBUSY; goto out; }So the
zram->init_lockin I/O path is to check whether the device has beeninitialized(echo xxx > /sys/block/zram/disk_size).
for mounted device (w/fs), we see initial (well, it goes up and down many times while we create device, but this is not interesting here) ->bd_holders increment in:
vfs_kern_mount -> mount_bdev -> blkdev_get_by_path -> blkdev_get
and it goes to zero in:
cleanup_mnt -> deactivate_super -> kill_block_super -> blkdev_put
after umount we still have init device. so, *theoretically*, we can see something like
CPU0 CPU1
umountreset_storebdev->bd_holders == 0 mount
... zram_make_request()zram_reset_device()
w/o zram->init_lock in both zram_reset_device() and zram_make_request() one of CPUs will be a bit sad.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Hello Sergey
2015-01-29 23:12 GMT+08:00 Sergey Senozhatsky <[email protected]>:
Quoted 15 lines · Ganesh Mahendran
Quoted 14 lines
Quoted 3 lines
Admin could reset zram during I/O operation going on so we have used
zram->init_lockas read-side lock in I/O path to prevent sudden zram meta freeing.When I/O operation is running, that means the
/dev/zram0is mounted or swaped on. Then the device could not be reset by below code:/* Do not reset an active device! */ if (bdev->bd_holders) { ret = -EBUSY; goto out; }
Quoted 3 lines
Quoted 3 lines
So the
zram->init_lockin I/O path is to check whether the device has beeninitialized(echo xxx > /sys/block/zram/disk_size).
Thanks for your explanation.
Quoted 2 lines
for mounted device (w/fs), we see initial (well, it goes up and down
What does "w/" mean?
Quoted 11 lines
many times while we create device, but this is not interesting here) ->bd_holders increment in:
vfs_kern_mount->mount_bdev->blkdev_get_by_path->blkdev_getand it goes to zero in:
cleanup_mnt->deactivate_super->kill_block_super->blkdev_putafter umount we still have init device. so, *theoretically*, we can see something like
Quoted 6 lines
CPU0 CPU1
umountreset_storebdev->bd_holders== 0 mount
...zram_make_request()zram_reset_device()
In this example, the data stored in zram will be corrupted. Since CPU0 will free meta while CPU1 is using. right?
Quoted 3 lines
w/o
zram->init_lockin bothzram_reset_device()andzram_make_request()one of CPUs will be a bit sad.
what does "w/o" mean?
Thanks
Quoted 2 lines
-ss--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 14 lines · Ganesh Mahendran
Quoted 10 lines
Quoted 10 lines
When I/O operation is running, that means the
/dev/zram0is mounted or swaped on. Then the device could not be reset by below code:/* Do not reset an active device! */ if (bdev->bd_holders) { ret = -EBUSY; goto out; }Quoted 3 lines
Quoted 3 lines
So the
zram->init_lockin I/O path is to check whether the device has beeninitialized(echo xxx > /sys/block/zram/disk_size).
Quoted 6 lines
Thanks for your explanation.
Quoted 2 lines
for mounted device (w/fs), we see initial (well, it goes up and down
What does "w/" mean?
'with fs'
Quoted 18 lines
Quoted 11 lines
many times while we create device, but this is not interesting here) ->bd_holders increment in:
vfs_kern_mount->mount_bdev->blkdev_get_by_path->blkdev_getand it goes to zero in:
cleanup_mnt->deactivate_super->kill_block_super->blkdev_putafter umount we still have init device. so, *theoretically*, we can see something like
Quoted 6 lines
CPU0 CPU1
umountreset_storebdev->bd_holders== 0 mount
...zram_make_request()zram_reset_device()
Quoted 4 lines
In this example, the data stored in zram will be corrupted. Since CPU0 will free meta while CPU1 is using. right?
with out ->init_lock protection in this case we have 'free' vs. 'use' race.
Quoted 5 lines
Quoted 4 lines
w/o
zram->init_lockin bothzram_reset_device()andzram_make_request()one of CPUs will be a bit sad.what does "w/o" mean?
'with out'
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
2015-01-30 16:08 GMT+08:00 Sergey Senozhatsky
<[email protected]>:
Quoted 42 lines · Ganesh Mahendran
Quoted 14 lines
Quoted 10 lines
Quoted 10 lines
When I/O operation is running, that means the
/dev/zram0is mounted or swaped on. Then the device could not be reset by below code:/* Do not reset an active device! */ if (bdev->bd_holders) { ret = -EBUSY; goto out; }Quoted 3 lines
Quoted 3 lines
So the
zram->init_lockin I/O path is to check whether the device has beeninitialized(echo xxx > /sys/block/zram/disk_size).Quoted 6 lines
Thanks for your explanation.
Quoted 2 lines
for mounted device (w/fs), we see initial (well, it goes up and down
What does "w/" mean?
'with fs'
Quoted 18 lines
Quoted 17 lines
many times while we create device, but this is not interesting here) ->bd_holders increment in:
vfs_kern_mount->mount_bdev->blkdev_get_by_path->blkdev_getand it goes to zero in:
cleanup_mnt->deactivate_super->kill_block_super->blkdev_putafter umount we still have init device. so, *theoretically*, we can see something like
CPU0 CPU1
umountreset_storebdev->bd_holders== 0 mount
...zram_make_request()zram_reset_device()
Quoted 6 lines
Quoted 4 lines
In this example, the data stored in zram will be corrupted. Since CPU0 will free meta while CPU1 is using. right?
with out ->init_lock protection in this case we have 'free' vs. 'use' race.
Maybe I did not explain clearly. I send a patch about this issue:
https://patchwork.kernel.org/patch/5754041/
Thanks
Quoted 11 lines
Quoted 6 lines
Quoted 5 lines
w/o
zram->init_lockin bothzram_reset_device()andzram_make_request()one of CPUs will be a bit sad.what does "w/o" mean?
'with out'
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Quoted 9 lines · Ganesh Mahendran
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
after umount we still have init device. so, *theoretically*, we can see something like
CPU0 CPU1
umountreset_storebdev->bd_holders== 0 mount
...zram_make_request()zram_reset_device()
[..]
Quoted 4 lines
Maybe I did not explain clearly. I send a patch about this issue:
excuse me? explain to me clearly what? my finding and my analysis?
this is the second time in a week that you hijack someone's work and you don't even bother to give any credit to people.
Minchan moved zram_meta_free(meta) out of init_lock here
https://lkml.org/lkml/2015/1/21/29
I proposed to also move zs_free() of meta->handles here
https://lkml.org/lkml/2015/1/21/384
... so what happened then -- you jumped in and sent a patch.
https://lkml.org/lkml/2015/1/24/50
Minchan sent you a hint https://lkml.org/lkml/2015/1/26/471
Quoted 1 line
but it seems the patch is based on my recent work "zram: free meta out of
init_lock".
"the patch is based on my work"!
now, for the last few days we were discussing init_lock and I first expressed my concerns and spoke about 'free' vs. 'use' problem here (but still didn't have enough spare to submit, besides we are in the middle of reset/init/write rework)
https://lkml.org/lkml/2015/1/27/1029
Quoted 13 lines
bdev->bd_holdersprotects from resetting device which has read/write operation ongoing on the onther CPU.I need to refresh on how ->bd_holders actually incremented/decremented. can the following race condition take a place?
CPU0 CPU1
reset_store()bdev->bd_holders== falsezram_make_request -rm- down_read(&zram->init_lock); init_done(zram) == true
Quoted 10 lines
zram_reset_device()valid_io_request()
__zram_make_requestdown_write(&zram->init_lock);zram_bvec_rw
[..]set_capacity(zram->disk, 0);zram->init_done= false;kick_all_cpus_sync();zram_bvec_writeorzram_bvec_read()zram_meta_free(zram->meta);zcomp_destroy(zram->comp);zcomp_compress()orzcomp_decompress()
and later here https://lkml.org/lkml/2015/1/29/645
Quoted 5 lines
after umount we still have init device. so, *theoretically*, we can see something like
Quoted 7 lines
CPU0 CPU1
umountreset_storebdev->bd_holders== 0 mount
...zram_make_request()zram_reset_device()
so what happened next? your patch happened next.
with quite familiar problem description
Quoted 6 lines
CPU0 CPU1
t1: bdput t2: mount /dev/zram0 /mnt t3: zram_reset_device
and now you say that I don't understant something in "your analysis"?
stop doing this. this is not how it works.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v1 2/2] zram: remove init_lock in zram_make_request
Hello, Sergey
2015-01-31 19:07 GMT+08:00 Sergey Senozhatsky <[email protected]>:
Quoted 14 lines · Ganesh Mahendran
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
after umount we still have init device. so, *theoretically*, we can see something like
CPU0 CPU1
umountreset_storebdev->bd_holders== 0 mount
...zram_make_request()zram_reset_device()[..]
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
Maybe I did not explain clearly. I send a patch about this issue:
excuse me? explain to me clearly what? my finding and my analysis?
Sorry, I missed this mail
https://lkml.org/lkml/2015/1/27/1029
That's why I ask questions in this
https://lkml.org/lkml/2015/1/29/580
after Minchan's description.
Quoted 11 lines
this is the second time in a week that you hijack someone's work and you don't even bother to give any credit to people.
Minchan moved
zram_meta_free(meta)out ofinit_lockhere
https://lkml.org/lkml/2015/1/21/29I proposed to also move
zs_free()ofmeta->handleshere
https://lkml.org/lkml/2015/1/21/384
I thought you wanted move the code block after
up_write(&zram->init_lock);
And I found the code block can be even encapsulated in zram_meta_free().
That's why I sent:
https://lkml.org/lkml/2015/1/24/50
Quoted 16 lines
... so what happened then -- you jumped in and sent a patch.
https://lkml.org/lkml/2015/1/24/50Minchan sent you a hint https://lkml.org/lkml/2015/1/26/471
Quoted 1 line
but it seems the patch is based on my recent work "zram: free meta out of
init_lock"."the patch is based on my work"!
Quoted 29 lines
now, for the last few days we were discussing
init_lockand I first expressed my concerns and spoke about 'free' vs. 'use' problem here (but still didn't have enough spare to submit, besides we are in the middle of reset/init/write rework)https://lkml.org/lkml/2015/1/27/1029
Quoted 22 lines
bdev->bd_holdersprotects from resetting device which has read/write operation ongoing on the onther CPU.I need to refresh on how ->bd_holders actually incremented/decremented. can the following race condition take a place?
CPU0 CPU1
reset_store()bdev->bd_holders== falsezram_make_request -rm- down_read(&zram->init_lock); init_done(zram) == true
zram_reset_device()valid_io_request()
__zram_make_requestdown_write(&zram->init_lock);zram_bvec_rw
[..]set_capacity(zram->disk, 0);zram->init_done= false;kick_all_cpus_sync();zram_bvec_writeorzram_bvec_read()zram_meta_free(zram->meta);zcomp_destroy(zram->comp);zcomp_compress()orzcomp_decompress()
Sorry, I did not check this mail.
Quoted 30 lines
Quoted 1 line
and later here https://lkml.org/lkml/2015/1/29/645
Quoted 5 lines
after umount we still have init device. so, *theoretically*, we can see something like
Quoted 7 lines
CPU0 CPU1
umountreset_storebdev->bd_holders== 0 mount
...zram_make_request()zram_reset_device()so what happened next? your patch happened next.
with quite familiar problem descriptionQuoted 6 lines
CPU0 CPU1
t1: bdput t2: mount /dev/zram0 /mnt t3: zram_reset_device
Quoted 9 lines
and now you say that I don't understant something in "your analysis"?
stop doing this. this is not how it works.
-ss
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 16 lines · Minchan Kim
zram_meta_alloc()andzram_meta_free()are a pair.
Inzram_meta_alloc(), meta table is allocated. So it it better to free it inzram_meta_free().Signed-off-by Ganesh Mahendran <[email protected]>Signed-off-by Minchan Kim <[email protected]>--- drivers/block/zram/zram_drv.c | 30 ++++++++++++++---------------- drivers/block/zram/zram_drv.h | 1 + 2 files changed, 15 insertions(+), 16 deletions(-)drivers/block/zram/zram_drv.c +0
-0
drivers/block/zram/zram_drv.c static inline int valid_io_request(struct zram *zram, · line 312
Quoted 4 lines
drivers/block/zram/zram_drv.c +1
-0@@ -309,6 +309,18 @@ static inline int valid_io_request(struct zram *zram,static void zram_meta_free (struct zram_meta *meta) { +size_t index;
I don't like how we bloat structs w/o any need.
zram keeps ->disksize, so let's use `zram->disksize >> PAGE_SHIFT' instead of introducing ->num_pages.
-ss
Quoted 73 lines
drivers/block/zram/zram_drv.c +12
-16+/* Free all pages that are still in this zram device */ +for (index =0 ; index < meta->num_pages; index++) { +unsigned long handle = meta->table[index].handle; + +if (!handle) +continue ; + +zs_free (meta->mem_pool, handle); + } +zs_destroy_pool (meta->mem_pool);vfree (meta->table);kfree (meta);@@ -316,15 +328,14 @@ static void zram_meta_free(struct zram_meta *meta)static struct zram_meta *zram_meta_alloc (int device_id,u64 disksize) { -size_t num_pages;char pool_name[8 ];struct zram_meta *meta =kmalloc (sizeof (*meta),GFP_KERNEL );if (!meta)return NULL ; - num_pages = disksize >>PAGE_SHIFT ; - meta->table =vzalloc (num_pages *sizeof (*meta->table)); + meta->num_pages = disksize >>PAGE_SHIFT ; + meta->table =vzalloc (meta->num_pages *sizeof (*meta->table));if (!meta->table) {pr_err ("Error allocating zram address table\n" ); goto out_error;@@ -706,9 +717,6 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { -size_t index; -struct zram_meta *meta; -down_write (&zram->init_lock); zram->limit_pages =0 ;@@ -718,16 +726,6 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } - meta = zram->meta; -/* 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; -if (!handle) -continue ; - -zs_free (meta->mem_pool, handle); - } -zcomp_destroy (zram->comp); zram->max_comp_streams =1 ;drivers/block/zram/zram_drv.h +1
-0@@ -96,6 +96,7 @@ struct zram_stats {struct zram_meta {struct zram_table_entry *table;struct zs_pool *mem_pool; +size_t num_pages; };struct zram {Signature · 1 line
1.9.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Minchan Kim
maintainerQuoted 18 lines · Sergey Senozhatsky
Quoted 17 lines · Minchan Kim
zram_meta_alloc()andzram_meta_free()are a pair.
Inzram_meta_alloc(), meta table is allocated. So it it better to free it inzram_meta_free().Signed-off-by Ganesh Mahendran <[email protected]>Signed-off-by Minchan Kim <[email protected]>--- drivers/block/zram/zram_drv.c | 30 ++++++++++++++---------------- drivers/block/zram/zram_drv.h | 1 + 2 files changed, 15 insertions(+), 16 deletions(-)drivers/block/zram/zram_drv.c +0
-0@@ -309,6 +309,18 @@ static inline int valid_io_request(struct zram *zram,
Quoted 8 lines
drivers/block/zram/zram_drv.c static inline int valid_io_request(struct zram *zram, · line 312
Quoted 3 lines
drivers/block/zram/zram_drv.c +1
-0static void zram_meta_free (struct zram_meta *meta) { +size_t index;I don't like how we bloat structs w/o any need.
zram keeps ->disksize, so let's use `zram->disksize >>PAGE_SHIFT' instead of introducing ->num_pages.
Right. I overlooked it. I just want to send my patch[2/2] and I thought it would be better ganesh's patch to merge first although it's orthogonal.
Ganesh, I hope you resend this patch with Sergey's suggestion. If you are busy, please tell me. I will do it instead of you.
Quoted 76 lines
-ssQuoted 73 lines
drivers/block/zram/zram_drv.c +12
-16+/* Free all pages that are still in this zram device */ +for (index =0 ; index < meta->num_pages; index++) { +unsigned long handle = meta->table[index].handle; + +if (!handle) +continue ; + +zs_free (meta->mem_pool, handle); + } +zs_destroy_pool (meta->mem_pool);vfree (meta->table);kfree (meta);@@ -316,15 +328,14 @@ static void zram_meta_free(struct zram_meta *meta)static struct zram_meta *zram_meta_alloc (int device_id,u64 disksize) { -size_t num_pages;char pool_name[8 ];struct zram_meta *meta =kmalloc (sizeof (*meta),GFP_KERNEL );if (!meta)return NULL ; - num_pages = disksize >>PAGE_SHIFT ; - meta->table =vzalloc (num_pages *sizeof (*meta->table)); + meta->num_pages = disksize >>PAGE_SHIFT ; + meta->table =vzalloc (meta->num_pages *sizeof (*meta->table));if (!meta->table) {pr_err ("Error allocating zram address table\n" ); goto out_error;@@ -706,9 +717,6 @@ static void zram_bio_discard(struct zram *zram, u32 index,static void zram_reset_device (struct zram *zram,bool reset_capacity) { -size_t index; -struct zram_meta *meta; -down_write (&zram->init_lock); zram->limit_pages =0 ;@@ -718,16 +726,6 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)return ; } - meta = zram->meta; -/* 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; -if (!handle) -continue ; - -zs_free (meta->mem_pool, handle); - } -zcomp_destroy (zram->comp); zram->max_comp_streams =1 ;drivers/block/zram/zram_drv.h +1
-0@@ -96,6 +96,7 @@ struct zram_stats {struct zram_meta {struct zram_table_entry *table;struct zs_pool *mem_pool; +size_t num_pages; };struct zram {Signature · 1 line
1.9.1
Signature · 8 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Hello, Minchan
2015-01-29 7:17 GMT+08:00 Minchan Kim <[email protected]>:
Quoted 28 lines · Sergey Senozhatsky
Quoted 18 lines · Minchan Kim
Quoted 17 lines
zram_meta_alloc()andzram_meta_free()are a pair.
Inzram_meta_alloc(), meta table is allocated. So it it better to free it inzram_meta_free().Signed-off-by Ganesh Mahendran <[email protected]>Signed-off-by Minchan Kim <[email protected]>--- drivers/block/zram/zram_drv.c | 30 ++++++++++++++---------------- drivers/block/zram/zram_drv.h | 1 + 2 files changed, 15 insertions(+), 16 deletions(-)drivers/block/zram/zram_drv.c +0
-0@@ -309,6 +309,18 @@ static inline int valid_io_request(struct zram *zram,Quoted 8 lines
drivers/block/zram/zram_drv.c static inline int valid_io_request(struct zram *zram, · line 312
Quoted 3 lines
drivers/block/zram/zram_drv.c +1
-0static void zram_meta_free (struct zram_meta *meta) { +size_t index;I don't like how we bloat structs w/o any need.
zram keeps ->disksize, so let's use `zram->disksize >>PAGE_SHIFT' instead of introducing ->num_pages.
Quoted 5 lines
Right. I overlooked it. I just want to send my patch[2/2] and I thought it would be better ganesh's patch to merge first although it's orthogonal.
Ganesh, I hope you resend this patch with Sergey's suggestion. If you are busy, please tell me. I will do it instead of you.
OK, I will do it today.
Thanks
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>