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

[Minwoo Im <minwoo.im.dev@gmail.com>]

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.