Re: [PATCH 1/2] iomap: simplify iomap_readpage_actor

[Matthew Wilcox <willy@infradead.org>]

On Tue, Jul 20, 2021 at 10:43:19AM +0200, Christoph Hellwig wrote:
> Now that the outstanding reads are counted in bytes, there is no need
> to use the low-level __bio_try_merge_page API, we can switch back to
> always using bio_add_page and simply iomap_readpage_actor again.

I don't think this quite works.  You need to check the return value
from bio_add_page(), otherwise you can be in a situation where you try
to add a page to the last bvec and it's not contiguous, so it fails.

I was imagining something more like this:

-       bool same_page = false, is_contig = false;
+       bool is_contig = false;
...
        /* Try to merge into a previous segment if we can */
        sector = iomap_sector(iomap, pos);
-       if (ctx->bio && bio_end_sector(ctx->bio) == sector) {
-               if (__bio_try_merge_page(ctx->bio, page, plen, poff,
-                               &same_page))
-                       goto done;
-               is_contig = true;
-       }
+       if (ctx->bio && bio_end_sector(ctx->bio) == sector)
+               is_contig = bio_add_page(ctx->bio, page, plen, poff) > 0;

-       if (!is_contig || bio_full(ctx->bio, plen)) {
+       if (!is_contig) {
                gfp_t gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL);
                gfp_t orig_gfp = gfp;
...
                bio_set_dev(ctx->bio, iomap->bdev);
                ctx->bio->bi_end_io = iomap_read_end_io;
+               bio_add_page(ctx->bio, page, plen, poff);
        }

-       bio_add_page(ctx->bio, page, plen, poff);
 done:
        /*