Re: [PATCH 4/5] block: change how we get page references in bio_iov_iter_get_pages

[Ming Lei <ming.lei@redhat.com>]

On Thu, Apr 11, 2019 at 08:23:30AM +0200, Christoph Hellwig wrote:
> Instead of needing a special macro to iterate over all pages in
> a bvec just do a second passs over the whole bio.  This also matches
> what we do on the release side.  The release side helper is moved
> up to where we need the get helper to clearly express the symmetry.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bio.c          | 51 ++++++++++++++++++++++----------------------
>  include/linux/bvec.h |  5 -----
>  2 files changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index c2a389b1509a..d3490aeb1a7e 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -861,6 +861,26 @@ 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;
> +	int i;
> +
> +	bio_for_each_segment_all(bvec, bio, i, iter_all)
> +		get_page(bvec->bv_page);
> +}
> +
> +static void bio_release_pages(struct bio *bio)
> +{
> +	struct bvec_iter_all iter_all;
> +	struct bio_vec *bvec;
> +	int i;
> +
> +	bio_for_each_segment_all(bvec, bio, i, iter_all)
> +		put_page(bvec->bv_page);
> +}
> +
>  static int __bio_iov_bvec_add_pages(struct bio *bio, struct iov_iter *iter)
>  {
>  	const struct bio_vec *bv = iter->bvec;
> @@ -875,15 +895,6 @@ static int __bio_iov_bvec_add_pages(struct bio *bio, struct iov_iter *iter)
>  				bv->bv_offset + iter->iov_offset);
>  	if (unlikely(size != len))
>  		return -EINVAL;
> -
> -	if (!bio_flagged(bio, BIO_NO_PAGE_REF)) {
> -		struct page *page;
> -		int i;
> -
> -		mp_bvec_for_each_page(page, bv, i)
> -			get_page(page);
> -	}
> -
>  	iov_iter_advance(iter, size);
>  	return 0;
>  }
> @@ -963,13 +974,6 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  	if (WARN_ON_ONCE(bio->bi_vcnt))
>  		return -EINVAL;
>  
> -	/*
> -	 * If this is a BVEC iter, then the pages are kernel pages. Don't
> -	 * release them on IO completion, if the caller asked us to.
> -	 */
> -	if (is_bvec && iov_iter_bvec_no_ref(iter))
> -		bio_set_flag(bio, BIO_NO_PAGE_REF);
> -
>  	do {
>  		if (is_bvec)
>  			ret = __bio_iov_bvec_add_pages(bio, iter);
> @@ -977,6 +981,11 @@ 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))
> +		bio_set_flag(bio, BIO_NO_PAGE_REF);
> +	else
> +		bio_get_pages(bio);
> +
>  	return bio->bi_vcnt ? 0 : ret;
>  }
>  
> @@ -1670,16 +1679,6 @@ void bio_set_pages_dirty(struct bio *bio)
>  	}
>  }
>  
> -static void bio_release_pages(struct bio *bio)
> -{
> -	struct bio_vec *bvec;
> -	int i;
> -	struct bvec_iter_all iter_all;
> -
> -	bio_for_each_segment_all(bvec, bio, i, iter_all)
> -		put_page(bvec->bv_page);
> -}
> -
>  /*
>   * bio_check_pages_dirty() will check that all the BIO's pages are still dirty.
>   * If they are, then fine.  If, however, some pages are clean then they must
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index f6275c4da13a..307bbda62b7b 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -189,9 +189,4 @@ static inline void mp_bvec_last_segment(const struct bio_vec *bvec,
>  	}
>  }
>  
> -#define mp_bvec_for_each_page(pg, bv, i)				\
> -	for (i = (bv)->bv_offset / PAGE_SIZE;				\
> -		(i <= (((bv)->bv_offset + (bv)->bv_len - 1) / PAGE_SIZE)) && \
> -		(pg = bvec_nth_page((bv)->bv_page, i)); i += 1)
> -
>  #endif /* __LINUX_BVEC_ITER_H */
> -- 
> 2.20.1
> 

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming