diff mbox series

[6/9] iomap: Convert read_count to byte count

Message ID 20200824145511.10500-7-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series THP iomap patches for 5.10 | expand

Commit Message

Matthew Wilcox Aug. 24, 2020, 2:55 p.m. UTC
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(-)

Comments

Dave Chinner Aug. 25, 2020, 12:09 a.m. UTC | #1
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.
Darrick J. Wong Aug. 25, 2020, 10:14 p.m. UTC | #2
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
Christoph Hellwig Aug. 27, 2020, 8:35 a.m. UTC | #3
> @@ -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 mbox series

Patch

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);