| Message ID | 20230403132221.94921-2-p.raghav@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] zram: always chain bio to the parent in read_from_bdev_async | expand |
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.
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.
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.
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
> > 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 --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; }
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(-)