diff mbox series

[v2,1/5] zram: always chain bio to the parent in read_from_bdev_async

Message ID 20230403132221.94921-2-p.raghav@samsung.com
State New
Headers
Series remove page_endio() |

Commit Message

Pankaj Raghav April 3, 2023, 1:22 p.m. UTC 
  zram_bvec_read() is called with the bio set to NULL only in
writeback_store() function. When a writeback is triggered,
zram_bvec_read() is called only if ZRAM_WB flag is not set. That will
result only calling zram_read_from_zspool() in __zram_bvec_read().

rw_page callback used to call read_from_bdev_async with a NULL parent
bio but that has been removed since commit 3222d8c2a7f8
("block: remove ->rw_page").

We can now safely always call bio_chain() as read_from_bdev_async() will
be called with a parent bio set. A WARN_ON_ONCE is added if this function
is called with parent set to NULL.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 drivers/block/zram/zram_drv.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

Comments

Minchan Kim April 3, 2023, 9:19 p.m. UTC | #1 
On Mon, Apr 03, 2023 at 03:22:17PM +0200, Pankaj Raghav wrote:
> zram_bvec_read() is called with the bio set to NULL only in
> writeback_store() function. When a writeback is triggered,
> zram_bvec_read() is called only if ZRAM_WB flag is not set. That will
> result only calling zram_read_from_zspool() in __zram_bvec_read().
> 
> rw_page callback used to call read_from_bdev_async with a NULL parent
> bio but that has been removed since commit 3222d8c2a7f8
> ("block: remove ->rw_page").
> 
> We can now safely always call bio_chain() as read_from_bdev_async() will
> be called with a parent bio set. A WARN_ON_ONCE is added if this function
> is called with parent set to NULL.
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
Acked-by: Minchan Kim <minchan@kernel.org>

Thanks.
Christoph Hellwig April 4, 2023, 3:06 p.m. UTC | #2 
On Mon, Apr 03, 2023 at 03:22:17PM +0200, Pankaj Raghav wrote:
> zram_bvec_read() is called with the bio set to NULL only in
> writeback_store() function. When a writeback is triggered,
> zram_bvec_read() is called only if ZRAM_WB flag is not set. That will
> result only calling zram_read_from_zspool() in __zram_bvec_read().
> 
> rw_page callback used to call read_from_bdev_async with a NULL parent
> bio but that has been removed since commit 3222d8c2a7f8
> ("block: remove ->rw_page").
> 
> We can now safely always call bio_chain() as read_from_bdev_async() will
> be called with a parent bio set. A WARN_ON_ONCE is added if this function
> is called with parent set to NULL.

I'm pretty sure this is wrong.  I've now sent a series to untangle
and fix up the zram I/O path, which should address the underlying
issue here.

It will obviously conflict with this patch, so maybe the best thing is
to get the other page_endio removals into their respective maintainer
trees, and then just do the final removal of the unused function after
-rc1.
Andrew Morton April 4, 2023, 7:31 p.m. UTC | #3 
On Tue, 4 Apr 2023 08:06:55 -0700 Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Apr 03, 2023 at 03:22:17PM +0200, Pankaj Raghav wrote:
> > zram_bvec_read() is called with the bio set to NULL only in
> > writeback_store() function. When a writeback is triggered,
> > zram_bvec_read() is called only if ZRAM_WB flag is not set. That will
> > result only calling zram_read_from_zspool() in __zram_bvec_read().
> > 
> > rw_page callback used to call read_from_bdev_async with a NULL parent
> > bio but that has been removed since commit 3222d8c2a7f8
> > ("block: remove ->rw_page").
> > 
> > We can now safely always call bio_chain() as read_from_bdev_async() will
> > be called with a parent bio set. A WARN_ON_ONCE is added if this function
> > is called with parent set to NULL.
> 
> I'm pretty sure this is wrong.

Thanks, I'll drop this v2 series.

>  I've now sent a series to untangle
> and fix up the zram I/O path, which should address the underlying
> issue here.

I can't find that series.

> It will obviously conflict with this patch, so maybe the best thing is
> to get the other page_endio removals into their respective maintainer
> trees, and then just do the final removal of the unused function after
> -rc1.
Christoph Hellwig April 5, 2023, 6:07 a.m. UTC | #4 
On Tue, Apr 04, 2023 at 12:31:31PM -0700, Andrew Morton wrote:
> >  I've now sent a series to untangle
> > and fix up the zram I/O path, which should address the underlying
> > issue here.
> 
> I can't find that series.

https://lore.kernel.org/linux-block/20230404150536.2142108-1-hch@lst.de/T/#t
Pankaj Raghav April 11, 2023, 7:34 a.m. UTC | #5 
> 
> It will obviously conflict with this patch, so maybe the best thing is
> to get the other page_endio removals into their respective maintainer
> trees, and then just do the final removal of the unused function after
> -rc1.

Alright, I will drop the last patch that removes the page_endio function, and
send it after rc1. I will make the other changes as suggested by you.
diff mbox series

Patch

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 3feadfb96114..d16d0630b178 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -606,15 +606,6 @@  static void free_block_bdev(struct zram *zram, unsigned long blk_idx)
 	atomic64_dec(&zram->stats.bd_count);
 }
 
-static void zram_page_end_io(struct bio *bio)
-{
-	struct page *page = bio_first_page_all(bio);
-
-	page_endio(page, op_is_write(bio_op(bio)),
-			blk_status_to_errno(bio->bi_status));
-	bio_put(bio);
-}
-
 /*
  * Returns 1 if the submission is successful.
  */
@@ -634,11 +625,10 @@  static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
 		return -EIO;
 	}
 
-	if (!parent)
-		bio->bi_end_io = zram_page_end_io;
-	else
-		bio_chain(bio, parent);
+	if (WARN_ON_ONCE(!parent))
+		return -EINVAL;
 
+	bio_chain(bio, parent);
 	submit_bio(bio);
 	return 1;
 }