* [PATCH] zram: rely on the bi_end_io for zram_rw_page fails @ 2014-11-14 0:49 Minchan Kim 2014-11-15 9:19 ` Sergey Senozhatsky 2014-11-18 23:23 ` Andrew Morton 0 siblings, 2 replies; 6+ messages in thread From: Minchan Kim @ 2014-11-14 0:49 UTC (permalink / raw) To: Andrew Morton Cc: Nitin Gupta, Jerome Marchand, Sergey Senozhatsky, linux-mm, linux-kernel, Minchan Kim, Matthew Wilcox, Karam Lee, Dave Chinner 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 <matthew.r.wilcox@intel.com> Cc: Karam Lee <karam.lee@lge.com> Cc: Dave Chinner <david@fromorbit.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- drivers/block/zram/zram_drv.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 4b4f4dbc3cfd..0e0650feab2a 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -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; } -- 2.0.0 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: rely on the bi_end_io for zram_rw_page fails 2014-11-14 0:49 [PATCH] zram: rely on the bi_end_io for zram_rw_page fails Minchan Kim @ 2014-11-15 9:19 ` Sergey Senozhatsky 2014-11-18 23:23 ` Andrew Morton 1 sibling, 0 replies; 6+ messages in thread From: Sergey Senozhatsky @ 2014-11-15 9:19 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, Nitin Gupta, Jerome Marchand, Sergey Senozhatsky, linux-mm, linux-kernel, Matthew Wilcox, Karam Lee, Dave Chinner Hi, On (11/14/14 09:49), Minchan Kim 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 > > [1] zram: make rw_page opeartion return 0 > > Cc: Matthew Wilcox <matthew.r.wilcox@intel.com> > Cc: Karam Lee <karam.lee@lge.com> > Cc: Dave Chinner <david@fromorbit.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> > --- > drivers/block/zram/zram_drv.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 4b4f4dbc3cfd..0e0650feab2a 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -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; > - /* > - * 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 <sergey.senozhatsky@gmail.com> --- drivers/block/zram/zram_drv.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 0e0650f..decca6f 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -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 majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: rely on the bi_end_io for zram_rw_page fails 2014-11-14 0:49 [PATCH] zram: rely on the bi_end_io for zram_rw_page fails Minchan Kim 2014-11-15 9:19 ` Sergey Senozhatsky @ 2014-11-18 23:23 ` Andrew Morton 2014-11-18 23:52 ` Minchan Kim 1 sibling, 1 reply; 6+ messages in thread From: Andrew Morton @ 2014-11-18 23:23 UTC (permalink / raw) To: Minchan Kim Cc: Nitin Gupta, Jerome Marchand, Sergey Senozhatsky, linux-mm, linux-kernel, Matthew Wilcox, Karam Lee, Dave Chinner On Fri, 14 Nov 2014 09:49:07 +0900 Minchan Kim <minchan@kernel.org> 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 > > ... > > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -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; 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: rely on the bi_end_io for zram_rw_page fails 2014-11-18 23:23 ` Andrew Morton @ 2014-11-18 23:52 ` Minchan Kim 2014-11-19 21:15 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Minchan Kim @ 2014-11-18 23:52 UTC (permalink / raw) To: Andrew Morton Cc: Nitin Gupta, Jerome Marchand, Sergey Senozhatsky, linux-mm, linux-kernel, Matthew Wilcox, Karam Lee, Dave Chinner On Tue, Nov 18, 2014 at 03:23:36PM -0800, Andrew Morton wrote: > On Fri, 14 Nov 2014 09:49:07 +0900 Minchan Kim <minchan@kernel.org> 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 > > > > ... > > > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -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; > > 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? diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index decca6f161b8..1d7c90d5e0d0 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -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; > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: rely on the bi_end_io for zram_rw_page fails 2014-11-18 23:52 ` Minchan Kim @ 2014-11-19 21:15 ` Andrew Morton 2014-11-19 23:32 ` Minchan Kim 0 siblings, 1 reply; 6+ messages in thread From: Andrew Morton @ 2014-11-19 21:15 UTC (permalink / raw) To: Minchan Kim Cc: Nitin Gupta, Jerome Marchand, Sergey Senozhatsky, linux-mm, linux-kernel, Matthew Wilcox, Karam Lee, Dave Chinner On Wed, 19 Nov 2014 08:52:01 +0900 Minchan Kim <minchan@kernel.org> wrote: > > > > > > - /* > > > - * 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? > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index decca6f161b8..1d7c90d5e0d0 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -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; 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: rely on the bi_end_io for zram_rw_page fails 2014-11-19 21:15 ` Andrew Morton @ 2014-11-19 23:32 ` Minchan Kim 0 siblings, 0 replies; 6+ messages in thread From: Minchan Kim @ 2014-11-19 23:32 UTC (permalink / raw) To: Andrew Morton Cc: Nitin Gupta, Jerome Marchand, Sergey Senozhatsky, linux-mm, linux-kernel, Matthew Wilcox, Karam Lee, Dave Chinner Hello, On Wed, Nov 19, 2014 at 01:15:35PM -0800, Andrew Morton wrote: > On Wed, 19 Nov 2014 08:52:01 +0900 Minchan Kim <minchan@kernel.org> wrote: > > > > > > > > > - /* > > > > - * 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? > > > > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > > index decca6f161b8..1d7c90d5e0d0 100644 > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -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; > > 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) > how the bio I/O complete handler (which one?) gets involved. bio->bi_end_io. > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-19 23:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-14 0:49 [PATCH] zram: rely on the bi_end_io for zram_rw_page fails Minchan Kim 2014-11-15 9:19 ` Sergey Senozhatsky 2014-11-18 23:23 ` Andrew Morton 2014-11-18 23:52 ` Minchan Kim 2014-11-19 21:15 ` Andrew Morton 2014-11-19 23:32 ` Minchan Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).