| Message ID | 20181115085306.9910-18-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: support multi-page bvec | expand |
On Thu, Nov 15, 2018 at 04:53:04PM +0800, Ming Lei wrote: > It is wrong to use bio->bi_vcnt to figure out how many segments > there are in the bio even though CLONED flag isn't set on this bio, > because this bio may be splitted or advanced. > > So always use bio_segments() in blk_recount_segments(), and it shouldn't > cause any performance loss now because the physical segment number is figured > out in blk_queue_split() and BIO_SEG_VALID is set meantime since > bdced438acd83ad83a6c ("block: setup bi_phys_segments after splitting"). > > Cc: Dave Chinner <dchinner@redhat.com> > Cc: Kent Overstreet <kent.overstreet@gmail.com> > Fixes: 7f60dcaaf91 ("block: blk-merge: fix blk_recount_segments()") From what I can tell, the problem was originally introduced by 76d8137a3113 ("blk-merge: recaculate segment if it isn't less than max segments") Is that right? > 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 | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/block/blk-merge.c b/block/blk-merge.c > index cb9f49bcfd36..153a659fde74 100644 > --- a/block/blk-merge.c > +++ b/block/blk-merge.c > @@ -429,13 +429,7 @@ void blk_recalc_rq_segments(struct request *rq) > > void blk_recount_segments(struct request_queue *q, struct bio *bio) > { > - unsigned short seg_cnt; > - > - /* estimate segment number by bi_vcnt for non-cloned bio */ > - if (bio_flagged(bio, BIO_CLONED)) > - seg_cnt = bio_segments(bio); > - else > - seg_cnt = bio->bi_vcnt; > + unsigned short seg_cnt = bio_segments(bio); > > if (test_bit(QUEUE_FLAG_NO_SG_MERGE, &q->queue_flags) && > (seg_cnt < queue_max_segments(q))) > -- > 2.9.5 >
On Thu, Nov 15, 2018 at 04:53:04PM +0800, Ming Lei wrote: > It is wrong to use bio->bi_vcnt to figure out how many segments > there are in the bio even though CLONED flag isn't set on this bio, > because this bio may be splitted or advanced. > > So always use bio_segments() in blk_recount_segments(), and it shouldn't > cause any performance loss now because the physical segment number is figured > out in blk_queue_split() and BIO_SEG_VALID is set meantime since > bdced438acd83ad83a6c ("block: setup bi_phys_segments after splitting"). Looks good, but shouldn't this go to the beginning of the series? Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Nov 15, 2018 at 06:11:40PM -0800, Omar Sandoval wrote: > On Thu, Nov 15, 2018 at 04:53:04PM +0800, Ming Lei wrote: > > It is wrong to use bio->bi_vcnt to figure out how many segments > > there are in the bio even though CLONED flag isn't set on this bio, > > because this bio may be splitted or advanced. > > > > So always use bio_segments() in blk_recount_segments(), and it shouldn't > > cause any performance loss now because the physical segment number is figured > > out in blk_queue_split() and BIO_SEG_VALID is set meantime since > > bdced438acd83ad83a6c ("block: setup bi_phys_segments after splitting"). > > > > Cc: Dave Chinner <dchinner@redhat.com> > > Cc: Kent Overstreet <kent.overstreet@gmail.com> > > Fixes: 7f60dcaaf91 ("block: blk-merge: fix blk_recount_segments()") > > From what I can tell, the problem was originally introduced by > 76d8137a3113 ("blk-merge: recaculate segment if it isn't less than max segments") > > Is that right? Indeed, will update it in next version. Thanks, Ming
diff --git a/block/blk-merge.c b/block/blk-merge.c index cb9f49bcfd36..153a659fde74 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -429,13 +429,7 @@ void blk_recalc_rq_segments(struct request *rq) void blk_recount_segments(struct request_queue *q, struct bio *bio) { - unsigned short seg_cnt; - - /* estimate segment number by bi_vcnt for non-cloned bio */ - if (bio_flagged(bio, BIO_CLONED)) - seg_cnt = bio_segments(bio); - else - seg_cnt = bio->bi_vcnt; + unsigned short seg_cnt = bio_segments(bio); if (test_bit(QUEUE_FLAG_NO_SG_MERGE, &q->queue_flags) && (seg_cnt < queue_max_segments(q)))
It is wrong to use bio->bi_vcnt to figure out how many segments there are in the bio even though CLONED flag isn't set on this bio, because this bio may be splitted or advanced. So always use bio_segments() in blk_recount_segments(), and it shouldn't cause any performance loss now because the physical segment number is figured out in blk_queue_split() and BIO_SEG_VALID is set meantime since bdced438acd83ad83a6c ("block: setup bi_phys_segments after splitting"). Cc: Dave Chinner <dchinner@redhat.com> Cc: Kent Overstreet <kent.overstreet@gmail.com> Fixes: 7f60dcaaf91 ("block: blk-merge: fix blk_recount_segments()") 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 | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)