| Message ID | 58fa893c24c67340a63323f09a179fefdca07f2a.1685532726.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bio: check return values of bio_add_page | expand |
On Wed, May 31, 2023 at 04:50:42AM -0700, Johannes Thumshirn wrote: > When the iomap buffered-io code can't add a folio to a bio, it allocates a > new bio and adds the folio to that one. This is done using bio_add_folio(), > but doesn't check for errors. > > As adding a folio to a newly created bio can't fail, use the newly > introduced bio_add_folio_nofail() function. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/iomap/buffered-io.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 063133ec77f4..0edab9deae2a 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -312,7 +312,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter, > ctx->bio->bi_opf |= REQ_RAHEAD; > ctx->bio->bi_iter.bi_sector = sector; > ctx->bio->bi_end_io = iomap_read_end_io; > - bio_add_folio(ctx->bio, folio, plen, poff); > + bio_add_folio_nofail(ctx->bio, folio, plen, poff); > } > > done: > @@ -539,7 +539,7 @@ static int iomap_read_folio_sync(loff_t block_start, struct folio *folio, > > bio_init(&bio, iomap->bdev, &bvec, 1, REQ_OP_READ); > bio.bi_iter.bi_sector = iomap_sector(iomap, block_start); > - bio_add_folio(&bio, folio, plen, poff); > + bio_add_folio_nofail(&bio, folio, plen, poff); > return submit_bio_wait(&bio); > } > > @@ -1582,7 +1582,7 @@ iomap_add_to_ioend(struct inode *inode, loff_t pos, struct folio *folio, > > if (!bio_add_folio(wpc->ioend->io_bio, folio, len, poff)) { > wpc->ioend->io_bio = iomap_chain_bio(wpc->ioend->io_bio); > - bio_add_folio(wpc->ioend->io_bio, folio, len, poff); > + bio_add_folio_nofail(wpc->ioend->io_bio, folio, len, poff); > } We lose adjacent page merging with this change. We've had performance regressions in the past that have been attributed to either the page allocator not handing out sequential adjacent pages for sequential writes and/or bios not merging adjacent pages. Some hardware is much more performant when it only has to do a single large DMA instead of (potentially) hundreds of single page DMAs for the same amount of data... What performance regression testing has been done on this change? -Dave.
On Thu, Jun 01, 2023 at 08:36:59AM +1000, Dave Chinner wrote:
> We lose adjacent page merging with this change.
This is only used for adding the first folio to a brand new bio,
so there is nothing to merge with yet at this point.
On Thu, Jun 01, 2023 at 06:20:21AM +0200, Christoph Hellwig wrote: > On Thu, Jun 01, 2023 at 08:36:59AM +1000, Dave Chinner wrote: > > We lose adjacent page merging with this change. > > This is only used for adding the first folio to a brand new bio, > so there is nothing to merge with yet at this point. Ah, sorry, missed that. Carry on. :) -Dave.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 063133ec77f4..0edab9deae2a 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -312,7 +312,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter, ctx->bio->bi_opf |= REQ_RAHEAD; ctx->bio->bi_iter.bi_sector = sector; ctx->bio->bi_end_io = iomap_read_end_io; - bio_add_folio(ctx->bio, folio, plen, poff); + bio_add_folio_nofail(ctx->bio, folio, plen, poff); } done: @@ -539,7 +539,7 @@ static int iomap_read_folio_sync(loff_t block_start, struct folio *folio, bio_init(&bio, iomap->bdev, &bvec, 1, REQ_OP_READ); bio.bi_iter.bi_sector = iomap_sector(iomap, block_start); - bio_add_folio(&bio, folio, plen, poff); + bio_add_folio_nofail(&bio, folio, plen, poff); return submit_bio_wait(&bio); } @@ -1582,7 +1582,7 @@ iomap_add_to_ioend(struct inode *inode, loff_t pos, struct folio *folio, if (!bio_add_folio(wpc->ioend->io_bio, folio, len, poff)) { wpc->ioend->io_bio = iomap_chain_bio(wpc->ioend->io_bio); - bio_add_folio(wpc->ioend->io_bio, folio, len, poff); + bio_add_folio_nofail(wpc->ioend->io_bio, folio, len, poff); } if (iop)