Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1511591 > unrolled thread

[PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair Ming Lei <tom.leiming@gmail.com> - 2016-10-29 10:30 +0200
    Re: [PATCH 45/60] block: bio: introduce  bio_for_each_segment_all_rd() and its write pair Theodore Ts'o <tytso@mit.edu> - 2016-10-31 15:10 +0100
      Re: [PATCH 45/60] block: bio: introduce  bio_for_each_segment_all_rd() and its write pair Christoph Hellwig <hch@infradead.org> - 2016-10-31 16:20 +0100
        Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd()  and its write pair Ming Lei <tom.leiming@gmail.com> - 2016-11-01 00:00 +0100
        Re: [PATCH 45/60] block: bio: introduce  bio_for_each_segment_all_rd() and its write pair Kent Overstreet <kent.overstreet@gmail.com> - 2016-11-02 04:10 +0100
      Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd()  and its write pair Ming Lei <tom.leiming@gmail.com> - 2016-10-31 23:50 +0100
        Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd()  and its write pair Ming Lei <tom.leiming@gmail.com> - 2016-11-01 01:00 +0100
          Re: [PATCH 45/60] block: bio: introduce  bio_for_each_segment_all_rd() and its write pair Theodore Ts'o <tytso@mit.edu> - 2016-11-01 15:20 +0100
            Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd()  and its write pair Ming Lei <tom.leiming@gmail.com> - 2016-11-02 03:00 +0100

#1511591 — [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

This patches introduce bio_for_each_segment_all_rd() and
bio_for_each_segment_all_wt().

bio_for_each_segment_all_rd() is for replacing
bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
is accessed as readonly.

bio_for_each_segment_all_wt() is for replacing
bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
need to be updated.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 include/linux/bio.h       | 15 +++++++++++++++
 include/linux/blk_types.h |  6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index ec1c0f2aaa19..f8a025ffaa9c 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -215,6 +215,21 @@ static inline void bio_advance_iter_mp(struct bio *bio, struct bvec_iter *iter,
 #define bio_for_each_segment_mp(bvl, bio, iter)				\
 	__bio_for_each_segment_mp(bvl, bio, iter, (bio)->bi_iter)
 
+/* the bio has to be singlepage bvecs based */
+#define bio_for_each_segment_all_wt(bvl, bio, i)                       \
+	bio_for_each_segment_all((bvl), (bio), (i))
+
+/*
+ * This helper returns singlepage bvec to caller for readonly
+ * purpose, and the caller can _not_ change the bvec stored in
+ * bio->bi_io_vec[] via this helper.
+ */
+#define bio_for_each_segment_all_rd(bvl, bio, i, bi)			\
+	for ((bi).iter = BVEC_ITER_ALL_INIT, i = 0, bvl = &(bi).bv;	\
+	     (bi).iter.bi_idx < (bio)->bi_vcnt &&			\
+		(((bi).bv = bio_iter_iovec((bio), (bi).iter)), 1);	\
+	     bio_advance_iter((bio), &(bi).iter, (bi).bv.bv_len), i++)
+
 #define bio_iter_last(bvec, iter) ((iter).bi_size == (bvec).bv_len)
 
 static inline unsigned __bio_segments(struct bio *bio, bool mp)
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index cd395ecec99d..b4a202e98016 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -108,6 +108,12 @@ struct bio {
 
 #define BIO_RESET_BYTES		offsetof(struct bio, bi_max_vecs)
 
+/* this iter is only for implementing bio_for_each_segment_rd() */
+struct bvec_iter_all {
+	struct bvec_iter	iter;
+	struct bio_vec		bv;      /* in-flight singlepage bvec */
+};
+
 /*
  * bio flags
  */
-- 
2.7.4

[toc] | [next] | [standalone]

#1512565 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Sat, Oct 29, 2016 at 04:08:44PM +0800, Ming Lei wrote:
> This patches introduce bio_for_each_segment_all_rd() and
> bio_for_each_segment_all_wt().
> 
> bio_for_each_segment_all_rd() is for replacing
> bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
> is accessed as readonly.
> 
> bio_for_each_segment_all_wt() is for replacing
> bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
> need to be updated.

What is _rd and _wt supposed to stand for?  And speaking more
generally, could you write up some more detailed notes about all of
the various new functions they have been added, when they should be
used, and some kind of roadmap about how things are supposed to work
beyond the very high-level description in the introduction in your
patch series?  Ideally it would go into the Documentation directory,
so that after this patch set gets applied, people will be able to
refer to it to understand how things are supposed to work.

Thanks!!

						- Ted

[toc] | [prev] | [next] | [standalone]

#1512614 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Mon, Oct 31, 2016 at 09:59:43AM -0400, Theodore Ts'o wrote:
> What is _rd and _wt supposed to stand for?

I think it's read and write, but I think the naming is highly
unfortunate.  I started dabbling around with the patches a bit,
and to keep my sanity a started reaming it to _pages and _bvec
which is the real semantics - the _rd or _pages gives you a synthetic
bvec for each page, and the other one gives you the full bvec.

[toc] | [prev] | [next] | [standalone]

#1512972 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Mon, Oct 31, 2016 at 11:11 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Oct 31, 2016 at 09:59:43AM -0400, Theodore Ts'o wrote:
>> What is _rd and _wt supposed to stand for?
>
> I think it's read and write, but I think the naming is highly
> unfortunate.  I started dabbling around with the patches a bit,
> and to keep my sanity a started reaming it to _pages and _bvec
> which is the real semantics - the _rd or _pages gives you a synthetic
> bvec for each page, and the other one gives you the full bvec.

Looks _pages & _bvec is better and still a little confusing and not reflect
its real purpose from user view, since both points to real bvec. Could we
just rename _rd as bio_for_each_segment_all_ro() which means the pointed
bvec is readonly and not introduce _wt?


Thanks,
Ming Lei

[toc] | [prev] | [next] | [standalone]

#1513630 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Mon, Oct 31, 2016 at 08:11:23AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 31, 2016 at 09:59:43AM -0400, Theodore Ts'o wrote:
> > What is _rd and _wt supposed to stand for?
> 
> I think it's read and write, but I think the naming is highly
> unfortunate.  I started dabbling around with the patches a bit,
> and to keep my sanity a started reaming it to _pages and _bvec
> which is the real semantics - the _rd or _pages gives you a synthetic
> bvec for each page, and the other one gives you the full bvec.

My original naming was bio_for_each_segment() and bio_for_each_page().

[toc] | [prev] | [next] | [standalone]

#1512967 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Mon, Oct 31, 2016 at 9:59 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sat, Oct 29, 2016 at 04:08:44PM +0800, Ming Lei wrote:
>> This patches introduce bio_for_each_segment_all_rd() and
>> bio_for_each_segment_all_wt().
>>
>> bio_for_each_segment_all_rd() is for replacing
>> bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
>> is accessed as readonly.
>>
>> bio_for_each_segment_all_wt() is for replacing
>> bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
>> need to be updated.
>
> What is _rd and _wt supposed to stand for?  And speaking more

As Christoph replied, _rd means read, which said the bvec pointed by
the iterator variable(bvec pointer) is read-only, and the bvec table
can't be written into via this usage any more. Maybe
bio_for_each_segment_all_ro is better?

On the other hand, _wr meands write, which said the bvec pointed by
the iterator variable(bvec pointer) can be written to. Maybe we can use
original bio_for_each_segment_all() for it?

> generally, could you write up some more detailed notes about all of
> the various new functions they have been added, when they should be
> used, and some kind of roadmap about how things are supposed to work
> beyond the very high-level description in the introduction in your
> patch series?  Ideally it would go into the Documentation directory,
> so that after this patch set gets applied, people will be able to
> refer to it to understand how things are supposed to work.

In the next post, I will add comment on the two helpers, thanks for
your review.

Thanks,
Ming

>
> Thanks!!
>
>                                                 - Ted



-- 
Ming Lei

[toc] | [prev] | [next] | [standalone]

#1512996 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Tue, Nov 1, 2016 at 6:46 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Mon, Oct 31, 2016 at 9:59 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>> On Sat, Oct 29, 2016 at 04:08:44PM +0800, Ming Lei wrote:
>>> This patches introduce bio_for_each_segment_all_rd() and
>>> bio_for_each_segment_all_wt().
>>>
>>> bio_for_each_segment_all_rd() is for replacing
>>> bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
>>> is accessed as readonly.
>>>
>>> bio_for_each_segment_all_wt() is for replacing
>>> bio_for_each_segment_all() in case the bvec from bio->bi_io_vec
>>> need to be updated.
>>
>> What is _rd and _wt supposed to stand for?  And speaking more
>
> As Christoph replied, _rd means read, which said the bvec pointed by
> the iterator variable(bvec pointer) is read-only, and the bvec table
> can't be written into via this usage any more. Maybe
> bio_for_each_segment_all_ro is better?

Sorry for forgetting to mention one important point:

- after multipage bvec is introduced, the iterated bvec pointer
still points to singlge page bvec, which is generated in-flight
and is readonly actually. That is the motivation about the introduction
of bio_for_each_segment_all_rd().

So maybe bio_for_each_page_all_ro() is better?

>
> On the other hand, _wr meands write, which said the bvec pointed by
> the iterator variable(bvec pointer) can be written to. Maybe we can use
> original bio_for_each_segment_all() for it?

For _wt(), we still can keep it as bio_for_each_segment(), which also
reflects that now the iterated bvec points to one whole segment if
we name _rd as bio_for_each_page_all_ro().


Thanks,
Ming Lei

[toc] | [prev] | [next] | [standalone]

#1513284 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Tue, Nov 01, 2016 at 07:51:27AM +0800, Ming Lei wrote:
> Sorry for forgetting to mention one important point:
> 
> - after multipage bvec is introduced, the iterated bvec pointer
> still points to singlge page bvec, which is generated in-flight
> and is readonly actually. That is the motivation about the introduction
> of bio_for_each_segment_all_rd().
> 
> So maybe bio_for_each_page_all_ro() is better?
> 
> For _wt(), we still can keep it as bio_for_each_segment(), which also
> reflects that now the iterated bvec points to one whole segment if
> we name _rd as bio_for_each_page_all_ro().

I'm agnostic as to what the right names are --- my big concern is
there is an explosion of bio_for_each_page_* functions, and that there
isn't good documentation about (a) when to use each of these
functions, and (b) why.  I was goinig through the patch series, and it
was hard for me to figure out why, and I was looking through all of
the patches.  Once all of the patches are merged in, I am concerned
this is going to be massive trapdoor that will snare a large number of
unwitting developers.

As far as my preference, from an abstract perspective, if one version
(the read-write variant, I presume) is always safe, while one (the
read-only variant) is faster, if you can work under restricted
circumstances, naming the safe version so it is the "default", and
more dangerous one with the name that makes it a bit more obvious what
you have to do in order to use it safely, and then very clearly
document both in sources, and in the Documentation directory, what the
issues are and what you have to do in order to use the faster version.

Cheers,

					- Ted
					

[toc] | [prev] | [next] | [standalone]

#1513615 — Re: [PATCH 45/60] block: bio: introduce bio_for_each_segment_all_rd() and its write pair

On Tue, Nov 1, 2016 at 10:17 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Nov 01, 2016 at 07:51:27AM +0800, Ming Lei wrote:
>> Sorry for forgetting to mention one important point:
>>
>> - after multipage bvec is introduced, the iterated bvec pointer
>> still points to singlge page bvec, which is generated in-flight
>> and is readonly actually. That is the motivation about the introduction
>> of bio_for_each_segment_all_rd().
>>
>> So maybe bio_for_each_page_all_ro() is better?
>>
>> For _wt(), we still can keep it as bio_for_each_segment(), which also
>> reflects that now the iterated bvec points to one whole segment if
>> we name _rd as bio_for_each_page_all_ro().
>
> I'm agnostic as to what the right names are --- my big concern is
> there is an explosion of bio_for_each_page_* functions, and that there

There isn't big users of bio_for_each_segment_all(), see:

        [ming@linux-2.6]$git grep -n bio_for_each_segment_all ./fs/ | wc -l
        23

I guess there isn't execuses to switch to that after this patchset.

From view of API, bio_for_each_segment_all() is ugly and
exposes the bvec table to users, and the main reason we keep it
is that it can avoid one bvec copy in one loop. And it can be replaced
easily by bio_for_each_segment().

> isn't good documentation about (a) when to use each of these
> functions, and (b) why.  I was goinig through the patch series, and it
> was hard for me to figure out why, and I was looking through all of
> the patches.  Once all of the patches are merged in, I am concerned
> this is going to be massive trapdoor that will snare a large number of
> unwitting developers.

I understand your concern, and let me explain the whole story a bit:

1) in current linus tree, we have the following two bio iterator helpers,
for which we still don't provide any document:

       bio_for_each_segment(bvl, bio, iter)
       bio_for_each_segment_all(bvl, bio, i)

- the former is used to traverse each 'segment' in the bio range
descibed by the 'iter'(just like [start, size]); the latter is used
to traverse each 'segment' in the whole bio, so there isn't 'iter'
passed in.

- in the former helper, typeof('bvl') is 'struct bvec', and the 'segment'
is copied to 'bvl'; in the latter helper, typeof('bvl') is 'struct bvec *', and
it just points to one bvec directly in the table(bio->bi_io_vec) one by one.

- we can use the former helper to implement the latter easily and provide
a more friendly interface, and the main reason we keep it is that _all can
avoid bvec copy in each loop, so it might be a bit efficient.

- even segment is used in the helper's name, but each 'bvl' in the
helper just describes one single page, so actually they should have been
named as the following:

         bio_for_each_page(bvl, bio, iter)
         bio_for_each_page(bvl, bio, iter)

2) this patchset introduces multipage bvec, which will store one
real segment in each 'bvec' of the table(bio->bi_io_vec), and one
segment may include more than one page

- bio_for_each_segment() is kept as current interface to retrieve
one page in each 'bvl', that is just for making current users happy,
and it will be replaced with bio_for_each_page() finally, which
should be a follow-up work of this patchset

- the story of introduction of bio_for_each_segment_all_rd(bvl, bio, i):
we can't simply make 'bvl' point to each bvec in the table direclty
any more, because now each bvec in the table store one real segment
instead of one page. So in this patchst the _rd() is implemented by
bio_for_each_segment(), and we can't change/write to the bvec in the
table any more using the pointer of 'bvl' via this helper.

>
> As far as my preference, from an abstract perspective, if one version
> (the read-write variant, I presume) is always safe, while one (the
> read-only variant) is faster, if you can work under restricted
> circumstances, naming the safe version so it is the "default", and
> more dangerous one with the name that makes it a bit more obvious what
> you have to do in order to use it safely, and then very clearly
> document both in sources, and in the Documentation directory, what the
> issues are and what you have to do in order to use the faster version.

I will add detailed documents about these helpers in next version:

    - bio_for_each_segment()
    - bio_for_each_segment_all()
    - bio_for_each_page_all_ro()(renamed from bio_for_each_segment_all_rd())

Thanks,
Ming

>
> Cheers,
>
>                                         - Ted
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web