[PATCH 0/2 v2] iomap: Cleanup of iomap_dio_rw()

[PATCH 0/2 v2] iomap: Cleanup of iomap_dio_rw()

[Jan Kara]

Hello,

here is the second version of the series. Since Darrick has already picked up
the patch fixing pipe page leakage, I'm resending only the updated cleanup
patch.

Changes since v1:
* Dropped fix patch as it is already in Darrick's tree
* Rebased cleanup patch on top of iomap tree (Christoph)
* Changed code in iomap_dio_rw() to reexpand the iter only in one place and
  jump there from elsewhere (Christoph)
* Expanded comment and moved 'orig_count' initialization (Christoph)

								Honza
Previous versions:
Link: http://lore.kernel.org/r/20191121161144.30802-1-jack@suse.cz

[PATCH] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Jan Kara]

iomap_dio_bio_actor() copies iter to a local variable and then limits it
to a file extent we have mapped. When IO is submitted,
iomap_dio_bio_actor() advances the original iter while the copied iter
is advanced inside bio_iov_iter_get_pages(). This logic is non-obvious
especially because both iters still point to same shared structures
(such as pipe info) so if iov_iter_advance() changes anything in the
shared structure, this scheme breaks. Let's just truncate and reexpand
the original iter as needed instead of playing games with copying iters
and keeping them in sync.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/iomap/direct-io.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 420c0c82f0ac..b75cd0b0609b 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -201,12 +201,12 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 	unsigned int blkbits = blksize_bits(bdev_logical_block_size(iomap->bdev));
 	unsigned int fs_block_size = i_blocksize(inode), pad;
 	unsigned int align = iov_iter_alignment(dio->submit.iter);
-	struct iov_iter iter;
 	struct bio *bio;
 	bool need_zeroout = false;
 	bool use_fua = false;
 	int nr_pages, ret = 0;
 	size_t copied = 0;
+	size_t orig_count;
 
 	if ((pos | length | align) & ((1 << blkbits) - 1))
 		return -EINVAL;
@@ -236,15 +236,18 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 	}
 
 	/*
-	 * Operate on a partial iter trimmed to the extent we were called for.
-	 * We'll update the iter in the dio once we're done with this extent.
+	 * Save the original count and trim the iter to just the extent we
+	 * are operating on right now.  The iter will be re-expanded once
+	 * we are done.
 	 */
-	iter = *dio->submit.iter;
-	iov_iter_truncate(&iter, length);
+	orig_count = iov_iter_count(dio->submit.iter);
+	iov_iter_truncate(dio->submit.iter, length);
 
-	nr_pages = iov_iter_npages(&iter, BIO_MAX_PAGES);
-	if (nr_pages <= 0)
-		return nr_pages;
+	nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
+	if (nr_pages <= 0) {
+		ret = nr_pages;
+		goto out;
+	}
 
 	if (need_zeroout) {
 		/* zero out from the start of the block to the write offset */
@@ -257,7 +260,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		size_t n;
 		if (dio->error) {
 			iov_iter_revert(dio->submit.iter, copied);
-			return 0;
+			copied = ret = 0;
+			goto out;
 		}
 
 		bio = bio_alloc(GFP_KERNEL, nr_pages);
@@ -268,7 +272,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		bio->bi_private = dio;
 		bio->bi_end_io = iomap_dio_bio_end_io;
 
-		ret = bio_iov_iter_get_pages(bio, &iter);
+		ret = bio_iov_iter_get_pages(bio, dio->submit.iter);
 		if (unlikely(ret)) {
 			/*
 			 * We have to stop part way through an IO. We must fall
@@ -294,13 +298,11 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 				bio_set_pages_dirty(bio);
 		}
 
-		iov_iter_advance(dio->submit.iter, n);
-
 		dio->size += n;
 		pos += n;
 		copied += n;
 
-		nr_pages = iov_iter_npages(&iter, BIO_MAX_PAGES);
+		nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
 		iomap_dio_submit_bio(dio, iomap, bio);
 	} while (nr_pages);
 
@@ -318,6 +320,9 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		if (pad)
 			iomap_dio_zero(dio, iomap, pos, fs_block_size - pad);
 	}
+out:
+	/* Undo iter limitation to current extent */
+	iov_iter_reexpand(dio->submit.iter, orig_count - copied);
 	if (copied)
 		return copied;
 	return ret;
-- 
2.16.4

Re: [PATCH] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Matthew Bobrowski]

On Mon, Nov 25, 2019 at 12:18:57PM +0100, Jan Kara wrote:
> iomap_dio_bio_actor() copies iter to a local variable and then limits it
> to a file extent we have mapped. When IO is submitted,
> iomap_dio_bio_actor() advances the original iter while the copied iter
> is advanced inside bio_iov_iter_get_pages(). This logic is non-obvious
> especially because both iters still point to same shared structures
> (such as pipe info) so if iov_iter_advance() changes anything in the
> shared structure, this scheme breaks. Let's just truncate and reexpand
> the original iter as needed instead of playing games with copying iters
> and keeping them in sync.

Looks good. Just one minor nit below which is eating me. I guess
Darrick can fix it up when applying it to his tree, if deemed
necessary to fix up.

Feel free to add:

Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>

>  	/*
> -	 * Operate on a partial iter trimmed to the extent we were called for.
> -	 * We'll update the iter in the dio once we're done with this extent.
> +	 * Save the original count and trim the iter to just the extent we
> +	 * are operating on right now.  The iter will be re-expanded once
  	       		    	      ^^
				      Extra whitespace here.

IMO, I think we can word the last sentence a little better too i.e.

/*                                                                               
 * Save the original count and trim the iter to the extent that we're            
 * currently operating on right now. The iter will then again be                 
 * expanded out once we're done.                                                 
 */

/M

Re: [PATCH] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Matthew Wilcox]

On Tue, Nov 26, 2019 at 08:11:50AM +1100, Matthew Bobrowski wrote:
> > +	 * are operating on right now.  The iter will be re-expanded once
>   	       		    	      ^^
> 				      Extra whitespace here.

That's controversial, not wrong.  We don't normally enforce a style there.
https://www.theatlantic.com/science/archive/2018/05/two-spaces-after-a-period/559304/
(for example.  you can find many many many pieces extolling one or
two spaces).

Re: [PATCH] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Darrick J. Wong]

 n Mon, Nov 25, 2019 at 12:18:57PM +0100, Jan Kara wrote:
> iomap_dio_bio_actor() copies iter to a local variable and then limits it
> to a file extent we have mapped. When IO is submitted,
> iomap_dio_bio_actor() advances the original iter while the copied iter
> is advanced inside bio_iov_iter_get_pages(). This logic is non-obvious
> especially because both iters still point to same shared structures
> (such as pipe info) so if iov_iter_advance() changes anything in the
> shared structure, this scheme breaks. Let's just truncate and reexpand
> the original iter as needed instead of playing games with copying iters
> and keeping them in sync.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks ok, seems to test ok too
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/iomap/direct-io.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 420c0c82f0ac..b75cd0b0609b 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -201,12 +201,12 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  	unsigned int blkbits = blksize_bits(bdev_logical_block_size(iomap->bdev));
>  	unsigned int fs_block_size = i_blocksize(inode), pad;
>  	unsigned int align = iov_iter_alignment(dio->submit.iter);
> -	struct iov_iter iter;
>  	struct bio *bio;
>  	bool need_zeroout = false;
>  	bool use_fua = false;
>  	int nr_pages, ret = 0;
>  	size_t copied = 0;
> +	size_t orig_count;
>  
>  	if ((pos | length | align) & ((1 << blkbits) - 1))
>  		return -EINVAL;
> @@ -236,15 +236,18 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  	}
>  
>  	/*
> -	 * Operate on a partial iter trimmed to the extent we were called for.
> -	 * We'll update the iter in the dio once we're done with this extent.
> +	 * Save the original count and trim the iter to just the extent we
> +	 * are operating on right now.  The iter will be re-expanded once
> +	 * we are done.
>  	 */
> -	iter = *dio->submit.iter;
> -	iov_iter_truncate(&iter, length);
> +	orig_count = iov_iter_count(dio->submit.iter);
> +	iov_iter_truncate(dio->submit.iter, length);
>  
> -	nr_pages = iov_iter_npages(&iter, BIO_MAX_PAGES);
> -	if (nr_pages <= 0)
> -		return nr_pages;
> +	nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
> +	if (nr_pages <= 0) {
> +		ret = nr_pages;
> +		goto out;
> +	}
>  
>  	if (need_zeroout) {
>  		/* zero out from the start of the block to the write offset */
> @@ -257,7 +260,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  		size_t n;
>  		if (dio->error) {
>  			iov_iter_revert(dio->submit.iter, copied);
> -			return 0;
> +			copied = ret = 0;
> +			goto out;
>  		}
>  
>  		bio = bio_alloc(GFP_KERNEL, nr_pages);
> @@ -268,7 +272,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  		bio->bi_private = dio;
>  		bio->bi_end_io = iomap_dio_bio_end_io;
>  
> -		ret = bio_iov_iter_get_pages(bio, &iter);
> +		ret = bio_iov_iter_get_pages(bio, dio->submit.iter);
>  		if (unlikely(ret)) {
>  			/*
>  			 * We have to stop part way through an IO. We must fall
> @@ -294,13 +298,11 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  				bio_set_pages_dirty(bio);
>  		}
>  
> -		iov_iter_advance(dio->submit.iter, n);
> -
>  		dio->size += n;
>  		pos += n;
>  		copied += n;
>  
> -		nr_pages = iov_iter_npages(&iter, BIO_MAX_PAGES);
> +		nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES);
>  		iomap_dio_submit_bio(dio, iomap, bio);
>  	} while (nr_pages);
>  
> @@ -318,6 +320,9 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
>  		if (pad)
>  			iomap_dio_zero(dio, iomap, pos, fs_block_size - pad);
>  	}
> +out:
> +	/* Undo iter limitation to current extent */
> +	iov_iter_reexpand(dio->submit.iter, orig_count - copied);
>  	if (copied)
>  		return copied;
>  	return ret;
> -- 
> 2.16.4
> 

Re: [PATCH] iomap: Do not create fake iter in iomap_dio_bio_actor()

[Matthew Bobrowski]

On Tue, Nov 26, 2019 at 07:12:16AM -0800, Matthew Wilcox wrote:
> On Tue, Nov 26, 2019 at 08:11:50AM +1100, Matthew Bobrowski wrote:
> > > +	 * are operating on right now.  The iter will be re-expanded once
> >   	       		    	      ^^
> > 				      Extra whitespace here.
> 
> That's controversial, not wrong.  We don't normally enforce a style there.
> https://www.theatlantic.com/science/archive/2018/05/two-spaces-after-a-period/559304/
> (for example.  you can find many many many pieces extolling one or
> two spaces).

Indeed controversial, a good read, and thank you for sharing. I guess
that I haven't been brought up with two spaces after a period being a
"thing", so it makes my wires trip when glancing over a snippet of
text.

At least I'll know this for next time. :)

/M