[PATCH 07/10] bcache: comment on direct access to bvec table

[PATCH 07/10] bcache: comment on direct access to bvec table

[Ming Lei]

All direct access to bvec table are safe even after multipage bvec is supported.

Cc: linux-bcache@vger.kernel.org
Acked-by: Coly Li <colyli@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/bcache/btree.c | 1 +
 drivers/md/bcache/util.c  | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 11c5503d31dc..c09f3dd4bf07 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -432,6 +432,7 @@ static void do_btree_node_write(struct btree *b)
 
 		continue_at(cl, btree_node_write_done, NULL);
 	} else {
+		/* No problem for multipage bvec since the bio is just allocated */
 		b->bio->bi_vcnt = 0;
 		bch_bio_map(b->bio, i);
 
diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index e548b8b51322..61813d230015 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -249,6 +249,13 @@ uint64_t bch_next_delay(struct bch_ratelimit *d, uint64_t done)
 		: 0;
 }
 
+/*
+ * Generally it isn't good to access .bi_io_vec and .bi_vcnt directly,
+ * the preferred way is bio_add_page, but in this case, bch_bio_map()
+ * supposes that the bvec table is empty, so it is safe to access
+ * .bi_vcnt & .bi_io_vec in this way even after multipage bvec is
+ * supported.
+ */
 void bch_bio_map(struct bio *bio, void *base)
 {
 	size_t size = bio->bi_iter.bi_size;
-- 
2.9.5

Re: [PATCH 08/10] block: move bio_alloc_pages() to bcache

[Michael Lyle]

On 12/08/2017 05:14 AM, Ming Lei wrote:
> bcache is the only user of bio_alloc_pages(), and all users should use
> bio_add_page() instead, so move this function into bcache, and avoid
> it misused in future.

Can things like this -please- be sent to the bcache list and bcache
maintainers?  I'm preparing my patch set for Jens and I'm surprised by
merge conflicts from stuff in my queue.  (Just showed up in next to show
the conflict in the past couple of days).

Mike

> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/bio.c              | 28 ----------------------------
>  drivers/md/bcache/util.c | 27 +++++++++++++++++++++++++++
>  drivers/md/bcache/util.h |  1 +
>  include/linux/bio.h      |  1 -
>  4 files changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 228229f3bb76..76bb3dafffea 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -969,34 +969,6 @@ void bio_advance(struct bio *bio, unsigned bytes)
>  EXPORT_SYMBOL(bio_advance);
>  
>  /**
> - * bio_alloc_pages - allocates a single page for each bvec in a bio
> - * @bio: bio to allocate pages for
> - * @gfp_mask: flags for allocation
> - *
> - * Allocates pages up to @bio->bi_vcnt.
> - *
> - * Returns 0 on success, -ENOMEM on failure. On failure, any allocated pages are
> - * freed.
> - */
> -int bio_alloc_pages(struct bio *bio, gfp_t gfp_mask)
> -{
> -	int i;
> -	struct bio_vec *bv;
> -
> -	bio_for_each_segment_all(bv, bio, i) {
> -		bv->bv_page = alloc_page(gfp_mask);
> -		if (!bv->bv_page) {
> -			while (--bv >= bio->bi_io_vec)
> -				__free_page(bv->bv_page);
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(bio_alloc_pages);
> -
> -/**
>   * bio_copy_data - copy contents of data buffers from one chain of bios to
>   * another
>   * @src: source bio list
> diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
> index 61813d230015..ac557e8c7ef5 100644
> --- a/drivers/md/bcache/util.c
> +++ b/drivers/md/bcache/util.c
> @@ -283,6 +283,33 @@ start:		bv->bv_len	= min_t(size_t, PAGE_SIZE - bv->bv_offset,
>  	}
>  }
>  
> +/**
> + * bio_alloc_pages - allocates a single page for each bvec in a bio
> + * @bio: bio to allocate pages for
> + * @gfp_mask: flags for allocation
> + *
> + * Allocates pages up to @bio->bi_vcnt.
> + *
> + * Returns 0 on success, -ENOMEM on failure. On failure, any allocated pages are
> + * freed.
> + */
> +int bio_alloc_pages(struct bio *bio, gfp_t gfp_mask)
> +{
> +	int i;
> +	struct bio_vec *bv;
> +
> +	bio_for_each_segment_all(bv, bio, i) {
> +		bv->bv_page = alloc_page(gfp_mask);
> +		if (!bv->bv_page) {
> +			while (--bv >= bio->bi_io_vec)
> +				__free_page(bv->bv_page);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Portions Copyright (c) 1996-2001, PostgreSQL Global Development Group (Any
>   * use permitted, subject to terms of PostgreSQL license; see.)
> diff --git a/drivers/md/bcache/util.h b/drivers/md/bcache/util.h
> index ed5e8a412eb8..c92de937bcab 100644
> --- a/drivers/md/bcache/util.h
> +++ b/drivers/md/bcache/util.h
> @@ -558,6 +558,7 @@ static inline unsigned fract_exp_two(unsigned x, unsigned fract_bits)
>  }
>  
>  void bch_bio_map(struct bio *bio, void *base);
> +int bio_alloc_pages(struct bio *bio, gfp_t gfp_mask);
>  
>  static inline sector_t bdev_sectors(struct block_device *bdev)
>  {
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 3f314e17364a..46cdbe0335a5 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -501,7 +501,6 @@ static inline void bio_flush_dcache_pages(struct bio *bi)
>  #endif
>  
>  extern void bio_copy_data(struct bio *dst, struct bio *src);
> -extern int bio_alloc_pages(struct bio *bio, gfp_t gfp);
>  extern void bio_free_pages(struct bio *bio);
>  
>  extern struct bio *bio_copy_user_iov(struct request_queue *,
> 

Re: [PATCH 08/10] block: move bio_alloc_pages() to bcache

[Ming Lei]

On Mon, Jan 08, 2018 at 10:05:09AM -0800, Michael Lyle wrote:
> On 12/08/2017 05:14 AM, Ming Lei wrote:
> > bcache is the only user of bio_alloc_pages(), and all users should use
> > bio_add_page() instead, so move this function into bcache, and avoid
> > it misused in future.
> 
> Can things like this -please- be sent to the bcache list and bcache
> maintainers?  I'm preparing my patch set for Jens and I'm surprised by
> merge conflicts from stuff in my queue.  (Just showed up in next to show
> the conflict in the past couple of days).

OK, will do next time.

-- 
Ming