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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > ---
> > 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.
