zram: idle writeback fixes and cleanup

PATCH In review Minchan Kim 2 participants 3 messages Dec 24, 2018 – Dec 28, 2018

Subsystems (3)

Documentation Block layer zram

lore

Share & tools

lore

Minchan Kim

author maintainer

2018-12-24 03:35 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 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(-)

Documentation/ABI/testing/sysfs-block-zram +9 -2

index 9d2339a485c8a..14b2bf2e5105c 100644
@@ -122,11 +122,18 @@ Contact:	Minchan Kim <[email protected]>
 		statistics (bd_count, bd_reads, bd_writes) in a format
 		similar to block layer statistics file format.
 
+What:		/sys/block/zram<id>/writeback_limit_enable
+Date:		November 2018
+Contact:	Minchan Kim <[email protected]>
+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<id>/writeback_limit
 Date:		November 2018
 Contact:	Minchan Kim <[email protected]>
 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.

Documentation/blockdev/zram.txt +47 ~~-27~~

101 lines not on this page · raw message

drivers/block/zram/zram_drv.c +63 ~~-23~~

161 lines not on this page · raw message

drivers/block/zram/zram_drv.h +3 -2

index 4bd3afd15e833..f2fd46daa7604 100644
@@ -86,7 +86,6 @@ struct zram_stats {
 	atomic64_t bd_count;		/* no. of pages in backing device */
 	atomic64_t bd_reads;		/* no. of reads from backing device */
 	atomic64_t bd_writes;		/* no. of writes from backing device */
-	atomic64_t bd_wb_limit;		/* writeback limit of backing device */
 #endif
 };

@@ -114,8 +113,10 @@ struct zram {
 	 */
 	bool claim; /* Protected by bdev->bd_mutex */
 	struct file *backing_dev;
-	bool stop_writeback;
 #ifdef CONFIG_ZRAM_WRITEBACK
+	spinlock_t wb_limit_lock;
+	bool wb_limit_enable;
+	u64 bd_wb_limit;
 	struct block_device *bdev;
 	unsigned int old_block_size;
 	unsigned long *bitmap;

Signature · 1 line

2.20.1.415.g653613c723-goog

Sergey Senozhatsky

2018-12-27 02:28 UTC #1 lore raw

On (12/24/18 12:35), Minchan Kim wrote:
[..]

Quoted 12 lines

drivers/block/zram/zram_drv.c +4 -1

@@ -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(&zram->wb_limit_lock);
+		if (zram->wb_limit_enable && !zram->bd_wb_limit) {
+			spin_unlock(&zram->wb_limit_lock);
 			ret = -EIO;
 			break;
 		}
+		spin_unlock(&zram->wb_limit_lock);

[..]

drivers/block/zram/zram_drv.c static ssize_t writeback_store(struct device *dev, · line 777

Quoted 13 lines

drivers/block/zram/zram_drv.c +4 -5

@@ -732,11 +771,10 @@ static ssize_t writeback_store(struct device *dev,
 		zram_set_element(zram, index, blk_idx);
 		blk_idx = 0;
 		atomic64_inc(&zram->stats.pages_stored);
-		if (atomic64_add_unless(&zram->stats.bd_wb_limit,
-					-1 << (PAGE_SHIFT - 12), 0)) {
-			if (atomic64_read(&zram->stats.bd_wb_limit) == 0)
-				zram->stop_writeback = true;
-		}
+		spin_lock(&zram->wb_limit_lock);
+		if (zram->wb_limit_enable && zram->bd_wb_limit > 0)
+			zram->bd_wb_limit -=  1UL << (PAGE_SHIFT - 12);
+		spin_unlock(&zram->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

Minchan Kim

author maintainer

2018-12-28 05:19 UTC #2 lore raw

Hi Sergey,

Quoted 26 lines · Sergey Senozhatsky

On (12/24/18 12:35), Minchan Kim wrote:
[..]

Quoted 12 lines

drivers/block/zram/zram_drv.c +4 -1

@@ -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(&zram->wb_limit_lock);
+		if (zram->wb_limit_enable && !zram->bd_wb_limit) {
+			spin_unlock(&zram->wb_limit_lock);
 			ret = -EIO;
 			break;
 		}
+		spin_unlock(&zram->wb_limit_lock);

[..]

Quoted 11 lines

drivers/block/zram/zram_drv.c +2 -5

@@ -732,11 +771,10 @@ static ssize_t writeback_store(struct device *dev,
 		zram_set_element(zram, index, blk_idx);
 		blk_idx = 0;
 		atomic64_inc(&zram->stats.pages_stored);
-		if (atomic64_add_unless(&zram->stats.bd_wb_limit,
-					-1 << (PAGE_SHIFT - 12), 0)) {
-			if (atomic64_read(&zram->stats.bd_wb_limit) == 0)
-				zram->stop_writeback = true;
-		}
+		spin_lock(&zram->wb_limit_lock);
+		if (zram->wb_limit_enable && zram->bd_wb_limit > 0)

Quoted 8 lines

drivers/block/zram/zram_drv.c static ssize_t writeback_store(struct device *dev, · line 777
Quoted 2 lines
drivers/block/zram/zram_drv.c +2 -0
+			zram->bd_wb_limit -=  1UL << (PAGE_SHIFT - 12);
+		spin_unlock(&zram->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.

Quoted 2 lines

-ss