* [PATCH V10 01/19] block: introduce multi-page page bvec helpers
2018-11-15 8:52 [PATCH V10 00/19] block: support multi-page bvec Ming Lei
@ 2018-11-15 8:52 ` Ming Lei
2018-11-15 18:25 ` Omar Sandoval
2018-11-16 13:13 ` Christoph Hellwig
2018-11-15 8:52 ` [PATCH V10 02/19] block: introduce bio_for_each_bvec() Ming Lei
` (18 subsequent siblings)
19 siblings, 2 replies; 103+ messages in thread
From: Ming Lei @ 2018-11-15 8:52 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, linux-kernel, linux-mm, Ming Lei, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
This patch introduces helpers of 'mp_bvec_iter_*' for multipage
bvec support.
The introduced helpers treate one bvec as real multi-page segment,
which may include more than one pages.
The existed helpers of bvec_iter_* are interfaces for supporting current
bvec iterator which is thought as single-page by drivers, fs, dm and
etc. These introduced helpers will build single-page bvec in flight, so
this way won't break current bio/bvec users, which needn't any change.
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Kent Overstreet <kent.overstreet@gmail.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: Shaohua Li <shli@kernel.org>
Cc: linux-raid@vger.kernel.org
Cc: linux-erofs@lists.ozlabs.org
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Cc: Gao Xiang <gaoxiang25@huawei.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: linux-ext4@vger.kernel.org
Cc: Coly Li <colyli@suse.de>
Cc: linux-bcache@vger.kernel.org
Cc: Boaz Harrosh <ooo@electrozaur.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: cluster-devel@redhat.com
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
include/linux/bvec.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 60 insertions(+), 3 deletions(-)
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index 02c73c6aa805..8ef904a50577 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -23,6 +23,44 @@
#include <linux/kernel.h>
#include <linux/bug.h>
#include <linux/errno.h>
+#include <linux/mm.h>
+
+/*
+ * What is multi-page bvecs?
+ *
+ * - bvecs stored in bio->bi_io_vec is always multi-page(mp) style
+ *
+ * - bvec(struct bio_vec) represents one physically contiguous I/O
+ * buffer, now the buffer may include more than one pages after
+ * multi-page(mp) bvec is supported, and all these pages represented
+ * by one bvec is physically contiguous. Before mp support, at most
+ * one page is included in one bvec, we call it single-page(sp)
+ * bvec.
+ *
+ * - .bv_page of the bvec represents the 1st page in the mp bvec
+ *
+ * - .bv_offset of the bvec represents offset of the buffer in the bvec
+ *
+ * The effect on the current drivers/filesystem/dm/bcache/...:
+ *
+ * - almost everyone supposes that one bvec only includes one single
+ * page, so we keep the sp interface not changed, for example,
+ * bio_for_each_segment() still returns bvec with single page
+ *
+ * - bio_for_each_segment*() will be changed to return single-page
+ * bvec too
+ *
+ * - during iterating, iterator variable(struct bvec_iter) is always
+ * updated in multipage bvec style and that means bvec_iter_advance()
+ * is kept not changed
+ *
+ * - returned(copied) single-page bvec is built in flight by bvec
+ * helpers from the stored multipage bvec
+ *
+ * - In case that some components(such as iov_iter) need to support
+ * multi-page bvec, we introduce new helpers(mp_bvec_iter_*) for
+ * them.
+ */
/*
* was unsigned short, but we might as well be ready for > 64kB I/O pages
@@ -50,16 +88,35 @@ struct bvec_iter {
*/
#define __bvec_iter_bvec(bvec, iter) (&(bvec)[(iter).bi_idx])
-#define bvec_iter_page(bvec, iter) \
+#define mp_bvec_iter_page(bvec, iter) \
(__bvec_iter_bvec((bvec), (iter))->bv_page)
-#define bvec_iter_len(bvec, iter) \
+#define mp_bvec_iter_len(bvec, iter) \
min((iter).bi_size, \
__bvec_iter_bvec((bvec), (iter))->bv_len - (iter).bi_bvec_done)
-#define bvec_iter_offset(bvec, iter) \
+#define mp_bvec_iter_offset(bvec, iter) \
(__bvec_iter_bvec((bvec), (iter))->bv_offset + (iter).bi_bvec_done)
+#define mp_bvec_iter_page_idx(bvec, iter) \
+ (mp_bvec_iter_offset((bvec), (iter)) / PAGE_SIZE)
+
+/*
+ * <page, offset,length> of single-page(sp) segment.
+ *
+ * This helpers are for building sp bvec in flight.
+ */
+#define bvec_iter_offset(bvec, iter) \
+ (mp_bvec_iter_offset((bvec), (iter)) % PAGE_SIZE)
+
+#define bvec_iter_len(bvec, iter) \
+ min_t(unsigned, mp_bvec_iter_len((bvec), (iter)), \
+ (PAGE_SIZE - (bvec_iter_offset((bvec), (iter)))))
+
+#define bvec_iter_page(bvec, iter) \
+ nth_page(mp_bvec_iter_page((bvec), (iter)), \
+ mp_bvec_iter_page_idx((bvec), (iter)))
+
#define bvec_iter_bvec(bvec, iter) \
((struct bio_vec) { \
.bv_page = bvec_iter_page((bvec), (iter)), \
--
2.9.5
^ permalink raw reply related [flat|nested] 103+ messages in thread* Re: [PATCH V10 01/19] block: introduce multi-page page bvec helpers
2018-11-15 8:52 ` [PATCH V10 01/19] block: introduce multi-page page bvec helpers Ming Lei
@ 2018-11-15 18:25 ` Omar Sandoval
2018-11-19 2:25 ` Ming Lei
2018-11-16 13:13 ` Christoph Hellwig
1 sibling, 1 reply; 103+ messages in thread
From: Omar Sandoval @ 2018-11-15 18:25 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
On Thu, Nov 15, 2018 at 04:52:48PM +0800, Ming Lei wrote:
> This patch introduces helpers of 'mp_bvec_iter_*' for multipage
> bvec support.
>
> The introduced helpers treate one bvec as real multi-page segment,
> which may include more than one pages.
>
> The existed helpers of bvec_iter_* are interfaces for supporting current
> bvec iterator which is thought as single-page by drivers, fs, dm and
> etc. These introduced helpers will build single-page bvec in flight, so
> this way won't break current bio/bvec users, which needn't any change.
>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Kent Overstreet <kent.overstreet@gmail.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: dm-devel@redhat.com
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: Shaohua Li <shli@kernel.org>
> Cc: linux-raid@vger.kernel.org
> Cc: linux-erofs@lists.ozlabs.org
> Cc: David Sterba <dsterba@suse.com>
> Cc: linux-btrfs@vger.kernel.org
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: linux-xfs@vger.kernel.org
> Cc: Gao Xiang <gaoxiang25@huawei.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: linux-ext4@vger.kernel.org
> Cc: Coly Li <colyli@suse.de>
> Cc: linux-bcache@vger.kernel.org
> Cc: Boaz Harrosh <ooo@electrozaur.com>
> Cc: Bob Peterson <rpeterso@redhat.com>
> Cc: cluster-devel@redhat.com
Reviewed-by: Omar Sandoval <osandov@fb.com>
But a couple of comments below.
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> include/linux/bvec.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index 02c73c6aa805..8ef904a50577 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -23,6 +23,44 @@
> #include <linux/kernel.h>
> #include <linux/bug.h>
> #include <linux/errno.h>
> +#include <linux/mm.h>
> +
> +/*
> + * What is multi-page bvecs?
> + *
> + * - bvecs stored in bio->bi_io_vec is always multi-page(mp) style
> + *
> + * - bvec(struct bio_vec) represents one physically contiguous I/O
> + * buffer, now the buffer may include more than one pages after
> + * multi-page(mp) bvec is supported, and all these pages represented
> + * by one bvec is physically contiguous. Before mp support, at most
> + * one page is included in one bvec, we call it single-page(sp)
> + * bvec.
> + *
> + * - .bv_page of the bvec represents the 1st page in the mp bvec
> + *
> + * - .bv_offset of the bvec represents offset of the buffer in the bvec
> + *
> + * The effect on the current drivers/filesystem/dm/bcache/...:
> + *
> + * - almost everyone supposes that one bvec only includes one single
> + * page, so we keep the sp interface not changed, for example,
> + * bio_for_each_segment() still returns bvec with single page
> + *
> + * - bio_for_each_segment*() will be changed to return single-page
> + * bvec too
> + *
> + * - during iterating, iterator variable(struct bvec_iter) is always
> + * updated in multipage bvec style and that means bvec_iter_advance()
> + * is kept not changed
> + *
> + * - returned(copied) single-page bvec is built in flight by bvec
> + * helpers from the stored multipage bvec
> + *
> + * - In case that some components(such as iov_iter) need to support
> + * multi-page bvec, we introduce new helpers(mp_bvec_iter_*) for
> + * them.
> + */
This comment sounds more like a commit message (i.e., how were things
before, and how are we changing them). In a couple of years when I read
this code, I probably won't care how it was changed, just how it works.
So I think a comment explaining the concepts of multi-page and
single-page bvecs is very useful, but please move all of the "foo was
changed" and "before mp support" type stuff to the commit message.
> /*
> * was unsigned short, but we might as well be ready for > 64kB I/O pages
> @@ -50,16 +88,35 @@ struct bvec_iter {
> */
> #define __bvec_iter_bvec(bvec, iter) (&(bvec)[(iter).bi_idx])
>
> -#define bvec_iter_page(bvec, iter) \
> +#define mp_bvec_iter_page(bvec, iter) \
> (__bvec_iter_bvec((bvec), (iter))->bv_page)
>
> -#define bvec_iter_len(bvec, iter) \
> +#define mp_bvec_iter_len(bvec, iter) \
> min((iter).bi_size, \
> __bvec_iter_bvec((bvec), (iter))->bv_len - (iter).bi_bvec_done)
>
> -#define bvec_iter_offset(bvec, iter) \
> +#define mp_bvec_iter_offset(bvec, iter) \
> (__bvec_iter_bvec((bvec), (iter))->bv_offset + (iter).bi_bvec_done)
>
> +#define mp_bvec_iter_page_idx(bvec, iter) \
> + (mp_bvec_iter_offset((bvec), (iter)) / PAGE_SIZE)
> +
> +/*
> + * <page, offset,length> of single-page(sp) segment.
> + *
> + * This helpers are for building sp bvec in flight.
> + */
> +#define bvec_iter_offset(bvec, iter) \
> + (mp_bvec_iter_offset((bvec), (iter)) % PAGE_SIZE)
> +
> +#define bvec_iter_len(bvec, iter) \
> + min_t(unsigned, mp_bvec_iter_len((bvec), (iter)), \
> + (PAGE_SIZE - (bvec_iter_offset((bvec), (iter)))))
The parentheses around (bvec_iter_offset((bvec), (iter))) and
(PAGE_SIZE - (bvec_iter_offset((bvec), (iter)))) are unnecessary
clutter. This looks easier to read to me:
#define bvec_iter_len(bvec, iter) \
min_t(unsigned, mp_bvec_iter_len((bvec), (iter)), \
PAGE_SIZE - bvec_iter_offset((bvec), (iter)))
> +
> +#define bvec_iter_page(bvec, iter) \
> + nth_page(mp_bvec_iter_page((bvec), (iter)), \
> + mp_bvec_iter_page_idx((bvec), (iter)))
> +
> #define bvec_iter_bvec(bvec, iter) \
> ((struct bio_vec) { \
> .bv_page = bvec_iter_page((bvec), (iter)), \
> --
> 2.9.5
>
^ permalink raw reply [flat|nested] 103+ messages in thread* Re: [PATCH V10 01/19] block: introduce multi-page page bvec helpers
2018-11-15 18:25 ` Omar Sandoval
@ 2018-11-19 2:25 ` Ming Lei
0 siblings, 0 replies; 103+ messages in thread
From: Ming Lei @ 2018-11-19 2:25 UTC (permalink / raw)
To: Omar Sandoval
Cc: Jens Axboe, linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
On Thu, Nov 15, 2018 at 10:25:59AM -0800, Omar Sandoval wrote:
> On Thu, Nov 15, 2018 at 04:52:48PM +0800, Ming Lei wrote:
> > This patch introduces helpers of 'mp_bvec_iter_*' for multipage
> > bvec support.
> >
> > The introduced helpers treate one bvec as real multi-page segment,
> > which may include more than one pages.
> >
> > The existed helpers of bvec_iter_* are interfaces for supporting current
> > bvec iterator which is thought as single-page by drivers, fs, dm and
> > etc. These introduced helpers will build single-page bvec in flight, so
> > this way won't break current bio/bvec users, which needn't any change.
> >
> > Cc: Dave Chinner <dchinner@redhat.com>
> > Cc: Kent Overstreet <kent.overstreet@gmail.com>
> > Cc: Mike Snitzer <snitzer@redhat.com>
> > Cc: dm-devel@redhat.com
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: Shaohua Li <shli@kernel.org>
> > Cc: linux-raid@vger.kernel.org
> > Cc: linux-erofs@lists.ozlabs.org
> > Cc: David Sterba <dsterba@suse.com>
> > Cc: linux-btrfs@vger.kernel.org
> > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > Cc: linux-xfs@vger.kernel.org
> > Cc: Gao Xiang <gaoxiang25@huawei.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: linux-ext4@vger.kernel.org
> > Cc: Coly Li <colyli@suse.de>
> > Cc: linux-bcache@vger.kernel.org
> > Cc: Boaz Harrosh <ooo@electrozaur.com>
> > Cc: Bob Peterson <rpeterso@redhat.com>
> > Cc: cluster-devel@redhat.com
>
> Reviewed-by: Omar Sandoval <osandov@fb.com>
>
> But a couple of comments below.
>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > include/linux/bvec.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 60 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> > index 02c73c6aa805..8ef904a50577 100644
> > --- a/include/linux/bvec.h
> > +++ b/include/linux/bvec.h
> > @@ -23,6 +23,44 @@
> > #include <linux/kernel.h>
> > #include <linux/bug.h>
> > #include <linux/errno.h>
> > +#include <linux/mm.h>
> > +
> > +/*
> > + * What is multi-page bvecs?
> > + *
> > + * - bvecs stored in bio->bi_io_vec is always multi-page(mp) style
> > + *
> > + * - bvec(struct bio_vec) represents one physically contiguous I/O
> > + * buffer, now the buffer may include more than one pages after
> > + * multi-page(mp) bvec is supported, and all these pages represented
> > + * by one bvec is physically contiguous. Before mp support, at most
> > + * one page is included in one bvec, we call it single-page(sp)
> > + * bvec.
> > + *
> > + * - .bv_page of the bvec represents the 1st page in the mp bvec
> > + *
> > + * - .bv_offset of the bvec represents offset of the buffer in the bvec
> > + *
> > + * The effect on the current drivers/filesystem/dm/bcache/...:
> > + *
> > + * - almost everyone supposes that one bvec only includes one single
> > + * page, so we keep the sp interface not changed, for example,
> > + * bio_for_each_segment() still returns bvec with single page
> > + *
> > + * - bio_for_each_segment*() will be changed to return single-page
> > + * bvec too
> > + *
> > + * - during iterating, iterator variable(struct bvec_iter) is always
> > + * updated in multipage bvec style and that means bvec_iter_advance()
> > + * is kept not changed
> > + *
> > + * - returned(copied) single-page bvec is built in flight by bvec
> > + * helpers from the stored multipage bvec
> > + *
> > + * - In case that some components(such as iov_iter) need to support
> > + * multi-page bvec, we introduce new helpers(mp_bvec_iter_*) for
> > + * them.
> > + */
>
> This comment sounds more like a commit message (i.e., how were things
> before, and how are we changing them). In a couple of years when I read
> this code, I probably won't care how it was changed, just how it works.
> So I think a comment explaining the concepts of multi-page and
> single-page bvecs is very useful, but please move all of the "foo was
> changed" and "before mp support" type stuff to the commit message.
OK.
>
> > /*
> > * was unsigned short, but we might as well be ready for > 64kB I/O pages
> > @@ -50,16 +88,35 @@ struct bvec_iter {
> > */
> > #define __bvec_iter_bvec(bvec, iter) (&(bvec)[(iter).bi_idx])
> >
> > -#define bvec_iter_page(bvec, iter) \
> > +#define mp_bvec_iter_page(bvec, iter) \
> > (__bvec_iter_bvec((bvec), (iter))->bv_page)
> >
> > -#define bvec_iter_len(bvec, iter) \
> > +#define mp_bvec_iter_len(bvec, iter) \
> > min((iter).bi_size, \
> > __bvec_iter_bvec((bvec), (iter))->bv_len - (iter).bi_bvec_done)
> >
> > -#define bvec_iter_offset(bvec, iter) \
> > +#define mp_bvec_iter_offset(bvec, iter) \
> > (__bvec_iter_bvec((bvec), (iter))->bv_offset + (iter).bi_bvec_done)
> >
> > +#define mp_bvec_iter_page_idx(bvec, iter) \
> > + (mp_bvec_iter_offset((bvec), (iter)) / PAGE_SIZE)
> > +
> > +/*
> > + * <page, offset,length> of single-page(sp) segment.
> > + *
> > + * This helpers are for building sp bvec in flight.
> > + */
> > +#define bvec_iter_offset(bvec, iter) \
> > + (mp_bvec_iter_offset((bvec), (iter)) % PAGE_SIZE)
> > +
> > +#define bvec_iter_len(bvec, iter) \
> > + min_t(unsigned, mp_bvec_iter_len((bvec), (iter)), \
> > + (PAGE_SIZE - (bvec_iter_offset((bvec), (iter)))))
>
> The parentheses around (bvec_iter_offset((bvec), (iter))) and
> (PAGE_SIZE - (bvec_iter_offset((bvec), (iter)))) are unnecessary
> clutter. This looks easier to read to me:
Good catch!
Thanks,
Ming
^ permalink raw reply [flat|nested] 103+ messages in thread
* Re: [PATCH V10 01/19] block: introduce multi-page page bvec helpers
2018-11-15 8:52 ` [PATCH V10 01/19] block: introduce multi-page page bvec helpers Ming Lei
2018-11-15 18:25 ` Omar Sandoval
@ 2018-11-16 13:13 ` Christoph Hellwig
2018-11-19 2:23 ` Ming Lei
1 sibling, 1 reply; 103+ messages in thread
From: Christoph Hellwig @ 2018-11-16 13:13 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
> -#define bvec_iter_page(bvec, iter) \
> +#define mp_bvec_iter_page(bvec, iter) \
> (__bvec_iter_bvec((bvec), (iter))->bv_page)
>
> -#define bvec_iter_len(bvec, iter) \
> +#define mp_bvec_iter_len(bvec, iter) \
I'd much prefer if we would stick to the segment naming that
we also use in the higher level helper.
So segment_iter_page, segment_iter_len, etc.
> + * This helpers are for building sp bvec in flight.
Please spell out single page, sp is not easy understandable.
^ permalink raw reply [flat|nested] 103+ messages in thread
* Re: [PATCH V10 01/19] block: introduce multi-page page bvec helpers
2018-11-16 13:13 ` Christoph Hellwig
@ 2018-11-19 2:23 ` Ming Lei
2018-11-19 3:10 ` Jens Axboe
0 siblings, 1 reply; 103+ messages in thread
From: Ming Lei @ 2018-11-19 2:23 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Theodore Ts'o, linux-ext4, Coly Li, linux-bcache,
Boaz Harrosh, Bob Peterson, cluster-devel
On Fri, Nov 16, 2018 at 02:13:05PM +0100, Christoph Hellwig wrote:
> > -#define bvec_iter_page(bvec, iter) \
> > +#define mp_bvec_iter_page(bvec, iter) \
> > (__bvec_iter_bvec((bvec), (iter))->bv_page)
> >
> > -#define bvec_iter_len(bvec, iter) \
> > +#define mp_bvec_iter_len(bvec, iter) \
>
> I'd much prefer if we would stick to the segment naming that
> we also use in the higher level helper.
>
> So segment_iter_page, segment_iter_len, etc.
We discussed the naming problem before, one big problem is that the 'segment'
in bio_for_each_segment*() means one single page segment actually.
If we use segment_iter_page() here for multi-page segment, it may
confuse people.
Of course, I prefer to the naming of segment/page,
And Jens didn't agree to rename bio_for_each_segment*() before.
So what is the solution we should take for moving on?
>
> > + * This helpers are for building sp bvec in flight.
>
> Please spell out single page, sp is not easy understandable.
OK.
Thanks,
Ming
^ permalink raw reply [flat|nested] 103+ messages in thread
* Re: [PATCH V10 01/19] block: introduce multi-page page bvec helpers
2018-11-19 2:23 ` Ming Lei
@ 2018-11-19 3:10 ` Jens Axboe
2018-11-19 3:35 ` Ming Lei
0 siblings, 1 reply; 103+ messages in thread
From: Jens Axboe @ 2018-11-19 3:10 UTC (permalink / raw)
To: Ming Lei, Christoph Hellwig
Cc: linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Theodore Ts'o, linux-ext4, Coly Li, linux-bcache,
Boaz Harrosh, Bob Peterson, cluster-devel
On 11/18/18 7:23 PM, Ming Lei wrote:
> On Fri, Nov 16, 2018 at 02:13:05PM +0100, Christoph Hellwig wrote:
>>> -#define bvec_iter_page(bvec, iter) \
>>> +#define mp_bvec_iter_page(bvec, iter) \
>>> (__bvec_iter_bvec((bvec), (iter))->bv_page)
>>>
>>> -#define bvec_iter_len(bvec, iter) \
>>> +#define mp_bvec_iter_len(bvec, iter) \
>>
>> I'd much prefer if we would stick to the segment naming that
>> we also use in the higher level helper.
>>
>> So segment_iter_page, segment_iter_len, etc.
>
> We discussed the naming problem before, one big problem is that the 'segment'
> in bio_for_each_segment*() means one single page segment actually.
>
> If we use segment_iter_page() here for multi-page segment, it may
> confuse people.
>
> Of course, I prefer to the naming of segment/page,
>
> And Jens didn't agree to rename bio_for_each_segment*() before.
I didn't like frivolous renaming (and I still don't), but mp_
is horrible imho. Don't name these after the fact that they
are done in conjunction with supporting multipage bvecs. That
very fact will be irrelevant very soon
--
Jens Axboe
^ permalink raw reply [flat|nested] 103+ messages in thread
* Re: [PATCH V10 01/19] block: introduce multi-page page bvec helpers
2018-11-19 3:10 ` Jens Axboe
@ 2018-11-19 3:35 ` Ming Lei
0 siblings, 0 replies; 103+ messages in thread
From: Ming Lei @ 2018-11-19 3:35 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, linux-block, linux-kernel, linux-mm,
Dave Chinner, Kent Overstreet, Mike Snitzer, dm-devel,
Alexander Viro, linux-fsdevel, Shaohua Li, linux-raid,
linux-erofs, David Sterba, linux-btrfs, Darrick J . Wong,
linux-xfs, Gao Xiang, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
On Sun, Nov 18, 2018 at 08:10:14PM -0700, Jens Axboe wrote:
> On 11/18/18 7:23 PM, Ming Lei wrote:
> > On Fri, Nov 16, 2018 at 02:13:05PM +0100, Christoph Hellwig wrote:
> >>> -#define bvec_iter_page(bvec, iter) \
> >>> +#define mp_bvec_iter_page(bvec, iter) \
> >>> (__bvec_iter_bvec((bvec), (iter))->bv_page)
> >>>
> >>> -#define bvec_iter_len(bvec, iter) \
> >>> +#define mp_bvec_iter_len(bvec, iter) \
> >>
> >> I'd much prefer if we would stick to the segment naming that
> >> we also use in the higher level helper.
> >>
> >> So segment_iter_page, segment_iter_len, etc.
> >
> > We discussed the naming problem before, one big problem is that the 'segment'
> > in bio_for_each_segment*() means one single page segment actually.
> >
> > If we use segment_iter_page() here for multi-page segment, it may
> > confuse people.
> >
> > Of course, I prefer to the naming of segment/page,
> >
> > And Jens didn't agree to rename bio_for_each_segment*() before.
>
> I didn't like frivolous renaming (and I still don't), but mp_
> is horrible imho. Don't name these after the fact that they
> are done in conjunction with supporting multipage bvecs. That
> very fact will be irrelevant very soon
OK, so what is your suggestion for the naming issue?
Are you fine to use segment_iter_page() here? Then the term of 'segment'
may be interpreted as multi-page segment here, but as single-page in
bio_for_each_segment*().
thanks
Ming
^ permalink raw reply [flat|nested] 103+ messages in thread
* [PATCH V10 02/19] block: introduce bio_for_each_bvec()
2018-11-15 8:52 [PATCH V10 00/19] block: support multi-page bvec Ming Lei
2018-11-15 8:52 ` [PATCH V10 01/19] block: introduce multi-page page bvec helpers Ming Lei
@ 2018-11-15 8:52 ` Ming Lei
2018-11-15 18:28 ` Omar Sandoval
2018-11-16 13:30 ` Christoph Hellwig
2018-11-15 8:52 ` [PATCH V10 03/19] block: use bio_for_each_bvec() to compute multi-page bvec count Ming Lei
` (17 subsequent siblings)
19 siblings, 2 replies; 103+ messages in thread
From: Ming Lei @ 2018-11-15 8:52 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, linux-kernel, linux-mm, Ming Lei, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
This helper is used for iterating over multi-page bvec for bio
split & merge code.
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Kent Overstreet <kent.overstreet@gmail.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: Shaohua Li <shli@kernel.org>
Cc: linux-raid@vger.kernel.org
Cc: linux-erofs@lists.ozlabs.org
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Cc: Gao Xiang <gaoxiang25@huawei.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: linux-ext4@vger.kernel.org
Cc: Coly Li <colyli@suse.de>
Cc: linux-bcache@vger.kernel.org
Cc: Boaz Harrosh <ooo@electrozaur.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: cluster-devel@redhat.com
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
include/linux/bio.h | 34 +++++++++++++++++++++++++++++++---
include/linux/bvec.h | 36 ++++++++++++++++++++++++++++++++----
2 files changed, 63 insertions(+), 7 deletions(-)
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 056fb627edb3..1f0dcf109841 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -76,6 +76,9 @@
#define bio_data_dir(bio) \
(op_is_write(bio_op(bio)) ? WRITE : READ)
+#define bio_iter_mp_iovec(bio, iter) \
+ mp_bvec_iter_bvec((bio)->bi_io_vec, (iter))
+
/*
* Check whether this bio carries any data or not. A NULL bio is allowed.
*/
@@ -135,18 +138,33 @@ static inline bool bio_full(struct bio *bio)
#define bio_for_each_segment_all(bvl, bio, i) \
for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
-static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
- unsigned bytes)
+static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
+ unsigned bytes, bool mp)
{
iter->bi_sector += bytes >> 9;
if (bio_no_advance_iter(bio))
iter->bi_size -= bytes;
else
- bvec_iter_advance(bio->bi_io_vec, iter, bytes);
+ if (!mp)
+ bvec_iter_advance(bio->bi_io_vec, iter, bytes);
+ else
+ mp_bvec_iter_advance(bio->bi_io_vec, iter, bytes);
/* TODO: It is reasonable to complete bio with error here. */
}
+static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
+ unsigned bytes)
+{
+ __bio_advance_iter(bio, iter, bytes, false);
+}
+
+static inline void bio_advance_mp_iter(struct bio *bio, struct bvec_iter *iter,
+ unsigned bytes)
+{
+ __bio_advance_iter(bio, iter, bytes, true);
+}
+
#define __bio_for_each_segment(bvl, bio, iter, start) \
for (iter = (start); \
(iter).bi_size && \
@@ -156,6 +174,16 @@ static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
#define bio_for_each_segment(bvl, bio, iter) \
__bio_for_each_segment(bvl, bio, iter, (bio)->bi_iter)
+#define __bio_for_each_bvec(bvl, bio, iter, start) \
+ for (iter = (start); \
+ (iter).bi_size && \
+ ((bvl = bio_iter_mp_iovec((bio), (iter))), 1); \
+ bio_advance_mp_iter((bio), &(iter), (bvl).bv_len))
+
+/* returns one real segment(multipage bvec) each time */
+#define bio_for_each_bvec(bvl, bio, iter) \
+ __bio_for_each_bvec(bvl, bio, iter, (bio)->bi_iter)
+
#define bio_iter_last(bvec, iter) ((iter).bi_size == (bvec).bv_len)
static inline unsigned bio_segments(struct bio *bio)
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index 8ef904a50577..3d61352cd8cf 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -124,8 +124,16 @@ struct bvec_iter {
.bv_offset = bvec_iter_offset((bvec), (iter)), \
})
-static inline bool bvec_iter_advance(const struct bio_vec *bv,
- struct bvec_iter *iter, unsigned bytes)
+#define mp_bvec_iter_bvec(bvec, iter) \
+((struct bio_vec) { \
+ .bv_page = mp_bvec_iter_page((bvec), (iter)), \
+ .bv_len = mp_bvec_iter_len((bvec), (iter)), \
+ .bv_offset = mp_bvec_iter_offset((bvec), (iter)), \
+})
+
+static inline bool __bvec_iter_advance(const struct bio_vec *bv,
+ struct bvec_iter *iter,
+ unsigned bytes, bool mp)
{
if (WARN_ONCE(bytes > iter->bi_size,
"Attempted to advance past end of bvec iter\n")) {
@@ -134,8 +142,14 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
}
while (bytes) {
- unsigned iter_len = bvec_iter_len(bv, *iter);
- unsigned len = min(bytes, iter_len);
+ unsigned len;
+
+ if (mp)
+ len = mp_bvec_iter_len(bv, *iter);
+ else
+ len = bvec_iter_len(bv, *iter);
+
+ len = min(bytes, len);
bytes -= len;
iter->bi_size -= len;
@@ -173,6 +187,20 @@ static inline bool bvec_iter_rewind(const struct bio_vec *bv,
return true;
}
+static inline bool bvec_iter_advance(const struct bio_vec *bv,
+ struct bvec_iter *iter,
+ unsigned bytes)
+{
+ return __bvec_iter_advance(bv, iter, bytes, false);
+}
+
+static inline bool mp_bvec_iter_advance(const struct bio_vec *bv,
+ struct bvec_iter *iter,
+ unsigned bytes)
+{
+ return __bvec_iter_advance(bv, iter, bytes, true);
+}
+
#define for_each_bvec(bvl, bio_vec, iter, start) \
for (iter = (start); \
(iter).bi_size && \
--
2.9.5
^ permalink raw reply related [flat|nested] 103+ messages in thread* Re: [PATCH V10 02/19] block: introduce bio_for_each_bvec()
2018-11-15 8:52 ` [PATCH V10 02/19] block: introduce bio_for_each_bvec() Ming Lei
@ 2018-11-15 18:28 ` Omar Sandoval
2018-11-16 13:30 ` Christoph Hellwig
1 sibling, 0 replies; 103+ messages in thread
From: Omar Sandoval @ 2018-11-15 18:28 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
On Thu, Nov 15, 2018 at 04:52:49PM +0800, Ming Lei wrote:
> This helper is used for iterating over multi-page bvec for bio
> split & merge code.
>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Kent Overstreet <kent.overstreet@gmail.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: dm-devel@redhat.com
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: Shaohua Li <shli@kernel.org>
> Cc: linux-raid@vger.kernel.org
> Cc: linux-erofs@lists.ozlabs.org
> Cc: David Sterba <dsterba@suse.com>
> Cc: linux-btrfs@vger.kernel.org
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: linux-xfs@vger.kernel.org
> Cc: Gao Xiang <gaoxiang25@huawei.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: linux-ext4@vger.kernel.org
> Cc: Coly Li <colyli@suse.de>
> Cc: linux-bcache@vger.kernel.org
> Cc: Boaz Harrosh <ooo@electrozaur.com>
> Cc: Bob Peterson <rpeterso@redhat.com>
> Cc: cluster-devel@redhat.com
Reviewed-by: Omar Sandoval <osandov@fb.com>
One comment below.
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> include/linux/bio.h | 34 +++++++++++++++++++++++++++++++---
> include/linux/bvec.h | 36 ++++++++++++++++++++++++++++++++----
> 2 files changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 056fb627edb3..1f0dcf109841 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -76,6 +76,9 @@
> #define bio_data_dir(bio) \
> (op_is_write(bio_op(bio)) ? WRITE : READ)
>
> +#define bio_iter_mp_iovec(bio, iter) \
> + mp_bvec_iter_bvec((bio)->bi_io_vec, (iter))
> +
> /*
> * Check whether this bio carries any data or not. A NULL bio is allowed.
> */
> @@ -135,18 +138,33 @@ static inline bool bio_full(struct bio *bio)
> #define bio_for_each_segment_all(bvl, bio, i) \
> for (i = 0, bvl = (bio)->bi_io_vec; i < (bio)->bi_vcnt; i++, bvl++)
>
> -static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> - unsigned bytes)
> +static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> + unsigned bytes, bool mp)
> {
> iter->bi_sector += bytes >> 9;
>
> if (bio_no_advance_iter(bio))
> iter->bi_size -= bytes;
> else
> - bvec_iter_advance(bio->bi_io_vec, iter, bytes);
> + if (!mp)
> + bvec_iter_advance(bio->bi_io_vec, iter, bytes);
> + else
> + mp_bvec_iter_advance(bio->bi_io_vec, iter, bytes);
if (!foo) {} else {} hurts my brain, please do
if (mp)
mp_bvec_iter_advance(bio->bi_io_vec, iter, bytes);
else
bvec_iter_advance(bio->bi_io_vec, iter, bytes);
^ permalink raw reply [flat|nested] 103+ messages in thread* Re: [PATCH V10 02/19] block: introduce bio_for_each_bvec()
2018-11-15 8:52 ` [PATCH V10 02/19] block: introduce bio_for_each_bvec() Ming Lei
2018-11-15 18:28 ` Omar Sandoval
@ 2018-11-16 13:30 ` Christoph Hellwig
2018-11-19 3:31 ` Ming Lei
1 sibling, 1 reply; 103+ messages in thread
From: Christoph Hellwig @ 2018-11-16 13:30 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
> +static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> + unsigned bytes, bool mp)
I think these magic 'bool np' arguments and wrappers over wrapper
don't help anyone to actually understand the code. I'd vote for
removing as many wrappers as we really don't need, and passing the
actual segment limit instead of the magic bool flag. Something like
this untested patch:
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 277921ad42e7..dcad0b69f57a 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -138,30 +138,21 @@ static inline bool bio_full(struct bio *bio)
bvec_for_each_segment(bvl, &((bio)->bi_io_vec[iter_all.idx]), i, iter_all)
static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
- unsigned bytes, bool mp)
+ unsigned bytes, unsigned max_segment)
{
iter->bi_sector += bytes >> 9;
if (bio_no_advance_iter(bio))
iter->bi_size -= bytes;
else
- if (!mp)
- bvec_iter_advance(bio->bi_io_vec, iter, bytes);
- else
- mp_bvec_iter_advance(bio->bi_io_vec, iter, bytes);
+ __bvec_iter_advance(bio->bi_io_vec, iter, bytes, max_segment);
/* TODO: It is reasonable to complete bio with error here. */
}
static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
unsigned bytes)
{
- __bio_advance_iter(bio, iter, bytes, false);
-}
-
-static inline void bio_advance_mp_iter(struct bio *bio, struct bvec_iter *iter,
- unsigned bytes)
-{
- __bio_advance_iter(bio, iter, bytes, true);
+ __bio_advance_iter(bio, iter, bytes, PAGE_SIZE);
}
#define __bio_for_each_segment(bvl, bio, iter, start) \
@@ -177,7 +168,7 @@ static inline void bio_advance_mp_iter(struct bio *bio, struct bvec_iter *iter,
for (iter = (start); \
(iter).bi_size && \
((bvl = bio_iter_mp_iovec((bio), (iter))), 1); \
- bio_advance_mp_iter((bio), &(iter), (bvl).bv_len))
+ __bio_advance_iter((bio), &(iter), (bvl).bv_len, 0))
/* returns one real segment(multipage bvec) each time */
#define bio_for_each_bvec(bvl, bio, iter) \
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index 02f26d2b59ad..5e2ed46c1c88 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -138,8 +138,7 @@ struct bvec_iter_all {
})
static inline bool __bvec_iter_advance(const struct bio_vec *bv,
- struct bvec_iter *iter,
- unsigned bytes, bool mp)
+ struct bvec_iter *iter, unsigned bytes, unsigned max_segment)
{
if (WARN_ONCE(bytes > iter->bi_size,
"Attempted to advance past end of bvec iter\n")) {
@@ -148,18 +147,18 @@ static inline bool __bvec_iter_advance(const struct bio_vec *bv,
}
while (bytes) {
- unsigned len;
+ unsigned segment_len = mp_bvec_iter_len(bv, *iter);
- if (mp)
- len = mp_bvec_iter_len(bv, *iter);
- else
- len = bvec_iter_len(bv, *iter);
+ if (max_segment) {
+ max_segment -= bvec_iter_offset(bv, *iter);
+ segment_len = min(segment_len, max_segment);
+ }
- len = min(bytes, len);
+ segment_len = min(bytes, segment_len);
- bytes -= len;
- iter->bi_size -= len;
- iter->bi_bvec_done += len;
+ bytes -= segment_len;
+ iter->bi_size -= segment_len;
+ iter->bi_bvec_done += segment_len;
if (iter->bi_bvec_done == __bvec_iter_bvec(bv, *iter)->bv_len) {
iter->bi_bvec_done = 0;
@@ -197,14 +196,7 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
struct bvec_iter *iter,
unsigned bytes)
{
- return __bvec_iter_advance(bv, iter, bytes, false);
-}
-
-static inline bool mp_bvec_iter_advance(const struct bio_vec *bv,
- struct bvec_iter *iter,
- unsigned bytes)
-{
- return __bvec_iter_advance(bv, iter, bytes, true);
+ return __bvec_iter_advance(bv, iter, bytes, PAGE_SIZE);
}
#define for_each_bvec(bvl, bio_vec, iter, start) \
^ permalink raw reply related [flat|nested] 103+ messages in thread* Re: [PATCH V10 02/19] block: introduce bio_for_each_bvec()
2018-11-16 13:30 ` Christoph Hellwig
@ 2018-11-19 3:31 ` Ming Lei
0 siblings, 0 replies; 103+ messages in thread
From: Ming Lei @ 2018-11-19 3:31 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, linux-block, linux-kernel, linux-mm, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Theodore Ts'o, linux-ext4, Coly Li, linux-bcache,
Boaz Harrosh, Bob Peterson, cluster-devel
On Fri, Nov 16, 2018 at 02:30:28PM +0100, Christoph Hellwig wrote:
> > +static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> > + unsigned bytes, bool mp)
>
> I think these magic 'bool np' arguments and wrappers over wrapper
> don't help anyone to actually understand the code. I'd vote for
> removing as many wrappers as we really don't need, and passing the
> actual segment limit instead of the magic bool flag. Something like
> this untested patch:
I think this way is fine, just a little comment.
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 277921ad42e7..dcad0b69f57a 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -138,30 +138,21 @@ static inline bool bio_full(struct bio *bio)
> bvec_for_each_segment(bvl, &((bio)->bi_io_vec[iter_all.idx]), i, iter_all)
>
> static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> - unsigned bytes, bool mp)
> + unsigned bytes, unsigned max_segment)
The new parameter should have been named as 'max_segment_len' or
'max_seg_len'.
> {
> iter->bi_sector += bytes >> 9;
>
> if (bio_no_advance_iter(bio))
> iter->bi_size -= bytes;
> else
> - if (!mp)
> - bvec_iter_advance(bio->bi_io_vec, iter, bytes);
> - else
> - mp_bvec_iter_advance(bio->bi_io_vec, iter, bytes);
> + __bvec_iter_advance(bio->bi_io_vec, iter, bytes, max_segment);
> /* TODO: It is reasonable to complete bio with error here. */
> }
>
> static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> unsigned bytes)
> {
> - __bio_advance_iter(bio, iter, bytes, false);
> -}
> -
> -static inline void bio_advance_mp_iter(struct bio *bio, struct bvec_iter *iter,
> - unsigned bytes)
> -{
> - __bio_advance_iter(bio, iter, bytes, true);
> + __bio_advance_iter(bio, iter, bytes, PAGE_SIZE);
> }
>
> #define __bio_for_each_segment(bvl, bio, iter, start) \
> @@ -177,7 +168,7 @@ static inline void bio_advance_mp_iter(struct bio *bio, struct bvec_iter *iter,
> for (iter = (start); \
> (iter).bi_size && \
> ((bvl = bio_iter_mp_iovec((bio), (iter))), 1); \
> - bio_advance_mp_iter((bio), &(iter), (bvl).bv_len))
> + __bio_advance_iter((bio), &(iter), (bvl).bv_len, 0))
Even we might pass '-1' for multi-page segment.
>
> /* returns one real segment(multipage bvec) each time */
> #define bio_for_each_bvec(bvl, bio, iter) \
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index 02f26d2b59ad..5e2ed46c1c88 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -138,8 +138,7 @@ struct bvec_iter_all {
> })
>
> static inline bool __bvec_iter_advance(const struct bio_vec *bv,
> - struct bvec_iter *iter,
> - unsigned bytes, bool mp)
> + struct bvec_iter *iter, unsigned bytes, unsigned max_segment)
> {
> if (WARN_ONCE(bytes > iter->bi_size,
> "Attempted to advance past end of bvec iter\n")) {
> @@ -148,18 +147,18 @@ static inline bool __bvec_iter_advance(const struct bio_vec *bv,
> }
>
> while (bytes) {
> - unsigned len;
> + unsigned segment_len = mp_bvec_iter_len(bv, *iter);
>
> - if (mp)
> - len = mp_bvec_iter_len(bv, *iter);
> - else
> - len = bvec_iter_len(bv, *iter);
> + if (max_segment) {
> + max_segment -= bvec_iter_offset(bv, *iter);
> + segment_len = min(segment_len, max_segment);
Looks 'max_segment' needs to be constant, shouldn't be updated.
If '-1' is passed for multipage case, the above change may become:
segment_len = min_t(segment_len, max_seg_len - bvec_iter_offset(bv, *iter));
This way is more clean, but with extra cost of the above line for multipage
case.
Thanks,
Ming
^ permalink raw reply [flat|nested] 103+ messages in thread
* [PATCH V10 03/19] block: use bio_for_each_bvec() to compute multi-page bvec count
2018-11-15 8:52 [PATCH V10 00/19] block: support multi-page bvec Ming Lei
2018-11-15 8:52 ` [PATCH V10 01/19] block: introduce multi-page page bvec helpers Ming Lei
2018-11-15 8:52 ` [PATCH V10 02/19] block: introduce bio_for_each_bvec() Ming Lei
@ 2018-11-15 8:52 ` Ming Lei
2018-11-15 20:20 ` Omar Sandoval
2018-11-15 8:52 ` [PATCH V10 04/19] block: use bio_for_each_bvec() to map sg Ming Lei
` (16 subsequent siblings)
19 siblings, 1 reply; 103+ messages in thread
From: Ming Lei @ 2018-11-15 8:52 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, linux-kernel, linux-mm, Ming Lei, Dave Chinner,
Kent Overstreet, Mike Snitzer, dm-devel, Alexander Viro,
linux-fsdevel, Shaohua Li, linux-raid, linux-erofs, David Sterba,
linux-btrfs, Darrick J . Wong, linux-xfs, Gao Xiang,
Christoph Hellwig, Theodore Ts'o, linux-ext4, Coly Li,
linux-bcache, Boaz Harrosh, Bob Peterson, cluster-devel
First it is more efficient to use bio_for_each_bvec() in both
blk_bio_segment_split() and __blk_recalc_rq_segments() to compute how
many multi-page bvecs there are in the bio.
Secondly once bio_for_each_bvec() is used, the bvec may need to be
splitted because its length can be very longer than max segment size,
so we have to split the big bvec into several segments.
Thirdly when splitting multi-page bvec into segments, the max segment
limit may be reached, so the bio split need to be considered under
this situation too.
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Kent Overstreet <kent.overstreet@gmail.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: Shaohua Li <shli@kernel.org>
Cc: linux-raid@vger.kernel.org
Cc: linux-erofs@lists.ozlabs.org
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Cc: Gao Xiang <gaoxiang25@huawei.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: linux-ext4@vger.kernel.org
Cc: Coly Li <colyli@suse.de>
Cc: linux-bcache@vger.kernel.org
Cc: Boaz Harrosh <ooo@electrozaur.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: cluster-devel@redhat.com
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-merge.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 76 insertions(+), 14 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 91b2af332a84..6f7deb94a23f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -160,6 +160,62 @@ static inline unsigned get_max_io_size(struct request_queue *q,
return sectors;
}
+/*
+ * Split the bvec @bv into segments, and update all kinds of
+ * variables.
+ */
+static bool bvec_split_segs(struct request_queue *q, struct bio_vec *bv,
+ unsigned *nsegs, unsigned *last_seg_size,
+ unsigned *front_seg_size, unsigned *sectors)
+{
+ bool need_split = false;
+ unsigned len = bv->bv_len;
+ unsigned total_len = 0;
+ unsigned new_nsegs = 0, seg_size = 0;
+
+ if ((*nsegs >= queue_max_segments(q)) || !len)
+ return need_split;
+
+ /*
+ * Multipage bvec may be too big to hold in one segment,
+ * so the current bvec has to be splitted as multiple
+ * segments.
+ */
+ while (new_nsegs + *nsegs < queue_max_segments(q)) {
+ seg_size = min(queue_max_segment_size(q), len);
+
+ new_nsegs++;
+ total_len += seg_size;
+ len -= seg_size;
+
+ if ((queue_virt_boundary(q) && ((bv->bv_offset +
+ total_len) & queue_virt_boundary(q))) || !len)
+ break;
+ }
+
+ /* split in the middle of the bvec */
+ if (len)
+ need_split = true;
+
+ /* update front segment size */
+ if (!*nsegs) {
+ unsigned first_seg_size = seg_size;
+
+ if (new_nsegs > 1)
+ first_seg_size = queue_max_segment_size(q);
+ if (*front_seg_size < first_seg_size)
+ *front_seg_size = first_seg_size;
+ }
+
+ /* update other varibles */
+ *last_seg_size = seg_size;
+ *nsegs += new_nsegs;
+ if (sectors)
+ *sectors += total_len >> 9;
+
+ return need_split;
+}
+
static struct bio *blk_bio_segment_split(struct request_queue *q,
struct bio *bio,
struct bio_set *bs,
@@ -173,7 +229,7 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
struct bio *new = NULL;
const unsigned max_sectors = get_max_io_size(q, bio);
- bio_for_each_segment(bv, bio, iter) {
+ bio_for_each_bvec(bv, bio, iter) {
/*
* If the queue doesn't support SG gaps and adding this
* offset would create a gap, disallow it.
@@ -188,8 +244,12 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
*/
if (nsegs < queue_max_segments(q) &&
sectors < max_sectors) {
- nsegs++;
- sectors = max_sectors;
+ /* split in the middle of bvec */
+ bv.bv_len = (max_sectors - sectors) << 9;
+ bvec_split_segs(q, &bv, &nsegs,
+ &seg_size,
+ &front_seg_size,
+ §ors);
}
goto split;
}
@@ -214,11 +274,12 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
if (nsegs == 1 && seg_size > front_seg_size)
front_seg_size = seg_size;
- nsegs++;
bvprv = bv;
bvprvp = &bvprv;
- seg_size = bv.bv_len;
- sectors += bv.bv_len >> 9;
+
+ if (bvec_split_segs(q, &bv, &nsegs, &seg_size,
+ &front_seg_size, §ors))
+ goto split;
}
@@ -296,6 +357,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
struct bio_vec bv, bvprv = { NULL };
int cluster, prev = 0;
unsigned int seg_size, nr_phys_segs;
+ unsigned front_seg_size = bio->bi_seg_front_size;
struct bio *fbio, *bbio;
struct bvec_iter iter;
@@ -316,7 +378,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
seg_size = 0;
nr_phys_segs = 0;
for_each_bio(bio) {
- bio_for_each_segment(bv, bio, iter) {
+ bio_for_each_bvec(bv, bio, iter) {
/*
* If SG merging is disabled, each bio vector is
* a segment
@@ -336,20 +398,20 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
continue;
}
new_segment:
- if (nr_phys_segs == 1 && seg_size >
- fbio->bi_seg_front_size)
- fbio->bi_seg_front_size = seg_size;
+ if (nr_phys_segs == 1 && seg_size > front_seg_size)
+ front_seg_size = seg_size;
- nr_phys_segs++;
bvprv = bv;
prev = 1;
- seg_size = bv.bv_len;