| Message ID | 20200824145511.10500-7-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | THP iomap patches for 5.10 | expand |
On Mon, Aug 24, 2020 at 03:55:07PM +0100, Matthew Wilcox (Oracle) wrote: > Instead of counting bio segments, count the number of bytes submitted. > This insulates us from the block layer's definition of what a 'same page' > is, which is not necessarily clear once THPs are involved. I'd like to see a comment on the definition of struct iomap_page to indicate that read_count (and write count) reflect the byte count of IO currently in flight on the page, not an IO count, because THP makes tracking this via bio state hard. Otherwise it is not at all obvious why it is done and why it is intentional... Otherwise the code looks OK. Cheers, Dave.
On Tue, Aug 25, 2020 at 10:09:02AM +1000, Dave Chinner wrote: > On Mon, Aug 24, 2020 at 03:55:07PM +0100, Matthew Wilcox (Oracle) wrote: > > Instead of counting bio segments, count the number of bytes submitted. > > This insulates us from the block layer's definition of what a 'same page' > > is, which is not necessarily clear once THPs are involved. > > I'd like to see a comment on the definition of struct iomap_page to > indicate that read_count (and write count) reflect the byte count of > IO currently in flight on the page, not an IO count, because THP > makes tracking this via bio state hard. Otherwise it is not at all > obvious why it is done and why it is intentional... Agreed. :) --D > Otherwise the code looks OK. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
> @@ -269,20 +263,17 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > if (ctx->bio && bio_end_sector(ctx->bio) == sector) > is_contig = true; > > - > /* > - * If we start a new segment we need to increase the read count, and we > - * need to do so before submitting any previous full bio to make sure > - * that we don't prematurely unlock the page. > + * We need to increase the read count before submitting any > + * previous bio to make sure that we don't prematurely unlock > + * the page. > */ > if (iop) > - atomic_inc(&iop->read_count); > + atomic_add(plen, &iop->read_count); > + > + if (is_contig && > + __bio_try_merge_page(ctx->bio, page, plen, poff, &same_page)) > + goto done; > > if (!ctx->bio || !is_contig || bio_full(ctx->bio, plen)) { I think we can simplify this a bit by reordering the checks: if (iop) atomic_add(plen, &iop->read_count); 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 (!is_contig || bio_full(ctx->bio, plen)) { // the existing case to allocate a new bio } Also I think read_count should be renamed to be more descriptive, something like read_bytes_pending? Same for the write side.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 844e95cacea8..c9b44f86d166 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -161,13 +161,6 @@ iomap_set_range_uptodate(struct page *page, unsigned off, unsigned len) SetPageUptodate(page); } -static void -iomap_read_finish(struct iomap_page *iop, struct page *page) -{ - if (!iop || atomic_dec_and_test(&iop->read_count)) - unlock_page(page); -} - static void iomap_read_page_end_io(struct bio_vec *bvec, int error) { @@ -181,7 +174,8 @@ iomap_read_page_end_io(struct bio_vec *bvec, int error) iomap_set_range_uptodate(page, bvec->bv_offset, bvec->bv_len); } - iomap_read_finish(iop, page); + if (!iop || atomic_sub_and_test(bvec->bv_len, &iop->read_count)) + unlock_page(page); } static void @@ -269,20 +263,17 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data, if (ctx->bio && bio_end_sector(ctx->bio) == sector) is_contig = true; - if (is_contig && - __bio_try_merge_page(ctx->bio, page, plen, poff, &same_page)) { - if (!same_page && iop) - atomic_inc(&iop->read_count); - goto done; - } - /* - * If we start a new segment we need to increase the read count, and we - * need to do so before submitting any previous full bio to make sure - * that we don't prematurely unlock the page. + * We need to increase the read count before submitting any + * previous bio to make sure that we don't prematurely unlock + * the page. */ if (iop) - atomic_inc(&iop->read_count); + atomic_add(plen, &iop->read_count); + + if (is_contig && + __bio_try_merge_page(ctx->bio, page, plen, poff, &same_page)) + goto done; if (!ctx->bio || !is_contig || bio_full(ctx->bio, plen)) { gfp_t gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL);
Instead of counting bio segments, count the number of bytes submitted. This insulates us from the block layer's definition of what a 'same page' is, which is not necessarily clear once THPs are involved. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/iomap/buffered-io.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-)