Re: [PATCH v3 07/49] bcache: comment on direct access to bvec table

[Ming Lei]

On Thu, Aug 10, 2017 at 04:26:03AM -0700, Christoph Hellwig wrote:
> I think all this bcache code needs bigger attention.  For one
> bio_alloc_pages is only used in bcache, so we should move it in there.

Looks a good idea.

> 
> Second the way  bio_alloc_pages is currently written looks potentially
> dangerous for multi-page biovecs, so we should think about a better
> calling convention.  The way bcache seems to generally use it is by
> allocating a bio, then calling bch_bio_map on it and then calling
> bio_alloc_pages.  I think it just needs a new bio_alloc_pages calling
> convention that passes the size to be allocated and stop looking into
> the segment count.

Looks a good idea, will try to do in this way.

> 
> Second bch_bio_map isn't something we should be doing in a driver,
> it should be rewritten using bio_add_page.

Yes, the idea way is to use bio_add_page always, but given
bch_bio_map() is used on a fresh bio, it is safe, and this
work can be done in another bcache cleanup patch.

> 
> > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > index 866dcf78ff8e..3da595ae565b 100644
> > --- a/drivers/md/bcache/btree.c
> > +++ b/drivers/md/bcache/btree.c
> > @@ -431,6 +431,7 @@ static void do_btree_node_write(struct btree *b)
> >  
> > continue_at(cl, btree_node_write_done, NULL);
> > } else {
> > +   /* No harm for multipage bvec since the new is just allocated */
> > b->bio->bi_vcnt = 0;
> 
> This should go away - bio_alloc_pages or it's replacement should not
> modify bi_vcnt on failure.

OK.

> 
> > +   /* single page bio, safe for multipage bvec */
> > dc->sb_bio.bi_io_vec[0].bv_page = sb_page;
> 
> needs to use bio_add_page.

OK.

> 
> > +   /* single page bio, safe for multipage bvec */
> > ca->sb_bio.bi_io_vec[0].bv_page = sb_page;
> 
> needs to use bio_add_page.

OK.

-- 
Ming

Re: [PATCH v3 07/49] bcache: comment on direct access to bvec table

[Christoph Hellwig]

I think all this bcache code needs bigger attention.  For one
bio_alloc_pages is only used in bcache, so we should move it in there.

Second the way  bio_alloc_pages is currently written looks potentially
dangerous for multi-page biovecs, so we should think about a better
calling convention.  The way bcache seems to generally use it is by
allocating a bio, then calling bch_bio_map on it and then calling
bio_alloc_pages.  I think it just needs a new bio_alloc_pages calling
convention that passes the size to be allocated and stop looking into
the segment count.

Second bch_bio_map isn't something we should be doing in a driver,
it should be rewritten using bio_add_page.

> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 866dcf78ff8e..3da595ae565b 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -431,6 +431,7 @@ static void do_btree_node_write(struct btree *b)
>  
>   continue_at(cl, btree_node_write_done, NULL);
>   } else {
> + /* No harm for multipage bvec since the new is just allocated */
>   b->bio->bi_vcnt = 0;

This should go away - bio_alloc_pages or it's replacement should not
modify bi_vcnt on failure.

> + /* single page bio, safe for multipage bvec */
>   dc->sb_bio.bi_io_vec[0].bv_page = sb_page;

needs to use bio_add_page.

> + /* single page bio, safe for multipage bvec */
>   ca->sb_bio.bi_io_vec[0].bv_page = sb_page;

needs to use bio_add_page.

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

[Ming Lei]

Looks all are safe after multipage bvec is supported.

Cc: linux-bca...@vger.kernel.org
Signed-off-by: Ming Lei 
---
 drivers/md/bcache/btree.c | 1 +
 drivers/md/bcache/super.c | 6 ++
 drivers/md/bcache/util.c  | 7 +++
 3 files changed, 14 insertions(+)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 866dcf78ff8e..3da595ae565b 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -431,6 +431,7 @@ static void do_btree_node_write(struct btree *b)
 
continue_at(cl, btree_node_write_done, NULL);
} else {
+   /* No harm for multipage bvec since the new is just allocated */
b->bio->bi_vcnt = 0;
bch_bio_map(b->bio, i);
 
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 8352fad765f6..6808f548cd13 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -208,6 +208,7 @@ static void write_bdev_super_endio(struct bio *bio)
 
 static void __write_super(struct cache_sb *sb, struct bio *bio)
 {
+   /* single page bio, safe for multipage bvec */
struct cache_sb *out = page_address(bio->bi_io_vec[0].bv_page);
unsigned i;
 
@@ -1154,6 +1155,8 @@ static void register_bdev(struct cache_sb *sb, struct 
page *sb_page,
dc->bdev->bd_holder = dc;
 
bio_init(>sb_bio, dc->sb_bio.bi_inline_vecs, 1);
+
+   /* single page bio, safe for multipage bvec */
dc->sb_bio.bi_io_vec[0].bv_page = sb_page;
get_page(sb_page);
 
@@ -1799,6 +1802,7 @@ void bch_cache_release(struct kobject *kobj)
for (i = 0; i < RESERVE_NR; i++)
free_fifo(>free[i]);
 
+   /* single page bio, safe for multipage bvec */
if (ca->sb_bio.bi_inline_vecs[0].bv_page)
put_page(ca->sb_bio.bi_io_vec[0].bv_page);
 
@@ -1854,6 +1858,8 @@ static int register_cache(struct cache_sb *sb, struct 
page *sb_page,
ca->bdev->bd_holder = ca;
 
bio_init(>sb_bio, ca->sb_bio.bi_inline_vecs, 1);
+
+   /* single page bio, safe for multipage bvec */
ca->sb_bio.bi_io_vec[0].bv_page = sb_page;
get_page(sb_page);
 
diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index 8c3a938f4bf0..11b4230ea6ad 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c
@@ -223,6 +223,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.4