[2/9] block: optionally mark pages dirty in bio_release_pages
diff mbox series

Message ID 20190626134928.7988-3-hch@lst.de
State New
Headers show
Series
  • [1/9] block: move the BIO_NO_PAGE_REF check into bio_release_pages
Related show

Commit Message

Christoph Hellwig June 26, 2019, 1:49 p.m. UTC
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(-)

Comments

Minwoo Im June 26, 2019, 8:52 p.m. UTC | #1
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?
Chaitanya Kulkarni June 27, 2019, 12:21 a.m. UTC | #2
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.
Christoph Hellwig June 27, 2019, 8:27 a.m. UTC | #3
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.
Minwoo Im June 27, 2019, 10:22 a.m. UTC | #4
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.
Minwoo Im June 27, 2019, 10:24 a.m. UTC | #5
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.

Patch
diff mbox series

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