| Message ID | 1477728600-12938-46-git-send-email-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(+)