zram: rely on the bi_end_io for zram_rw_page fails

PATCH In review Minchan Kim 3 participants 6 messages Nov 14, 2014 – Nov 19, 2014

Subsystems (3)

Memory management Block layer zram

lore

Share & tools

lore

Minchan Kim

author maintainer

2014-11-14 00:49 UTC #0 lore raw

PATCH 1/1 zram: rely on the bi_end_io for zram_rw_page fails

When I tested zram, I found processes got segfaulted. The reason was zram_rw_page doesn't make the page dirty again when swap write failed, and even it doesn't return error by [1].

If error by zram internal happens, zram_rw_page should return non-zero without calling page_endio.
It causes resubmit the IO with bio so that it ends up calling bio->bi_end_io.

The reason is zram could be used for a block device for FS and swap, which they uses different bio complete callback, which works differently. So, we should rely on the bio I/O complete handler rather than zram_bvec_rw itself in case of I/O fail.

This patch fixes the segfault issue as well one [1]'s mentioned

[1] zram: make rw_page opeartion return 0

Cc Matthew Wilcox <[email protected]>

Cc Karam Lee <[email protected]>

Cc Dave Chinner <[email protected]>

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

---
 drivers/block/zram/zram_drv.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

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

index 4b4f4dbc3cfd..0e0650feab2a 100644
@@ -978,12 +978,10 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 out_unlock:
 	up_read(&zram->init_lock);
 out:
-	page_endio(page, rw, err);
+	if (unlikely(err))
+		return err;
 
-	/*
-	 * Return 0 prevents I/O fallback trial caused by rw_page fail
-	 * and upper layer can handle this IO error via page error.
-	 */
+	page_endio(page, rw, 0);
 	return 0;
 }

Signature · 7 lines

2.0.0

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

Sergey Senozhatsky

2014-11-15 09:19 UTC #1 lore raw

Hi,

Quoted 33 lines · Minchan Kim

When I tested zram, I found processes got segfaulted. The reason was zram_rw_page doesn't make the page dirty again when swap write failed, and even it doesn't return error by [1].
If error by zram internal happens, zram_rw_page should return non-zero without calling page_endio.
It causes resubmit the IO with bio so that it ends up calling bio->bi_end_io.
The reason is zram could be used for a block device for FS and swap, which they uses different bio complete callback, which works differently. So, we should rely on the bio I/O complete handler rather than zram_bvec_rw itself in case of I/O fail.
This patch fixes the segfault issue as well one [1]'s mentioned
[1] zram: make rw_page opeartion return 0
Cc Matthew Wilcox <[email protected]>
Cc Karam Lee <[email protected]>
Cc Dave Chinner <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/block/zram/zram_drv.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/block/zram/zram_drv.c +0 -0
index 4b4f4dbc3cfd..0e0650feab2a 100644

drivers/block/zram/zram_drv.c static int zram_rw_page(struct block_device *bdev, sector_t sector, · line 982

Quoted 6 lines

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

@@ -978,12 +978,10 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 out_unlock:
 	up_read(&zram->init_lock);
 out:
-	page_endio(page, rw, err);
+	if (unlikely(err))
+		return err;

this unlikely() case can be turned into a likely() one:

	if (err == 0)
		page_endio(page, rw, 0);
	return err;

drivers/block/zram/zram_drv.c static int zram_rw_page(struct block_device *bdev, sector_t sector, · line 985

Quoted 7 lines

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

-	/*
-	 * Return 0 prevents I/O fallback trial caused by rw_page fail
-	 * and upper layer can handle this IO error via page error.
-	 */
+	page_endio(page, rw, 0);
 	return 0;
 }

seems like we also can drop at least one goto (jump-to-return) for invalid request.

(not sure about `goto out_unblock', yet another up_read(&zram->init_lock) just will make function bigger).

Signed-off-by Sergey Senozhatsky <[email protected]>

---

 drivers/block/zram/zram_drv.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

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

index 0e0650f..decca6f 100644
@@ -956,8 +956,7 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 	zram = bdev->bd_disk->private_data;
 	if (!valid_io_request(zram, sector, PAGE_SIZE)) {
 		atomic64_inc(&zram->stats.invalid_io);
-		err = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	down_read(&zram->init_lock);

@@ -974,15 +973,11 @@ 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);
-out:
-	if (unlikely(err))
-		return err;
-
-	page_endio(page, rw, 0);
-	return 0;
+	if (err == 0)
+		page_endio(page, rw, 0);
+	return err;
 }
 
 static const struct block_device_operations zram_devops = {

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

Andrew Morton

maintainer

2014-11-18 23:23 UTC #2 lore raw

In reply to Minchan Kim

Quoted 26 lines · On Fri, 14 Nov 2014 09:49:07 +0900 Minchan Kim <[email protected]> wrote:

When I tested zram, I found processes got segfaulted. The reason was zram_rw_page doesn't make the page dirty again when swap write failed, and even it doesn't return error by [1].
If error by zram internal happens, zram_rw_page should return non-zero without calling page_endio.
It causes resubmit the IO with bio so that it ends up calling bio->bi_end_io.
The reason is zram could be used for a block device for FS and swap, which they uses different bio complete callback, which works differently. So, we should rely on the bio I/O complete handler rather than zram_bvec_rw itself in case of I/O fail.
This patch fixes the segfault issue as well one [1]'s mentioned
...
drivers/block/zram/zram_drv.c +0 -0
@@ -978,12 +978,10 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 out_unlock:
 	up_read(&zram->init_lock);
 out:

drivers/block/zram/zram_drv.c static int zram_rw_page(struct block_device *bdev, sector_t sector, · line 985

Quoted 10 lines

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

-	page_endio(page, rw, err);
+	if (unlikely(err))
+		return err;
 
-	/*
-	 * Return 0 prevents I/O fallback trial caused by rw_page fail
-	 * and upper layer can handle this IO error via page error.
-	 */
+	page_endio(page, rw, 0);
 	return 0;

Losing the comment makes me sad. The code is somewhat odd-looking. We should add some words explaining why we're not reporting errors at this point.

Minchan Kim

author maintainer

2014-11-18 23:51 UTC #3 lore raw

Quoted 32 lines · Andrew Morton

Quoted 30 lines · On Fri, 14 Nov 2014 09:49:07 +0900 Minchan Kim <[email protected]> wrote:
When I tested zram, I found processes got segfaulted. The reason was zram_rw_page doesn't make the page dirty again when swap write failed, and even it doesn't return error by [1].
If error by zram internal happens, zram_rw_page should return non-zero without calling page_endio.
It causes resubmit the IO with bio so that it ends up calling bio->bi_end_io.
The reason is zram could be used for a block device for FS and swap, which they uses different bio complete callback, which works differently. So, we should rely on the bio I/O complete handler rather than zram_bvec_rw itself in case of I/O fail.
This patch fixes the segfault issue as well one [1]'s mentioned
...
drivers/block/zram/zram_drv.c +2 -1
@@ -978,12 +978,10 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 out_unlock:
 	up_read(&zram->init_lock);
 out:
-	page_endio(page, rw, err);
+	if (unlikely(err))
+		return err;
 

Quoted 10 lines

drivers/block/zram/zram_drv.c static int zram_rw_page(struct block_device *bdev, sector_t sector, · line 985
Quoted 6 lines
drivers/block/zram/zram_drv.c +1 -4
-	/*
-	 * Return 0 prevents I/O fallback trial caused by rw_page fail
-	 * and upper layer can handle this IO error via page error.
-	 */
+	page_endio(page, rw, 0);
 	return 0;
Losing the comment makes me sad. The code is somewhat odd-looking. We should add some words explaining why we're not reporting errors at this point.

Okay. How about this?

drivers/block/zram/zram_drv.c +6 -0

index decca6f161b8..1d7c90d5e0d0 100644
@@ -975,6 +975,12 @@ 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);
+	/*
+	 * If I/O fails, just return error without calling page_endio.
+	 * It causes resubmit the I/O with bio request by rw_page fallback
+	 * and bio I/O complete handler does things to handle the error
+	 * (e.g., set_page_dirty of swap_writepage fail).
+	 */
 	if (err == 0)
 		page_endio(page, rw, 0);
 	return err;

Quoted 6 lines

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

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>

Andrew Morton

maintainer

2014-11-19 21:15 UTC #4 lore raw

Quoted 22 lines · On Wed, 19 Nov 2014 08:52:01 +0900 Minchan Kim <[email protected]> wrote:

Quoted 11 lines
Review
Quoted 7 lines
(quoted diff) +1 -4
-	/*
-	 * Return 0 prevents I/O fallback trial caused by rw_page fail
-	 * and upper layer can handle this IO error via page error.
-	 */
+	page_endio(page, rw, 0);
 	return 0;
Losing the comment makes me sad. The code is somewhat odd-looking. We should add some words explaining why we're not reporting errors at this point.
Okay. How about this?
drivers/block/zram/zram_drv.c +0 -0
index decca6f161b8..1d7c90d5e0d0 100644
@@ -975,6 +975,12 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 	err = zram_bvec_rw(zram, &bv, index, offset, rw);
 out_unlock:

drivers/block/zram/zram_drv.c static int zram_rw_page(struct block_device *bdev, sector_t sector, · line 986

Quoted 10 lines

drivers/block/zram/zram_drv.c +6 -0

 	up_read(&zram->init_lock);
+	/*
+	 * If I/O fails, just return error without calling page_endio.
+	 * It causes resubmit the I/O with bio request by rw_page fallback
+	 * and bio I/O complete handler does things to handle the error
+	 * (e.g., set_page_dirty of swap_writepage fail).
+	 */
 	if (err == 0)
 		page_endio(page, rw, 0);
 	return err;

I don't understand the comment :( bdev_read_page() doesn't resubmit the IO if block_device_operations.rw_page() returns zero and it's unclear how the bio I/O complete handler (which one?) gets involved.

It would help in the comment was more specific. Instead of using vague terms like "rw_page fallback" and "bio I/O complete handler", use actual function names so the reader understand exactly what code we're referring to.

Minchan Kim

author maintainer

2014-11-19 23:32 UTC #5 lore raw

Hello,

Quoted 29 lines · Andrew Morton

Quoted 27 lines · On Wed, 19 Nov 2014 08:52:01 +0900 Minchan Kim <[email protected]> wrote:
Quoted 11 lines
Review
Quoted 7 lines
(quoted diff) +1 -4
-	/*
-	 * Return 0 prevents I/O fallback trial caused by rw_page fail
-	 * and upper layer can handle this IO error via page error.
-	 */
+	page_endio(page, rw, 0);
 	return 0;
Losing the comment makes me sad. The code is somewhat odd-looking. We should add some words explaining why we're not reporting errors at this point.
Okay. How about this?
drivers/block/zram/zram_drv.c +4 -0
index decca6f161b8..1d7c90d5e0d0 100644
@@ -975,6 +975,12 @@ 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);
+	/*
+	 * If I/O fails, just return error without calling page_endio.
+	 * It causes resubmit the I/O with bio request by rw_page fallback
+	 * and bio I/O complete handler does things to handle the error

Quoted 8 lines

drivers/block/zram/zram_drv.c static int zram_rw_page(struct block_device *bdev, sector_t sector, · line 986
Quoted 5 lines
drivers/block/zram/zram_drv.c +2 -0
+	 * (e.g., set_page_dirty of swap_writepage fail).
+	 */
 	if (err == 0)
 		page_endio(page, rw, 0);
 	return err;
I don't understand the comment :( bdev_read_page() doesn't resubmit the IO if block_device_operations.rw_page() returns zero and it's unclear

It's not bdev_read_page but upper functions.
(ie, do_mpage_readpage, swap_readpage, __mpage_writepage, __swap_writepage)

Quoted 1 line

how the bio I/O complete handler (which one?) gets involved.

bio->bi_end_io.

Quoted 5 lines

It would help in the comment was more specific. Instead of using vague terms like "rw_page fallback" and "bio I/O complete handler", use actual function names so the reader understand exactly what code we're referring to.

Indeed. I was terrible.

Hope this is better.

Quoted 1 line

From 635f22cac08f76b909ccc13b2de0b5d437eef5aa Mon Sep 17 00:00:00 2001

From: Minchan Kim <[email protected]>
Date: Wed, 19 Nov 2014 08:57:49 +0900
Subject: [PATCH] zram: add comment

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

---
 drivers/block/zram/zram_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/block/zram/zram_drv.c +8 -0

index decca6f161b8..6f1ceb14e64b 100644
@@ -975,6 +975,14 @@ 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);
+	/*
+	 * If I/O fails, just return error(ie, non-zero) without
+	 * calling page_endio.
+	 * It causes resubmit the I/O with bio request by upper functions
+	 * of rw_page(e.g., swap_readpage, __swap_writepage) and
+	 * bio->bi_end_io does things to handle the error
+	 * (e.g., SetPageError, set_page_dirty and extra works).
+	 */
 	if (err == 0)
 		page_endio(page, rw, 0);
 	return err;

Signature · 1 line

2.0.0

Quoted 6 lines

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

Signature · 2 lines

Kind regards,
Minchan Kim