Re: [PATCH] zram: idle writeback fixes and cleanup
Hi Sergey,
On Thu, Dec 27, 2018 at 11:26:24AM +0900, Sergey Senozhatsky wrote:
> On (12/24/18 12:35), Minchan Kim wrote:
> [..]
> > @@ -645,10 +680,13 @@ static ssize_t writeback_store(struct device *dev,
> > bvec.bv_len = PAGE_SIZE;
> > bvec.bv_offset = 0;
> >
> > - if (zram->stop_writeback) {
> > + spin_lock(>wb_limit_lock);
> > + if (zram->wb_limit_enable && !zram->bd_wb_limit) {
> > + spin_unlock(>wb_limit_lock);
> > ret = -EIO;
> > break;
> > }
> > + spin_unlock(>wb_limit_lock);
> [..]
> > @@ -732,11 +771,10 @@ static ssize_t writeback_store(struct device *dev,
> > zram_set_element(zram, index, blk_idx);
> > blk_idx = 0;
> > atomic64_inc(>stats.pages_stored);
> > - if (atomic64_add_unless(>stats.bd_wb_limit,
> > - -1 << (PAGE_SHIFT - 12), 0)) {
> > - if (atomic64_read(>stats.bd_wb_limit) == 0)
> > - zram->stop_writeback = true;
> > - }
> > + spin_lock(>wb_limit_lock);
> > + if (zram->wb_limit_enable && zram->bd_wb_limit > 0)
> > + zram->bd_wb_limit -= 1UL << (PAGE_SHIFT - 12);
> > + spin_unlock(>wb_limit_lock);
>
> Do we really need ->wb_limit_lock spinlock? We kinda punch it twice
> in this loop. If someone clears ->wb_limit_enable somewhere in between
> then the worst thing to happen is that we will just write extra page
> to the backing device; not a very big deal to me. Am I missing
> something?
Without the lock, bd_wb_limit store/read would be racy.
CPU A CPU B
if (zram->wb_limit_enable && zram->bd_wb_limit > 0)
zram->bd_wb_limit = 0
zram->bd_wb_limit -= 1UL << (PAGE_SHIFT - 12)
It makes limit feature void.
>
> -ss
Re: [PATCH] zram: idle writeback fixes and cleanup
On (12/24/18 12:35), Minchan Kim wrote:
[..]
> @@ -645,10 +680,13 @@ static ssize_t writeback_store(struct device *dev,
> bvec.bv_len = PAGE_SIZE;
> bvec.bv_offset = 0;
>
> - if (zram->stop_writeback) {
> + spin_lock(>wb_limit_lock);
> + if (zram->wb_limit_enable && !zram->bd_wb_limit) {
> + spin_unlock(>wb_limit_lock);
> ret = -EIO;
> break;
> }
> + spin_unlock(>wb_limit_lock);
[..]
> @@ -732,11 +771,10 @@ static ssize_t writeback_store(struct device *dev,
> zram_set_element(zram, index, blk_idx);
> blk_idx = 0;
> atomic64_inc(>stats.pages_stored);
> - if (atomic64_add_unless(>stats.bd_wb_limit,
> - -1 << (PAGE_SHIFT - 12), 0)) {
> - if (atomic64_read(>stats.bd_wb_limit) == 0)
> - zram->stop_writeback = true;
> - }
> + spin_lock(>wb_limit_lock);
> + if (zram->wb_limit_enable && zram->bd_wb_limit > 0)
> + zram->bd_wb_limit -= 1UL << (PAGE_SHIFT - 12);
> + spin_unlock(>wb_limit_lock);
Do we really need ->wb_limit_lock spinlock? We kinda punch it twice
in this loop. If someone clears ->wb_limit_enable somewhere in between
then the worst thing to happen is that we will just write extra page
to the backing device; not a very big deal to me. Am I missing
something?
-ss
[PATCH] zram: idle writeback fixes and cleanup
This patch includes some fixes and cleanup for idle-page writeback. 1. writeback_limit interface Now writeback_limit interface is rather conusing. For example, once writeback limit budget is exausted, admin can see 0 from /sys/block/zramX/writeback_limit which is same semantic with disable writeback_limit at this moment. IOW, admin cannot tell that zero came from disable writeback limit or exausted writeback limit. To make the interface clear, let's sepatate enable of writeback limit to another knob - /sys/block/zram0/writeback_limit_enable * before: while true : # to re-enable writeback limit once previous one is used up echo 0 > /sys/block/zram0/writeback_limit echo $((200<<20)) > /sys/block/zram0/writeback_limit .. .. # used up the writeback limit budget * new # To enable writeback limit, from the beginning, admin should # enable it. echo $((200<<20)) > /sys/block/zram0/writeback_limit echo 1 > /sys/block/zram/0/writeback_limit_enable while true : echo $((200<<20)) > /sys/block/zram0/writeback_limit .. .. # used up the writeback limit budget It's much strightforward. 2. fix condition check idle/huge writeback mode check The mode in writeback_store is not bit opeartion any more so no need to use bit operations. Furthermore, current condition check is broken in that it does writeback every pages regardless of huge/idle. 3. clean up idle_store No need to use goto. Suggested-by: John Dias Signed-off-by: Minchan Kim --- Documentation/ABI/testing/sysfs-block-zram | 11 ++- Documentation/blockdev/zram.txt| 74 --- drivers/block/zram/zram_drv.c | 86 -- drivers/block/zram/zram_drv.h | 5 +- 4 files changed, 122 insertions(+), 54 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram index 9d2339a485c8a..14b2bf2e5105c 100644 --- a/Documentation/ABI/testing/sysfs-block-zram +++ b/Documentation/ABI/testing/sysfs-block-zram @@ -122,11 +122,18 @@ Contact: Minchan Kim statistics (bd_count, bd_reads, bd_writes) in a format similar to block layer statistics file format. +What: /sys/block/zram/writeback_limit_enable +Date: November 2018 +Contact: Minchan Kim +Description: + The writeback_limit_enable file is read-write and specifies + eanbe of writeback_limit feature. "1" means eable the feature. + No limit "0" is the initial state. + What: /sys/block/zram/writeback_limit Date: November 2018 Contact: Minchan Kim Description: The writeback_limit file is read-write and specifies the maximum amount of writeback ZRAM can do. The limit could be changed - in run time and "0" means disable the limit. - No limit is the initial state. + in run time. diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt index 436c5e98e1b60..4df0ce2710857 100644 --- a/Documentation/blockdev/zram.txt +++ b/Documentation/blockdev/zram.txt @@ -156,22 +156,23 @@ Per-device statistics are exported as various nodes under /sys/block/zram/ A brief description of exported device attributes. For more details please read Documentation/ABI/testing/sysfs-block-zram. -Nameaccessdescription ------ -disksize RWshow and set the device's disk size -initstate ROshows the initialization state of the device -reset WOtrigger device reset -mem_used_max WOreset the `mem_used_max' counter (see later) -mem_limit WOspecifies the maximum amount of memory ZRAM can use -to store the compressed data -writeback_limit WOspecifies the maximum amount of write IO zram can - write out to backing device as 4KB unit -max_comp_streams RWthe number of possible concurrent compress operations -comp_algorithmRWshow and change the compression algorithm -compact WOtrigger memory compaction -debug_statROthis file is used for zram debugging purposes -backing_dev RWset up backend storage for zram to write out -idle WOmark allocated slot as idle +Name accessdescription + ----- +disksize RW show and set the device's disk size +initstate RO shows the initialization state of the device +reset WO trigger device reset +mem_used_max WO reset the `mem_used_max' counter (see later) +mem_limit WO specifies the maximum amount of memory ZRAM can use + to store the compressed data +writeback_limitWO specifies the maximum amount of write IO
