Re: [PATCH] iomap: Submit BIOs at the end of each extent

[Matthew Wilcox <willy@infradead.org>]

On Thu, Mar 19, 2020 at 08:18:19AM -0700, Darrick J. Wong wrote:
> On Thu, Mar 19, 2020 at 08:07:20AM -0700, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > 
> > By definition, an extent covers a range of consecutive blocks, so
> > it would be quite rare to be able to just add pages to the BIO from
> > a previous range.  The only case we can think of is a mapped extent
> > followed by a hole extent, followed by another mapped extent which has
> > been allocated immediately after the first extent.  We believe this to
> 
> Well... userspace can induce that with fallocate(INSERT_RANGE). :)

It's not impossible, of course ... just unlikely.  Nobody actually uses
INSERT_RANGE anyway.

> > be an unlikely layout for a filesystem to choose and, since the queue
> > is plugged, those two BIOs would be merged by the block layer.
> > 
> > The reason we care is that ext2/ext4 choose to lay out blocks 0-11
> > consecutively, followed by the indirect block, and we want to merge those
> > two BIOs.  If we don't submit the data BIO before asking the filesystem
> > for the next extent, then the indirect BIO will be submitted first,
> > and waited for, leading to inefficient I/O patterns.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  fs/iomap/buffered-io.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 83438b3257de..8d26920ddf00 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -388,6 +388,11 @@ iomap_readahead_actor(struct inode *inode, loff_t pos, loff_t length,
> >  				ctx, iomap, srcmap);
> >  	}
> >  
> > +	if (ctx->bio) {
> > +		submit_bio(ctx->bio);
> > +		ctx->bio = NULL;
> > +	}
> 
> Makes sense, but could we have a quick comment here to capture why we're
> submitting the bio here?
> 
> /*
>  * Submit the bio now so that we neither combine IO requests for
>  * non-adjacent ranges nor interleave data and metadata requests.
>  */

How about:

         * Submitting the bio here leads to better I/O patterns for
         * filesystems which need to do metadata reads to find the
         * next range.

I also realised we can add:

@@ -454,8 +459,6 @@ iomap_readpages(struct address_space *mapping, struct list_head *pages,
        }
        ret = 0;
 done:
-       if (ctx.bio)
-               submit_bio(ctx.bio);
        if (ctx.cur_page) {
                if (!ctx.cur_page_in_bio)
                        unlock_page(ctx.cur_page);

since we always subit the bio in readpages_actor.