Re: [Cluster-devel] [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE
On Fri, Nov 16, 2018 at 02:58:03PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 15, 2018 at 04:53:05PM +0800, Ming Lei wrote:
> > Since bdced438acd83ad83a6c ("block: setup bi_phys_segments after
> > splitting"),
> > physical segment number is mainly figured out in blk_queue_split() for
> > fast path, and the flag of BIO_SEG_VALID is set there too.
> >
> > Now only blk_recount_segments() and blk_recalc_rq_segments() use this
> > flag.
> >
> > Basically blk_recount_segments() is bypassed in fast path given
> > BIO_SEG_VALID
> > is set in blk_queue_split().
> >
> > For another user of blk_recalc_rq_segments():
> >
> > - run in partial completion branch of blk_update_request, which is an
> > unusual case
> >
> > - run in blk_cloned_rq_check_limits(), still not a big problem if the flag
> > is killed
> > since dm-rq is the only user.
> >
> > Multi-page bvec is enabled now, QUEUE_FLAG_NO_SG_MERGE doesn't make sense
> > any more.
> >
> > Cc: Dave Chinner
> > Cc: Kent Overstreet
> > Cc: Mike Snitzer
> > Cc: dm-de...@redhat.com
> > Cc: Alexander Viro
> > Cc: linux-fsde...@vger.kernel.org
> > Cc: Shaohua Li
> > Cc: linux-r...@vger.kernel.org
> > Cc: linux-er...@lists.ozlabs.org
> > Cc: David Sterba
> > Cc: linux-bt...@vger.kernel.org
> > Cc: Darrick J. Wong
> > Cc: linux-...@vger.kernel.org
> > Cc: Gao Xiang
> > Cc: Christoph Hellwig
> > Cc: Theodore Ts'o
> > Cc: linux-e...@vger.kernel.org
> > Cc: Coly Li
> > Cc: linux-bca...@vger.kernel.org
> > Cc: Boaz Harrosh
> > Cc: Bob Peterson
> > Cc: cluster-devel@redhat.com
> > Signed-off-by: Ming Lei
> > ---
> > block/blk-merge.c | 31 ++-
> > block/blk-mq-debugfs.c | 1 -
> > block/blk-mq.c | 3 ---
> > drivers/md/dm-table.c | 13 -
> > include/linux/blkdev.h | 1 -
> > 5 files changed, 6 insertions(+), 43 deletions(-)
> >
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index 153a659fde74..06be298be332 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -351,8 +351,7 @@ void blk_queue_split(struct request_queue *q, struct
> > bio **bio)
> > EXPORT_SYMBOL(blk_queue_split);
> >
> > static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
> > -struct bio *bio,
> > -bool no_sg_merge)
> > +struct bio *bio)
> > {
> > struct bio_vec bv, bvprv = { NULL };
> > int cluster, prev = 0;
> > @@ -379,13 +378,6 @@ static unsigned int __blk_recalc_rq_segments(struct
> > request_queue *q,
> > nr_phys_segs = 0;
> > for_each_bio(bio) {
> > bio_for_each_bvec(bv, bio, iter) {
> > - /*
> > -* If SG merging is disabled, each bio vector is
> > -* a segment
> > -*/
> > - if (no_sg_merge)
> > - goto new_segment;
> > -
> > if (prev && cluster) {
> > if (seg_size + bv.bv_len
> > > queue_max_segment_size(q))
> > @@ -420,27 +412,16 @@ static unsigned int __blk_recalc_rq_segments(struct
> > request_queue *q,
> >
> > void blk_recalc_rq_segments(struct request *rq)
> > {
> > - bool no_sg_merge = !!test_bit(QUEUE_FLAG_NO_SG_MERGE,
> > - >q->queue_flags);
> > -
> > - rq->nr_phys_segments = __blk_recalc_rq_segments(rq->q, rq->bio,
> > - no_sg_merge);
> > + rq->nr_phys_segments = __blk_recalc_rq_segments(rq->q, rq->bio);
>
> Can we rename __blk_recalc_rq_segments to blk_recalc_rq_segments
> can kill the old blk_recalc_rq_segments now?
Sure.
Thanks,
Ming
Re: [Cluster-devel] [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE
On Thu, Nov 15, 2018 at 06:18:11PM -0800, Omar Sandoval wrote:
> On Thu, Nov 15, 2018 at 04:53:05PM +0800, Ming Lei wrote:
> > Since bdced438acd83ad83a6c ("block: setup bi_phys_segments after
> > splitting"),
> > physical segment number is mainly figured out in blk_queue_split() for
> > fast path, and the flag of BIO_SEG_VALID is set there too.
> >
> > Now only blk_recount_segments() and blk_recalc_rq_segments() use this
> > flag.
> >
> > Basically blk_recount_segments() is bypassed in fast path given
> > BIO_SEG_VALID
> > is set in blk_queue_split().
> >
> > For another user of blk_recalc_rq_segments():
> >
> > - run in partial completion branch of blk_update_request, which is an
> > unusual case
> >
> > - run in blk_cloned_rq_check_limits(), still not a big problem if the flag
> > is killed
> > since dm-rq is the only user.
> >
> > Multi-page bvec is enabled now, QUEUE_FLAG_NO_SG_MERGE doesn't make sense
> > any more.
>
> This commit message wasn't very clear. Is it the case that
> QUEUE_FLAG_NO_SG_MERGE is no longer set by any drivers?
OK, I will add the explanation to commit log in next version.
05f1dd53152173 (block: add queue flag for disabling SG merging) introduces this
flag for NVMe performance purpose only, so that merging to segment can
be bypassed for NVMe.
Actually this optimization was bypassed by 54efd50bfd873e2d (block: make
generic_make_request handle arbitrarily sized bios) and bdced438acd83ad83a6c
("block: setup bi_phys_segments after splitting").
Now segment computation can be very quick, given most of times one bvec
can be thought as one segment, so we can remove the flag.
thanks,
Ming
Re: [Cluster-devel] [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE
On Fri, Nov 16, 2018 at 02:59:22PM +0100, Christoph Hellwig wrote: > On Thu, Nov 15, 2018 at 06:18:11PM -0800, Omar Sandoval wrote: > > This commit message wasn't very clear. Is it the case that > > QUEUE_FLAG_NO_SG_MERGE is no longer set by any drivers? > > I think he wants to say that not doing S/G merging is rather pointless > with the current setup of the I/O path, as it isn't going to save > you a significant amount of cycles. Okay, that makes sense. Ming, you can add Reviewed-by: Omar Sandoval
Re: [Cluster-devel] [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE
On Thu, Nov 15, 2018 at 06:18:11PM -0800, Omar Sandoval wrote: > This commit message wasn't very clear. Is it the case that > QUEUE_FLAG_NO_SG_MERGE is no longer set by any drivers? I think he wants to say that not doing S/G merging is rather pointless with the current setup of the I/O path, as it isn't going to save you a significant amount of cycles.
Re: [Cluster-devel] [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE
On Thu, Nov 15, 2018 at 04:53:05PM +0800, Ming Lei wrote:
> Since bdced438acd83ad83a6c ("block: setup bi_phys_segments after splitting"),
> physical segment number is mainly figured out in blk_queue_split() for
> fast path, and the flag of BIO_SEG_VALID is set there too.
>
> Now only blk_recount_segments() and blk_recalc_rq_segments() use this
> flag.
>
> Basically blk_recount_segments() is bypassed in fast path given BIO_SEG_VALID
> is set in blk_queue_split().
>
> For another user of blk_recalc_rq_segments():
>
> - run in partial completion branch of blk_update_request, which is an unusual
> case
>
> - run in blk_cloned_rq_check_limits(), still not a big problem if the flag is
> killed
> since dm-rq is the only user.
>
> Multi-page bvec is enabled now, QUEUE_FLAG_NO_SG_MERGE doesn't make sense any
> more.
>
> Cc: Dave Chinner
> Cc: Kent Overstreet
> Cc: Mike Snitzer
> Cc: dm-de...@redhat.com
> Cc: Alexander Viro
> Cc: linux-fsde...@vger.kernel.org
> Cc: Shaohua Li
> Cc: linux-r...@vger.kernel.org
> Cc: linux-er...@lists.ozlabs.org
> Cc: David Sterba
> Cc: linux-bt...@vger.kernel.org
> Cc: Darrick J. Wong
> Cc: linux-...@vger.kernel.org
> Cc: Gao Xiang
> Cc: Christoph Hellwig
> Cc: Theodore Ts'o
> Cc: linux-e...@vger.kernel.org
> Cc: Coly Li
> Cc: linux-bca...@vger.kernel.org
> Cc: Boaz Harrosh
> Cc: Bob Peterson
> Cc: cluster-devel@redhat.com
> Signed-off-by: Ming Lei
> ---
> block/blk-merge.c | 31 ++-
> block/blk-mq-debugfs.c | 1 -
> block/blk-mq.c | 3 ---
> drivers/md/dm-table.c | 13 -
> include/linux/blkdev.h | 1 -
> 5 files changed, 6 insertions(+), 43 deletions(-)
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 153a659fde74..06be298be332 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -351,8 +351,7 @@ void blk_queue_split(struct request_queue *q, struct bio
> **bio)
> EXPORT_SYMBOL(blk_queue_split);
>
> static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
> - struct bio *bio,
> - bool no_sg_merge)
> + struct bio *bio)
> {
> struct bio_vec bv, bvprv = { NULL };
> int cluster, prev = 0;
> @@ -379,13 +378,6 @@ static unsigned int __blk_recalc_rq_segments(struct
> request_queue *q,
> nr_phys_segs = 0;
> for_each_bio(bio) {
> bio_for_each_bvec(bv, bio, iter) {
> - /*
> - * If SG merging is disabled, each bio vector is
> - * a segment
> - */
> - if (no_sg_merge)
> - goto new_segment;
> -
> if (prev && cluster) {
> if (seg_size + bv.bv_len
> > queue_max_segment_size(q))
> @@ -420,27 +412,16 @@ static unsigned int __blk_recalc_rq_segments(struct
> request_queue *q,
>
> void blk_recalc_rq_segments(struct request *rq)
> {
> - bool no_sg_merge = !!test_bit(QUEUE_FLAG_NO_SG_MERGE,
> - >q->queue_flags);
> -
> - rq->nr_phys_segments = __blk_recalc_rq_segments(rq->q, rq->bio,
> - no_sg_merge);
> + rq->nr_phys_segments = __blk_recalc_rq_segments(rq->q, rq->bio);
Can we rename __blk_recalc_rq_segments to blk_recalc_rq_segments
can kill the old blk_recalc_rq_segments now?
Otherwise looks fine:
Reviewed-by: Christoph Hellwig
Re: [Cluster-devel] [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE
On Thu, Nov 15, 2018 at 04:53:05PM +0800, Ming Lei wrote:
> Since bdced438acd83ad83a6c ("block: setup bi_phys_segments after splitting"),
> physical segment number is mainly figured out in blk_queue_split() for
> fast path, and the flag of BIO_SEG_VALID is set there too.
>
> Now only blk_recount_segments() and blk_recalc_rq_segments() use this
> flag.
>
> Basically blk_recount_segments() is bypassed in fast path given BIO_SEG_VALID
> is set in blk_queue_split().
>
> For another user of blk_recalc_rq_segments():
>
> - run in partial completion branch of blk_update_request, which is an unusual
> case
>
> - run in blk_cloned_rq_check_limits(), still not a big problem if the flag is
> killed
> since dm-rq is the only user.
>
> Multi-page bvec is enabled now, QUEUE_FLAG_NO_SG_MERGE doesn't make sense any
> more.
This commit message wasn't very clear. Is it the case that
QUEUE_FLAG_NO_SG_MERGE is no longer set by any drivers?
[Cluster-devel] [PATCH V10 18/19] block: kill QUEUE_FLAG_NO_SG_MERGE
Since bdced438acd83ad83a6c ("block: setup bi_phys_segments after splitting"),
physical segment number is mainly figured out in blk_queue_split() for
fast path, and the flag of BIO_SEG_VALID is set there too.
Now only blk_recount_segments() and blk_recalc_rq_segments() use this
flag.
Basically blk_recount_segments() is bypassed in fast path given BIO_SEG_VALID
is set in blk_queue_split().
For another user of blk_recalc_rq_segments():
- run in partial completion branch of blk_update_request, which is an unusual
case
- run in blk_cloned_rq_check_limits(), still not a big problem if the flag is
killed
since dm-rq is the only user.
Multi-page bvec is enabled now, QUEUE_FLAG_NO_SG_MERGE doesn't make sense any
more.
Cc: Dave Chinner
Cc: Kent Overstreet
Cc: Mike Snitzer
Cc: dm-de...@redhat.com
Cc: Alexander Viro
Cc: linux-fsde...@vger.kernel.org
Cc: Shaohua Li
Cc: linux-r...@vger.kernel.org
Cc: linux-er...@lists.ozlabs.org
Cc: David Sterba
Cc: linux-bt...@vger.kernel.org
Cc: Darrick J. Wong
Cc: linux-...@vger.kernel.org
Cc: Gao Xiang
Cc: Christoph Hellwig
Cc: Theodore Ts'o
Cc: linux-e...@vger.kernel.org
Cc: Coly Li
Cc: linux-bca...@vger.kernel.org
Cc: Boaz Harrosh
Cc: Bob Peterson
Cc: cluster-devel@redhat.com
Signed-off-by: Ming Lei
---
block/blk-merge.c | 31 ++-
block/blk-mq-debugfs.c | 1 -
block/blk-mq.c | 3 ---
drivers/md/dm-table.c | 13 -
include/linux/blkdev.h | 1 -
5 files changed, 6 insertions(+), 43 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 153a659fde74..06be298be332 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -351,8 +351,7 @@ void blk_queue_split(struct request_queue *q, struct bio
**bio)
EXPORT_SYMBOL(blk_queue_split);
static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
-struct bio *bio,
-bool no_sg_merge)
+struct bio *bio)
{
struct bio_vec bv, bvprv = { NULL };
int cluster, prev = 0;
@@ -379,13 +378,6 @@ static unsigned int __blk_recalc_rq_segments(struct
request_queue *q,
nr_phys_segs = 0;
for_each_bio(bio) {
bio_for_each_bvec(bv, bio, iter) {
- /*
-* If SG merging is disabled, each bio vector is
-* a segment
-*/
- if (no_sg_merge)
- goto new_segment;
-
if (prev && cluster) {
if (seg_size + bv.bv_len
> queue_max_segment_size(q))
@@ -420,27 +412,16 @@ static unsigned int __blk_recalc_rq_segments(struct
request_queue *q,
void blk_recalc_rq_segments(struct request *rq)
{
- bool no_sg_merge = !!test_bit(QUEUE_FLAG_NO_SG_MERGE,
- >q->queue_flags);
-
- rq->nr_phys_segments = __blk_recalc_rq_segments(rq->q, rq->bio,
- no_sg_merge);
+ rq->nr_phys_segments = __blk_recalc_rq_segments(rq->q, rq->bio);
}
void blk_recount_segments(struct request_queue *q, struct bio *bio)
{
- unsigned short seg_cnt = bio_segments(bio);
-
- if (test_bit(QUEUE_FLAG_NO_SG_MERGE, >queue_flags) &&
- (seg_cnt < queue_max_segments(q)))
- bio->bi_phys_segments = seg_cnt;
- else {
- struct bio *nxt = bio->bi_next;
+ struct bio *nxt = bio->bi_next;
- bio->bi_next = NULL;
- bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, false);
- bio->bi_next = nxt;
- }
+ bio->bi_next = NULL;
+ bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio);
+ bio->bi_next = nxt;
bio_set_flag(bio, BIO_SEG_VALID);
}
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index f021f4817b80..e188b1090759 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -128,7 +128,6 @@ static const char *const blk_queue_flag_name[] = {
QUEUE_FLAG_NAME(SAME_FORCE),
QUEUE_FLAG_NAME(DEAD),
QUEUE_FLAG_NAME(INIT_DONE),
- QUEUE_FLAG_NAME(NO_SG_MERGE),
QUEUE_FLAG_NAME(POLL),
QUEUE_FLAG_NAME(WC),
QUEUE_FLAG_NAME(FUA),
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 411be60d0cb6..ed484af5744b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2755,9 +2755,6 @@ struct request_queue *blk_mq_init_allocated_queue(struct
blk_mq_tag_set *set,
q->queue_flags |= QUEUE_FLAG_MQ_DEFAULT;
- if (!(set->flags & BLK_MQ_F_SG_MERGE))
- queue_flag_set_unlocked(QUEUE_FLAG_NO_SG_MERGE, q);
-
q->sg_reserved_size = INT_MAX;
INIT_DELAYED_WORK(>requeue_work, blk_mq_requeue_work);
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 9038c302d5c2..22fed6987aea 100644
---
