cleanup bio page releasing

cleanup bio page releasing

[Christoph Hellwig]

Hi Jens,

this series cleans up the various direct I/O and pass through
routines by switching them over to common bio helpers.

The last patch just unconditionally applies the no page ref
behavior for bvec iters.  I looked at all the callers, and
there is none that drops the pre-required references before
completing the request.

[PATCH 1/9] block: move the BIO_NO_PAGE_REF check into bio_release_pages

[Christoph Hellwig]

Move the BIO_NO_PAGE_REF check into bio_release_pages instead of
duplicating it in both callers.

Also make the function available outside of bio.c so that we can
reuse it in other direct I/O implementations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 block/bio.c         | 11 ++++++-----
 include/linux/bio.h |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index ad9c3aa9bf7d..9bc7d28ae997 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -845,11 +845,14 @@ static void bio_get_pages(struct bio *bio)
 		get_page(bvec->bv_page);
 }
 
-static void bio_release_pages(struct bio *bio)
+void bio_release_pages(struct bio *bio)
 {
 	struct bvec_iter_all iter_all;
 	struct bio_vec *bvec;
 
+	if (bio_flagged(bio, BIO_NO_PAGE_REF))
+		return;
+
 	bio_for_each_segment_all(bvec, bio, iter_all)
 		put_page(bvec->bv_page);
 }
@@ -1681,8 +1684,7 @@ static void bio_dirty_fn(struct work_struct *work)
 		next = bio->bi_private;
 
 		bio_set_pages_dirty(bio);
-		if (!bio_flagged(bio, BIO_NO_PAGE_REF))
-			bio_release_pages(bio);
+		bio_release_pages(bio);
 		bio_put(bio);
 	}
 }
@@ -1698,8 +1700,7 @@ void bio_check_pages_dirty(struct bio *bio)
 			goto defer;
 	}
 
-	if (!bio_flagged(bio, BIO_NO_PAGE_REF))
-		bio_release_pages(bio);
+	bio_release_pages(bio);
 	bio_put(bio);
 	return;
 defer:
diff --git a/include/linux/bio.h b/include/linux/bio.h
index ee11c4324751..87e6c8637bce 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -426,6 +426,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page *page,
 void __bio_add_page(struct bio *bio, struct page *page,
 		unsigned int len, unsigned int off);
 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
+void bio_release_pages(struct bio *bio);
 struct rq_map_data;
 extern struct bio *bio_map_user_iov(struct request_queue *,
 				    struct iov_iter *, gfp_t);
-- 
2.20.1

Re: [PATCH 1/9] block: move the BIO_NO_PAGE_REF check into bio_release_pages

[Minwoo Im]

On 19-06-26 15:49:20, Christoph Hellwig wrote:
> Move the BIO_NO_PAGE_REF check into bio_release_pages instead of
> duplicating it in both callers.

This part looks good to me.

> Also make the function available outside of bio.c so that we can
> reuse it in other direct I/O implementations.

This is also for the next patches.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

[PATCH 2/9] block: optionally mark pages dirty in bio_release_pages

[Christoph Hellwig]

A lot of callers of bio_release_pages also want to mark the released
pages as dirty.  Add a mark_dirty parameter to avoid a second
relatively expensive bio_for_each_segment_all loop.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c         | 12 +++++++-----
 include/linux/bio.h |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 9bc7d28ae997..7f3920b6baca 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio)
 		get_page(bvec->bv_page);
 }
 
-void bio_release_pages(struct bio *bio)
+void bio_release_pages(struct bio *bio, bool mark_dirty)
 {
 	struct bvec_iter_all iter_all;
 	struct bio_vec *bvec;
@@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio)
 	if (bio_flagged(bio, BIO_NO_PAGE_REF))
 		return;
 
-	bio_for_each_segment_all(bvec, bio, iter_all)
+	bio_for_each_segment_all(bvec, bio, iter_all) {
+		if (mark_dirty && !PageCompound(bvec->bv_page))
+			set_page_dirty_lock(bvec->bv_page);
 		put_page(bvec->bv_page);
+	}
 }
 
 static int __bio_iov_bvec_add_pages(struct bio *bio, struct iov_iter *iter)
@@ -1683,8 +1686,7 @@ static void bio_dirty_fn(struct work_struct *work)
 	while ((bio = next) != NULL) {
 		next = bio->bi_private;
 
-		bio_set_pages_dirty(bio);
-		bio_release_pages(bio);
+		bio_release_pages(bio, true);
 		bio_put(bio);
 	}
 }
@@ -1700,7 +1702,7 @@ void bio_check_pages_dirty(struct bio *bio)
 			goto defer;
 	}
 
-	bio_release_pages(bio);
+	bio_release_pages(bio, false);
 	bio_put(bio);
 	return;
 defer:
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 87e6c8637bce..9dca313d948a 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -426,7 +426,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page *page,
 void __bio_add_page(struct bio *bio, struct page *page,
 		unsigned int len, unsigned int off);
 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter);
-void bio_release_pages(struct bio *bio);
+void bio_release_pages(struct bio *bio, bool mark_dirty);
 struct rq_map_data;
 extern struct bio *bio_map_user_iov(struct request_queue *,
 				    struct iov_iter *, gfp_t);
-- 
2.20.1

Re: [PATCH 2/9] block: optionally mark pages dirty in bio_release_pages

[Minwoo Im]

On 19-06-26 15:49:21, Christoph Hellwig wrote:
> A lot of callers of bio_release_pages also want to mark the released
> pages as dirty.  Add a mark_dirty parameter to avoid a second
> relatively expensive bio_for_each_segment_all loop.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio.c         | 12 +++++++-----
>  include/linux/bio.h |  2 +-
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 9bc7d28ae997..7f3920b6baca 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio)
>  		get_page(bvec->bv_page);
>  }
>  
> -void bio_release_pages(struct bio *bio)
> +void bio_release_pages(struct bio *bio, bool mark_dirty)
>  {
>  	struct bvec_iter_all iter_all;
>  	struct bio_vec *bvec;
> @@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio)
>  	if (bio_flagged(bio, BIO_NO_PAGE_REF))
>  		return;
>  
> -	bio_for_each_segment_all(bvec, bio, iter_all)
> +	bio_for_each_segment_all(bvec, bio, iter_all) {
> +		if (mark_dirty && !PageCompound(bvec->bv_page))
> +			set_page_dirty_lock(bvec->bv_page);

Christoph,

Could you please explain a bit why we should not make it dirty in case
of compound page?

Re: [PATCH 2/9] block: optionally mark pages dirty in bio_release_pages

[Chaitanya Kulkarni]

On 6/26/19, 1:52 PM, "linux-block-owner@vger.kernel.org on behalf of Minwoo Im" <linux-block-owner@vger.kernel.org on behalf of minwoo.im.dev@gmail.com> wrote:

    On 19-06-26 15:49:21, Christoph Hellwig wrote:
    > A lot of callers of bio_release_pages also want to mark the released
    > pages as dirty.  Add a mark_dirty parameter to avoid a second
    > relatively expensive bio_for_each_segment_all loop.
    > 
    > Signed-off-by: Christoph Hellwig <hch@lst.de>
    > ---
    >  block/bio.c         | 12 +++++++-----
    >  include/linux/bio.h |  2 +-
    >  2 files changed, 8 insertions(+), 6 deletions(-)
    > 
    > diff --git a/block/bio.c b/block/bio.c
    > index 9bc7d28ae997..7f3920b6baca 100644
    > --- a/block/bio.c
    > +++ b/block/bio.c
    > @@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio)
    >  		get_page(bvec->bv_page);
    >  }
    >  
    > -void bio_release_pages(struct bio *bio)
    > +void bio_release_pages(struct bio *bio, bool mark_dirty)
    >  {
    >  	struct bvec_iter_all iter_all;
    >  	struct bio_vec *bvec;
    > @@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio)
    >  	if (bio_flagged(bio, BIO_NO_PAGE_REF))
    >  		return;
    >  
    > -	bio_for_each_segment_all(bvec, bio, iter_all)
    > +	bio_for_each_segment_all(bvec, bio, iter_all) {
    > +		if (mark_dirty && !PageCompound(bvec->bv_page))
    > +			set_page_dirty_lock(bvec->bv_page);
    
    Christoph,
    
    Could you please explain a bit why we should not make it dirty in case
    of compound page?

Maybe because of [PATCH 7/9] block_dev: use bio_release_pages in bio_unmap_user :-

@@ -259,13 +258,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
	}
	__set_current_state(TASK_RUNNING);
-	bio_for_each_segment_all(bvec, &bio, iter_all) {
-		if (should_dirty && !PageCompound(bvec->bv_page))
-			set_page_dirty_lock(bvec->bv_page);
-		if (!bio_flagged(&bio, BIO_NO_PAGE_REF))
-			put_page(bvec->bv_page);
-	}
-
+	bio_release_pages(&bio, should_dirty);

I'll let Christoph confirm that.







    

Re: [PATCH 2/9] block: optionally mark pages dirty in bio_release_pages

[Minwoo Im]

On 19-06-27 00:21:27, Chaitanya Kulkarni wrote:
> 
> 
> On 6/26/19, 1:52 PM, "linux-block-owner@vger.kernel.org on behalf of Minwoo Im" <linux-block-owner@vger.kernel.org on behalf of minwoo.im.dev@gmail.com> wrote:
> 
>     On 19-06-26 15:49:21, Christoph Hellwig wrote:
>     > A lot of callers of bio_release_pages also want to mark the released
>     > pages as dirty.  Add a mark_dirty parameter to avoid a second
>     > relatively expensive bio_for_each_segment_all loop.
>     > 
>     > Signed-off-by: Christoph Hellwig <hch@lst.de>
>     > ---
>     >  block/bio.c         | 12 +++++++-----
>     >  include/linux/bio.h |  2 +-
>     >  2 files changed, 8 insertions(+), 6 deletions(-)
>     > 
>     > diff --git a/block/bio.c b/block/bio.c
>     > index 9bc7d28ae997..7f3920b6baca 100644
>     > --- a/block/bio.c
>     > +++ b/block/bio.c
>     > @@ -845,7 +845,7 @@ static void bio_get_pages(struct bio *bio)
>     >  		get_page(bvec->bv_page);
>     >  }
>     >  
>     > -void bio_release_pages(struct bio *bio)
>     > +void bio_release_pages(struct bio *bio, bool mark_dirty)
>     >  {
>     >  	struct bvec_iter_all iter_all;
>     >  	struct bio_vec *bvec;
>     > @@ -853,8 +853,11 @@ void bio_release_pages(struct bio *bio)
>     >  	if (bio_flagged(bio, BIO_NO_PAGE_REF))
>     >  		return;
>     >  
>     > -	bio_for_each_segment_all(bvec, bio, iter_all)
>     > +	bio_for_each_segment_all(bvec, bio, iter_all) {
>     > +		if (mark_dirty && !PageCompound(bvec->bv_page))
>     > +			set_page_dirty_lock(bvec->bv_page);
>     
>     Christoph,
>     
>     Could you please explain a bit why we should not make it dirty in case
>     of compound page?
> 
> Maybe because of [PATCH 7/9] block_dev: use bio_release_pages in bio_unmap_user :-
> 
> @@ -259,13 +258,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
> 	}
> 	__set_current_state(TASK_RUNNING);
> -	bio_for_each_segment_all(bvec, &bio, iter_all) {
> -		if (should_dirty && !PageCompound(bvec->bv_page))
> -			set_page_dirty_lock(bvec->bv_page);
> -		if (!bio_flagged(&bio, BIO_NO_PAGE_REF))
> -			put_page(bvec->bv_page);
> -	}
> -
> +	bio_release_pages(&bio, should_dirty);
> 
> I'll let Christoph confirm that.

Chaitanya,

Thanks for sharing this.  Actually I have seen that commit log but
didn't catch up why the compound pages are so.  Now I understood what
Christoph has mentioned.

Thanks for your help, again.

Re: [PATCH 2/9] block: optionally mark pages dirty in bio_release_pages

[Christoph Hellwig]

On Thu, Jun 27, 2019 at 05:52:47AM +0900, Minwoo Im wrote:
> Could you please explain a bit why we should not make it dirty in case
> of compound page?

PageCompount is only true for the tail pages, and marking them dirty
doesn't work.  On the other hand marking the head page dirty covers
the whole compount, so we are fine.

Re: [PATCH 2/9] block: optionally mark pages dirty in bio_release_pages

[Minwoo Im]

On 19-06-27 10:27:59, Christoph Hellwig wrote:
> On Thu, Jun 27, 2019 at 05:52:47AM +0900, Minwoo Im wrote:
> > Could you please explain a bit why we should not make it dirty in case
> > of compound page?
> 
> PageCompount is only true for the tail pages, and marking them dirty
> doesn't work.  On the other hand marking the head page dirty covers
> the whole compount, so we are fine.

Christoph,

Thanks for letting me know this.

[PATCH 3/9] block: use bio_release_pages in bio_unmap_user

[Christoph Hellwig]

Use bio_release_pages instead of open coding it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 7f3920b6baca..20a16347bcbb 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1437,24 +1437,6 @@ struct bio *bio_map_user_iov(struct request_queue *q,
 	return ERR_PTR(ret);
 }
 
-static void __bio_unmap_user(struct bio *bio)
-{
-	struct bio_vec *bvec;
-	struct bvec_iter_all iter_all;
-
-	/*
-	 * make sure we dirty pages we wrote to
-	 */
-	bio_for_each_segment_all(bvec, bio, iter_all) {
-		if (bio_data_dir(bio) == READ)
-			set_page_dirty_lock(bvec->bv_page);
-
-		put_page(bvec->bv_page);
-	}
-
-	bio_put(bio);
-}
-
 /**
  *	bio_unmap_user	-	unmap a bio
  *	@bio:		the bio being unmapped
@@ -1466,7 +1448,8 @@ static void __bio_unmap_user(struct bio *bio)
  */
 void bio_unmap_user(struct bio *bio)
 {
-	__bio_unmap_user(bio);
+	bio_release_pages(bio, bio_data_dir(bio) == READ);
+	bio_put(bio);
 	bio_put(bio);
 }
 
-- 
2.20.1

Re: [PATCH 3/9] block: use bio_release_pages in bio_unmap_user

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On 06/26/2019 06:49 AM, Christoph Hellwig wrote:
> Use bio_release_pages instead of open coding it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/bio.c | 21 ++-------------------
>   1 file changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 7f3920b6baca..20a16347bcbb 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1437,24 +1437,6 @@ struct bio *bio_map_user_iov(struct request_queue *q,
>   	return ERR_PTR(ret);
>   }
>
> -static void __bio_unmap_user(struct bio *bio)
> -{
> -	struct bio_vec *bvec;
> -	struct bvec_iter_all iter_all;
> -
> -	/*
> -	 * make sure we dirty pages we wrote to
> -	 */
> -	bio_for_each_segment_all(bvec, bio, iter_all) {
> -		if (bio_data_dir(bio) == READ)
> -			set_page_dirty_lock(bvec->bv_page);
> -
> -		put_page(bvec->bv_page);
> -	}
> -
> -	bio_put(bio);
> -}
> -
>   /**
>    *	bio_unmap_user	-	unmap a bio
>    *	@bio:		the bio being unmapped
> @@ -1466,7 +1448,8 @@ static void __bio_unmap_user(struct bio *bio)
>    */
>   void bio_unmap_user(struct bio *bio)
>   {
> -	__bio_unmap_user(bio);
> +	bio_release_pages(bio, bio_data_dir(bio) == READ);
> +	bio_put(bio);
>   	bio_put(bio);
>   }
>
>

[PATCH 4/9] block: use bio_release_pages in bio_map_user_iov

[Christoph Hellwig]

Use bio_release_pages instead of open coding it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 20a16347bcbb..a96d33d2de44 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1362,8 +1362,6 @@ struct bio *bio_map_user_iov(struct request_queue *q,
 	int j;
 	struct bio *bio;
 	int ret;
-	struct bio_vec *bvec;
-	struct bvec_iter_all iter_all;
 
 	if (!iov_iter_count(iter))
 		return ERR_PTR(-EINVAL);
@@ -1430,9 +1428,7 @@ struct bio *bio_map_user_iov(struct request_queue *q,
 	return bio;
 
  out_unmap:
-	bio_for_each_segment_all(bvec, bio, iter_all) {
-		put_page(bvec->bv_page);
-	}
+	bio_release_pages(bio, false);
 	bio_put(bio);
 	return ERR_PTR(ret);
 }
-- 
2.20.1

Re: [PATCH 4/9] block: use bio_release_pages in bio_map_user_iov

[Minwoo Im]

This looks good to me.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH 4/9] block: use bio_release_pages in bio_map_user_iov

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 06/26/2019 06:49 AM, Christoph Hellwig wrote:
> Use bio_release_pages instead of open coding it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/bio.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 20a16347bcbb..a96d33d2de44 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1362,8 +1362,6 @@ struct bio *bio_map_user_iov(struct request_queue *q,
>   	int j;
>   	struct bio *bio;
>   	int ret;
> -	struct bio_vec *bvec;
> -	struct bvec_iter_all iter_all;
>
>   	if (!iov_iter_count(iter))
>   		return ERR_PTR(-EINVAL);
> @@ -1430,9 +1428,7 @@ struct bio *bio_map_user_iov(struct request_queue *q,
>   	return bio;
>
>    out_unmap:
> -	bio_for_each_segment_all(bvec, bio, iter_all) {
> -		put_page(bvec->bv_page);
> -	}
> +	bio_release_pages(bio, false);
>   	bio_put(bio);
>   	return ERR_PTR(ret);
>   }
>

[PATCH 5/9] iomap: use bio_release_pages in iomap_dio_bio_end_io

[Christoph Hellwig]

Use bio_release_pages instead of duplicating it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index 23ef63fd1669..3798eaf789d7 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -1595,13 +1595,7 @@ static void iomap_dio_bio_end_io(struct bio *bio)
 	if (should_dirty) {
 		bio_check_pages_dirty(bio);
 	} else {
-		if (!bio_flagged(bio, BIO_NO_PAGE_REF)) {
-			struct bvec_iter_all iter_all;
-			struct bio_vec *bvec;
-
-			bio_for_each_segment_all(bvec, bio, iter_all)
-				put_page(bvec->bv_page);
-		}
+		bio_release_pages(bio, false);
 		bio_put(bio);
 	}
 }
-- 
2.20.1

Re: [PATCH 5/9] iomap: use bio_release_pages in iomap_dio_bio_end_io

[Minwoo Im]

This looks good to me.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH 5/9] iomap: use bio_release_pages in iomap_dio_bio_end_io

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 06/26/2019 06:50 AM, Christoph Hellwig wrote:
> Use bio_release_pages instead of duplicating it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/iomap.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 23ef63fd1669..3798eaf789d7 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -1595,13 +1595,7 @@ static void iomap_dio_bio_end_io(struct bio *bio)
>   	if (should_dirty) {
>   		bio_check_pages_dirty(bio);
>   	} else {
> -		if (!bio_flagged(bio, BIO_NO_PAGE_REF)) {
> -			struct bvec_iter_all iter_all;
> -			struct bio_vec *bvec;
> -
> -			bio_for_each_segment_all(bvec, bio, iter_all)
> -				put_page(bvec->bv_page);
> -		}
> +		bio_release_pages(bio, false);
>   		bio_put(bio);
>   	}
>   }
>

[PATCH 6/9] block_dev: use bio_release_pages in blkdev_bio_end_io

[Christoph Hellwig]

Use bio_release_pages instead of duplicating it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/block_dev.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 749f5984425d..a6572a811880 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -335,13 +335,7 @@ static void blkdev_bio_end_io(struct bio *bio)
 	if (should_dirty) {
 		bio_check_pages_dirty(bio);
 	} else {
-		if (!bio_flagged(bio, BIO_NO_PAGE_REF)) {
-			struct bvec_iter_all iter_all;
-			struct bio_vec *bvec;
-
-			bio_for_each_segment_all(bvec, bio, iter_all)
-				put_page(bvec->bv_page);
-		}
+		bio_release_pages(bio, false);
 		bio_put(bio);
 	}
 }
-- 
2.20.1

Re: [PATCH 6/9] block_dev: use bio_release_pages in blkdev_bio_end_io

[Minwoo Im]

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: [PATCH 6/9] block_dev: use bio_release_pages in blkdev_bio_end_io

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 06/26/2019 06:49 AM, Christoph Hellwig wrote:
> Use bio_release_pages instead of duplicating it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/block_dev.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 749f5984425d..a6572a811880 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -335,13 +335,7 @@ static void blkdev_bio_end_io(struct bio *bio)
>   	if (should_dirty) {
>   		bio_check_pages_dirty(bio);
>   	} else {
> -		if (!bio_flagged(bio, BIO_NO_PAGE_REF)) {
> -			struct bvec_iter_all iter_all;
> -			struct bio_vec *bvec;
> -
> -			bio_for_each_segment_all(bvec, bio, iter_all)
> -				put_page(bvec->bv_page);
> -		}
> +		bio_release_pages(bio, false);
>   		bio_put(bio);
>   	}
>   }
>

[PATCH 7/9] block_dev: use bio_release_pages in bio_unmap_user

[Christoph Hellwig]

Use bio_release_pages instead of duplicating it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/block_dev.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index a6572a811880..f00b569a9f89 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -203,13 +203,12 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 {
 	struct file *file = iocb->ki_filp;
 	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
-	struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs, *bvec;
+	struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs;
 	loff_t pos = iocb->ki_pos;
 	bool should_dirty = false;
 	struct bio bio;
 	ssize_t ret;
 	blk_qc_t qc;
-	struct bvec_iter_all iter_all;
 
 	if ((pos | iov_iter_alignment(iter)) &
 	    (bdev_logical_block_size(bdev) - 1))
@@ -259,13 +258,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 	}
 	__set_current_state(TASK_RUNNING);
 
-	bio_for_each_segment_all(bvec, &bio, iter_all) {
-		if (should_dirty && !PageCompound(bvec->bv_page))
-			set_page_dirty_lock(bvec->bv_page);
-		if (!bio_flagged(&bio, BIO_NO_PAGE_REF))
-			put_page(bvec->bv_page);
-	}
-
+	bio_release_pages(&bio, should_dirty);
 	if (unlikely(bio.bi_status))
 		ret = blk_status_to_errno(bio.bi_status);
 
-- 
2.20.1

Re: [PATCH 7/9] block_dev: use bio_release_pages in bio_unmap_user

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 06/26/2019 06:50 AM, Christoph Hellwig wrote:
> Use bio_release_pages instead of duplicating it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/block_dev.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index a6572a811880..f00b569a9f89 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -203,13 +203,12 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>   {
>   	struct file *file = iocb->ki_filp;
>   	struct block_device *bdev = I_BDEV(bdev_file_inode(file));
> -	struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs, *bvec;
> +	struct bio_vec inline_vecs[DIO_INLINE_BIO_VECS], *vecs;
>   	loff_t pos = iocb->ki_pos;
>   	bool should_dirty = false;
>   	struct bio bio;
>   	ssize_t ret;
>   	blk_qc_t qc;
> -	struct bvec_iter_all iter_all;
>
>   	if ((pos | iov_iter_alignment(iter)) &
>   	    (bdev_logical_block_size(bdev) - 1))
> @@ -259,13 +258,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>   	}
>   	__set_current_state(TASK_RUNNING);
>
> -	bio_for_each_segment_all(bvec, &bio, iter_all) {
> -		if (should_dirty && !PageCompound(bvec->bv_page))
> -			set_page_dirty_lock(bvec->bv_page);
> -		if (!bio_flagged(&bio, BIO_NO_PAGE_REF))
> -			put_page(bvec->bv_page);
> -	}
> -
> +	bio_release_pages(&bio, should_dirty);
>   	if (unlikely(bio.bi_status))
>   		ret = blk_status_to_errno(bio.bi_status);
>
>

[PATCH 8/9] direct-io: use bio_release_pages in dio_bio_complete

[Christoph Hellwig]

Use bio_release_pages instead of duplicating it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/direct-io.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index ac7fb19b6ade..ae196784f487 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -538,8 +538,8 @@ static struct bio *dio_await_one(struct dio *dio)
  */
 static blk_status_t dio_bio_complete(struct dio *dio, struct bio *bio)
 {
-	struct bio_vec *bvec;
 	blk_status_t err = bio->bi_status;
+	bool should_dirty = dio->op == REQ_OP_READ && dio->should_dirty;
 
 	if (err) {
 		if (err == BLK_STS_AGAIN && (bio->bi_opf & REQ_NOWAIT))
@@ -548,19 +548,10 @@ static blk_status_t dio_bio_complete(struct dio *dio, struct bio *bio)
 			dio->io_error = -EIO;
 	}
 
-	if (dio->is_async && dio->op == REQ_OP_READ && dio->should_dirty) {
+	if (dio->is_async && should_dirty) {
 		bio_check_pages_dirty(bio);	/* transfers ownership */
 	} else {
-		struct bvec_iter_all iter_all;
-
-		bio_for_each_segment_all(bvec, bio, iter_all) {
-			struct page *page = bvec->bv_page;
-
-			if (dio->op == REQ_OP_READ && !PageCompound(page) &&
-					dio->should_dirty)
-				set_page_dirty_lock(page);
-			put_page(page);
-		}
+		bio_release_pages(bio, should_dirty);
 		bio_put(bio);
 	}
 	return err;
-- 
2.20.1

Re: [PATCH 8/9] direct-io: use bio_release_pages in dio_bio_complete

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 06/26/2019 06:49 AM, Christoph Hellwig wrote:
> Use bio_release_pages instead of duplicating it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/direct-io.c | 15 +++------------
>   1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index ac7fb19b6ade..ae196784f487 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -538,8 +538,8 @@ static struct bio *dio_await_one(struct dio *dio)
>    */
>   static blk_status_t dio_bio_complete(struct dio *dio, struct bio *bio)
>   {
> -	struct bio_vec *bvec;
>   	blk_status_t err = bio->bi_status;
> +	bool should_dirty = dio->op == REQ_OP_READ && dio->should_dirty;
>
>   	if (err) {
>   		if (err == BLK_STS_AGAIN && (bio->bi_opf & REQ_NOWAIT))
> @@ -548,19 +548,10 @@ static blk_status_t dio_bio_complete(struct dio *dio, struct bio *bio)
>   			dio->io_error = -EIO;
>   	}
>
> -	if (dio->is_async && dio->op == REQ_OP_READ && dio->should_dirty) {
> +	if (dio->is_async && should_dirty) {
>   		bio_check_pages_dirty(bio);	/* transfers ownership */
>   	} else {
> -		struct bvec_iter_all iter_all;
> -
> -		bio_for_each_segment_all(bvec, bio, iter_all) {
> -			struct page *page = bvec->bv_page;
> -
> -			if (dio->op == REQ_OP_READ && !PageCompound(page) &&
> -					dio->should_dirty)
> -				set_page_dirty_lock(page);
> -			put_page(page);
> -		}
> +		bio_release_pages(bio, should_dirty);
>   		bio_put(bio);
>   	}
>   	return err;
>

[PATCH 9/9] block: never take page references for ITER_BVEC

[Christoph Hellwig]

If we pass pages through an iov_iter we always already have a reference
in the caller.  Thus remove the ITER_BVEC_FLAG_NO_REF and don't take
reference to pages by default for bvec backed iov_iters.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 block/bio.c          | 14 +-------------
 drivers/block/loop.c | 16 ++++------------
 fs/io_uring.c        |  3 ---
 include/linux/uio.h  | 10 +---------
 4 files changed, 6 insertions(+), 37 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index a96d33d2de44..3c9225caff8d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -836,15 +836,6 @@ int bio_add_page(struct bio *bio, struct page *page,
 }
 EXPORT_SYMBOL(bio_add_page);
 
-static void bio_get_pages(struct bio *bio)
-{
-	struct bvec_iter_all iter_all;
-	struct bio_vec *bvec;
-
-	bio_for_each_segment_all(bvec, bio, iter_all)
-		get_page(bvec->bv_page);
-}
-
 void bio_release_pages(struct bio *bio, bool mark_dirty)
 {
 	struct bvec_iter_all iter_all;
@@ -960,11 +951,8 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 			ret = __bio_iov_iter_get_pages(bio, iter);
 	} while (!ret && iov_iter_count(iter) && !bio_full(bio));
 
-	if (iov_iter_bvec_no_ref(iter))
+	if (is_bvec)
 		bio_set_flag(bio, BIO_NO_PAGE_REF);
-	else if (is_bvec)
-		bio_get_pages(bio);
-
 	return bio->bi_vcnt ? 0 : ret;
 }
 
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f11b7dc16e9d..44c9985f352a 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -264,20 +264,12 @@ lo_do_transfer(struct loop_device *lo, int cmd,
 	return ret;
 }
 
-static inline void loop_iov_iter_bvec(struct iov_iter *i,
-		unsigned int direction, const struct bio_vec *bvec,
-		unsigned long nr_segs, size_t count)
-{
-	iov_iter_bvec(i, direction, bvec, nr_segs, count);
-	i->type |= ITER_BVEC_FLAG_NO_REF;
-}
-
 static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos)
 {
 	struct iov_iter i;
 	ssize_t bw;
 
-	loop_iov_iter_bvec(&i, WRITE, bvec, 1, bvec->bv_len);
+	iov_iter_bvec(&i, WRITE, bvec, 1, bvec->bv_len);
 
 	file_start_write(file);
 	bw = vfs_iter_write(file, &i, ppos, 0);
@@ -355,7 +347,7 @@ static int lo_read_simple(struct loop_device *lo, struct request *rq,
 	ssize_t len;
 
 	rq_for_each_segment(bvec, rq, iter) {
-		loop_iov_iter_bvec(&i, READ, &bvec, 1, bvec.bv_len);
+		iov_iter_bvec(&i, READ, &bvec, 1, bvec.bv_len);
 		len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0);
 		if (len < 0)
 			return len;
@@ -396,7 +388,7 @@ static int lo_read_transfer(struct loop_device *lo, struct request *rq,
 		b.bv_offset = 0;
 		b.bv_len = bvec.bv_len;
 
-		loop_iov_iter_bvec(&i, READ, &b, 1, b.bv_len);
+		iov_iter_bvec(&i, READ, &b, 1, b.bv_len);
 		len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0);
 		if (len < 0) {
 			ret = len;
@@ -563,7 +555,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
 	}
 	atomic_set(&cmd->ref, 2);
 
-	loop_iov_iter_bvec(&iter, rw, bvec, nr_bvec, blk_rq_bytes(rq));
+	iov_iter_bvec(&iter, rw, bvec, nr_bvec, blk_rq_bytes(rq));
 	iter.iov_offset = offset;
 
 	cmd->iocb.ki_pos = pos;
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 86a2bd721900..eb6ab1507913 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -997,9 +997,6 @@ static int io_import_fixed(struct io_ring_ctx *ctx, int rw,
 	iov_iter_bvec(iter, rw, imu->bvec, imu->nr_bvecs, offset + len);
 	if (offset)
 		iov_iter_advance(iter, offset);
-
-	/* don't drop a reference to these pages */
-	iter->type |= ITER_BVEC_FLAG_NO_REF;
 	return 0;
 }
 
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 2c90a0842ee8..cea1761c5672 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -19,9 +19,6 @@ struct kvec {
 };
 
 enum iter_type {
-	/* set if ITER_BVEC doesn't hold a bv_page ref */
-	ITER_BVEC_FLAG_NO_REF = 2,
-
 	/* iter types */
 	ITER_IOVEC = 4,
 	ITER_KVEC = 8,
@@ -56,7 +53,7 @@ struct iov_iter {
 
 static inline enum iter_type iov_iter_type(const struct iov_iter *i)
 {
-	return i->type & ~(READ | WRITE | ITER_BVEC_FLAG_NO_REF);
+	return i->type & ~(READ | WRITE);
 }
 
 static inline bool iter_is_iovec(const struct iov_iter *i)
@@ -89,11 +86,6 @@ static inline unsigned char iov_iter_rw(const struct iov_iter *i)
 	return i->type & (READ | WRITE);
 }
 
-static inline bool iov_iter_bvec_no_ref(const struct iov_iter *i)
-{
-	return (i->type & ITER_BVEC_FLAG_NO_REF) != 0;
-}
-
 /*
  * Total number of bytes covered by an iovec.
  *
-- 
2.20.1

Re: [PATCH 9/9] block: never take page references for ITER_BVEC

[Minwoo Im]

On 19-06-26 15:49:28, Christoph Hellwig wrote:
> If we pass pages through an iov_iter we always already have a reference
> in the caller.  Thus remove the ITER_BVEC_FLAG_NO_REF and don't take
> reference to pages by default for bvec backed iov_iters.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

This looks good to me.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Re: cleanup bio page releasing

[Jens Axboe]

On 6/26/19 7:49 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series cleans up the various direct I/O and pass through
> routines by switching them over to common bio helpers.
> 
> The last patch just unconditionally applies the no page ref
> behavior for bvec iters.  I looked at all the callers, and
> there is none that drops the pre-required references before
> completing the request.

Nice cleanup, applied.

-- 
Jens Axboe