| Message ID | 20220523210119.2500150-6-kbusch@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | direct io dma alignment | expand |
On Mon, May 23, 2022 at 02:01:18PM -0700, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > Individual bv_len's may not be a sector size. > > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > block/bounce.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/block/bounce.c b/block/bounce.c > index 8f7b6fe3b4db..20a43c4dbdda 100644 > --- a/block/bounce.c > +++ b/block/bounce.c > @@ -207,17 +207,18 @@ void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig) > struct bvec_iter iter; > unsigned i = 0; > bool bounce = false; > - int sectors = 0; > + int sectors = 0, bytes = 0; > > bio_for_each_segment(from, *bio_orig, iter) { > if (i++ < BIO_MAX_VECS) > - sectors += from.bv_len >> 9; > + bytes += from.bv_len; > if (PageHighMem(from.bv_page)) > bounce = true; > } > if (!bounce) > return; > > + sectors = ALIGN_DOWN(bytes, queue_logical_block_size(q)) >> 9; Same comment about SECTOR_SHIFT and a comment here. That being said, why do we even align here? Shouldn't bytes always be setor aligned here and this should be a WARN_ON or other sanity check? Probably the same for the previous patch.
On Mon, May 23, 2022 at 02:01:18PM -0700, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > Individual bv_len's may not be a sector size. > > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > block/bounce.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/block/bounce.c b/block/bounce.c > index 8f7b6fe3b4db..20a43c4dbdda 100644 > --- a/block/bounce.c > +++ b/block/bounce.c > @@ -207,17 +207,18 @@ void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig) > struct bvec_iter iter; > unsigned i = 0; > bool bounce = false; > - int sectors = 0; > + int sectors = 0, bytes = 0; > > bio_for_each_segment(from, *bio_orig, iter) { > if (i++ < BIO_MAX_VECS) > - sectors += from.bv_len >> 9; > + bytes += from.bv_len; bv.len is unsigned int. bytes variable should also unsigned int to be on the safe side. > if (PageHighMem(from.bv_page)) > bounce = true; > } > if (!bounce) > return; > > + sectors = ALIGN_DOWN(bytes, queue_logical_block_size(q)) >> 9; > if (sectors < bio_sectors(*bio_orig)) { > bio = bio_split(*bio_orig, sectors, GFP_NOIO, &bounce_bio_split); > bio_chain(bio, *bio_orig); > -- > 2.30.2 >
On Tue, May 24, 2022 at 08:09:21AM +0200, Christoph Hellwig wrote: > On Mon, May 23, 2022 at 02:01:18PM -0700, Keith Busch wrote: > > From: Keith Busch <kbusch@kernel.org> > > > > Individual bv_len's may not be a sector size. > > > > Signed-off-by: Keith Busch <kbusch@kernel.org> > > --- > > block/bounce.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/block/bounce.c b/block/bounce.c > > index 8f7b6fe3b4db..20a43c4dbdda 100644 > > --- a/block/bounce.c > > +++ b/block/bounce.c > > @@ -207,17 +207,18 @@ void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig) > > struct bvec_iter iter; > > unsigned i = 0; > > bool bounce = false; > > - int sectors = 0; > > + int sectors = 0, bytes = 0; > > > > bio_for_each_segment(from, *bio_orig, iter) { > > if (i++ < BIO_MAX_VECS) > > - sectors += from.bv_len >> 9; > > + bytes += from.bv_len; > > if (PageHighMem(from.bv_page)) > > bounce = true; > > } > > if (!bounce) > > return; > > > > + sectors = ALIGN_DOWN(bytes, queue_logical_block_size(q)) >> 9; > > Same comment about SECTOR_SHIFT and a comment here. That being said, > why do we even align here? Shouldn't bytes always be setor aligned here > and this should be a WARN_ON or other sanity check? Probably the same > for the previous patch. Yes, the total bytes should be sector aligned. I'm not exactly sure why we're counting it this way, though. We could just skip the iterative addition and get the total from bio->bi_iter.bi_size. Unless bio_orig has more segments that BIO_MAX_VECS, which doesn't seem possible.
On Wed, May 25, 2022 at 08:08:07AM -0600, Keith Busch wrote: > On Tue, May 24, 2022 at 08:09:21AM +0200, Christoph Hellwig wrote: > > > bio_for_each_segment(from, *bio_orig, iter) { > > > if (i++ < BIO_MAX_VECS) > > > - sectors += from.bv_len >> 9; > > > + bytes += from.bv_len; > > > if (PageHighMem(from.bv_page)) > > > bounce = true; > > > } > > > if (!bounce) > > > return; > > > > > > + sectors = ALIGN_DOWN(bytes, queue_logical_block_size(q)) >> 9; > > > > Same comment about SECTOR_SHIFT and a comment here. That being said, > > why do we even align here? Shouldn't bytes always be setor aligned here > > and this should be a WARN_ON or other sanity check? Probably the same > > for the previous patch. > > Yes, the total bytes should be sector aligned. > > I'm not exactly sure why we're counting it this way, though. We could just skip > the iterative addition and get the total from bio->bi_iter.bi_size. Unless > bio_orig has more segments that BIO_MAX_VECS, which doesn't seem possible. Oh, there's a comment explaining the original can have more than BIO_MAX_VECS, so the ALIGN_DOWN is necessary to ensure a logical block sized split.
diff --git a/block/bounce.c b/block/bounce.c index 8f7b6fe3b4db..20a43c4dbdda 100644 --- a/block/bounce.c +++ b/block/bounce.c @@ -207,17 +207,18 @@ void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig) struct bvec_iter iter; unsigned i = 0; bool bounce = false; - int sectors = 0; + int sectors = 0, bytes = 0; bio_for_each_segment(from, *bio_orig, iter) { if (i++ < BIO_MAX_VECS) - sectors += from.bv_len >> 9; + bytes += from.bv_len; if (PageHighMem(from.bv_page)) bounce = true; } if (!bounce) return; + sectors = ALIGN_DOWN(bytes, queue_logical_block_size(q)) >> 9; if (sectors < bio_sectors(*bio_orig)) { bio = bio_split(*bio_orig, sectors, GFP_NOIO, &bounce_bio_split); bio_chain(bio, *bio_orig);