| Message ID | 20240705125700.2174367-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] block: also check bio alignment for bio based drivers | expand |
On Fri, Jul 05, 2024 at 02:56:50PM +0200, Christoph Hellwig wrote: > Extend the checks added in 0676c434a99b ("block: check bio alignment > in blk_mq_submit_bio") for blk-mq drivers to bio based drivers as > all the same reasons apply for them as well. Do we have bio based driver which may re-configure logical block size? If yes, is it enough to do so? Cause queue usage counter is only held during bio submission, and it won't cover the whole bio lifetime. Thanks, Ming
On 05/07/2024 13:56, Christoph Hellwig wrote: > Extend the checks added in 0676c434a99b ("block: check bio alignment > in blk_mq_submit_bio") for blk-mq drivers to bio based drivers as > all the same reasons apply for them as well. So do we now still need blkdev_dio_invalid() -> bdev_logical_block_size() pos checks for simple and async paths in fops? They are not doing harm and messy to remove, I suppose (if strictly not required). > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-core.c | 7 +++++-- > block/blk-mq.c | 11 ----------- > block/blk.h | 11 +++++++++++ > 3 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 71b7622c523a30..ffb55d8843a121 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -615,8 +615,11 @@ static void __submit_bio(struct bio *bio) > blk_mq_submit_bio(bio); > } else if (likely(bio_queue_enter(bio) == 0)) { > struct gendisk *disk = bio->bi_bdev->bd_disk; > - > - disk->fops->submit_bio(bio); > + > + if (unlikely(bio_unaligned(bio, disk->queue))) > + bio_io_error(bio); > + else > + disk->fops->submit_bio(bio); > blk_queue_exit(disk->queue); > } > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index e3c3c0c21b5536..94a102abb88055 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -2909,17 +2909,6 @@ static void blk_mq_use_cached_rq(struct request *rq, struct blk_plug *plug, > INIT_LIST_HEAD(&rq->queuelist); > } > > -static bool bio_unaligned(const struct bio *bio, struct request_queue *q) > -{ > - unsigned int bs_mask = queue_logical_block_size(q) - 1; > - > - /* .bi_sector of any zero sized bio need to be initialized */ > - if ((bio->bi_iter.bi_size & bs_mask) || > - ((bio->bi_iter.bi_sector << SECTOR_SHIFT) & bs_mask)) > - return true; > - return false; > -} > - > /** > * blk_mq_submit_bio - Create and send a request to block device. > * @bio: Bio pointer. > diff --git a/block/blk.h b/block/blk.h > index 8e8936e97307c6..d099ef1df5f64a 100644 > --- a/block/blk.h > +++ b/block/blk.h > @@ -40,6 +40,17 @@ int __bio_queue_enter(struct request_queue *q, struct bio *bio); > void submit_bio_noacct_nocheck(struct bio *bio); > void bio_await_chain(struct bio *bio); > > +static inline bool bio_unaligned(const struct bio *bio, struct request_queue *q) > +{ > + unsigned int bs_mask = queue_logical_block_size(q) - 1; > + > + /* .bi_sector of any zero sized bio need to be initialized */ > + if ((bio->bi_iter.bi_size & bs_mask) || > + ((bio->bi_iter.bi_sector << SECTOR_SHIFT) & bs_mask)) > + return true; > + return false; > +} > + > static inline bool blk_try_enter_queue(struct request_queue *q, bool pm) > { > rcu_read_lock();
On Fri, Jul 05, 2024 at 09:22:35PM +0800, Ming Lei wrote: > On Fri, Jul 05, 2024 at 02:56:50PM +0200, Christoph Hellwig wrote: > > Extend the checks added in 0676c434a99b ("block: check bio alignment > > in blk_mq_submit_bio") for blk-mq drivers to bio based drivers as > > all the same reasons apply for them as well. > > Do we have bio based driver which may re-configure logical block size? nvme multipath can, and it looks drbd might be able to do so as well. > If yes, is it enough to do so? Cause queue usage counter is only held > during bio submission, and it won't cover the whole bio lifetime. Yes. But for me the prime intend here is not to prevent that, but to ensure we actually have the damn sanity check for all drivers instead of just a few and instead a gazillion more or less equivalent open coded versions. That doesn't mean we shouldn't look into actually holding q_usage_count over the entire bio lifetime for bio based drivers, but that's a separate project.
On Fri, Jul 05, 2024 at 02:25:59PM +0100, John Garry wrote: > On 05/07/2024 13:56, Christoph Hellwig wrote: >> Extend the checks added in 0676c434a99b ("block: check bio alignment >> in blk_mq_submit_bio") for blk-mq drivers to bio based drivers as >> all the same reasons apply for them as well. > > So do we now still need blkdev_dio_invalid() -> bdev_logical_block_size() > pos checks for simple and async paths in fops? > > They are not doing harm and messy to remove, I suppose (if strictly not > required). Going all the way down into the block layer vs just doing the trivial check ahead of time does not sound like a winning proposition..
On Fri, Jul 05, 2024 at 03:36:30PM +0200, Christoph Hellwig wrote: > On Fri, Jul 05, 2024 at 09:22:35PM +0800, Ming Lei wrote: > > On Fri, Jul 05, 2024 at 02:56:50PM +0200, Christoph Hellwig wrote: > > > Extend the checks added in 0676c434a99b ("block: check bio alignment > > > in blk_mq_submit_bio") for blk-mq drivers to bio based drivers as > > > all the same reasons apply for them as well. > > > > Do we have bio based driver which may re-configure logical block size? > > nvme multipath can, and it looks drbd might be able to do so as well. nvme multipath should be fine since the check is done for the underlying nvme device. drbd doesn't call freeze queue, so it shouldn't have done that. > > > If yes, is it enough to do so? Cause queue usage counter is only held > > during bio submission, and it won't cover the whole bio lifetime. > > Yes. But for me the prime intend here is not to prevent that, but > to ensure we actually have the damn sanity check for all drivers > instead of just a few and instead a gazillion more or less equivalent > open coded versions. > > That doesn't mean we shouldn't look into actually holding q_usage_count > over the entire bio lifetime for bio based drivers, but that's a > separate project. What if logical block size is changed between bio submission and completion? For blk-mq device, we need to drain any IO when re-configuring device, however it can't be supported generically for bio based driver. Thanks, Ming
On Tue, Jul 09, 2024 at 09:50:48PM +0800, Ming Lei wrote: > > That doesn't mean we shouldn't look into actually holding q_usage_count > > over the entire bio lifetime for bio based drivers, but that's a > > separate project. > > What if logical block size is changed between bio submission and > completion? > > For blk-mq device, we need to drain any IO when re-configuring device, > however it can't be supported generically for bio based driver. Many bio based drivers do the same, just reimplemented without block helpers (e.g. md/dm). But as I said the point is that I really want the sanity check to always be there. I'd also like to eventually make freeze work for bio based drivers, but that is a separate issue.
diff --git a/block/blk-core.c b/block/blk-core.c index 71b7622c523a30..ffb55d8843a121 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -615,8 +615,11 @@ static void __submit_bio(struct bio *bio) blk_mq_submit_bio(bio); } else if (likely(bio_queue_enter(bio) == 0)) { struct gendisk *disk = bio->bi_bdev->bd_disk; - - disk->fops->submit_bio(bio); + + if (unlikely(bio_unaligned(bio, disk->queue))) + bio_io_error(bio); + else + disk->fops->submit_bio(bio); blk_queue_exit(disk->queue); } diff --git a/block/blk-mq.c b/block/blk-mq.c index e3c3c0c21b5536..94a102abb88055 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2909,17 +2909,6 @@ static void blk_mq_use_cached_rq(struct request *rq, struct blk_plug *plug, INIT_LIST_HEAD(&rq->queuelist); } -static bool bio_unaligned(const struct bio *bio, struct request_queue *q) -{ - unsigned int bs_mask = queue_logical_block_size(q) - 1; - - /* .bi_sector of any zero sized bio need to be initialized */ - if ((bio->bi_iter.bi_size & bs_mask) || - ((bio->bi_iter.bi_sector << SECTOR_SHIFT) & bs_mask)) - return true; - return false; -} - /** * blk_mq_submit_bio - Create and send a request to block device. * @bio: Bio pointer. diff --git a/block/blk.h b/block/blk.h index 8e8936e97307c6..d099ef1df5f64a 100644 --- a/block/blk.h +++ b/block/blk.h @@ -40,6 +40,17 @@ int __bio_queue_enter(struct request_queue *q, struct bio *bio); void submit_bio_noacct_nocheck(struct bio *bio); void bio_await_chain(struct bio *bio); +static inline bool bio_unaligned(const struct bio *bio, struct request_queue *q) +{ + unsigned int bs_mask = queue_logical_block_size(q) - 1; + + /* .bi_sector of any zero sized bio need to be initialized */ + if ((bio->bi_iter.bi_size & bs_mask) || + ((bio->bi_iter.bi_sector << SECTOR_SHIFT) & bs_mask)) + return true; + return false; +} + static inline bool blk_try_enter_queue(struct request_queue *q, bool pm) { rcu_read_lock();
Extend the checks added in 0676c434a99b ("block: check bio alignment in blk_mq_submit_bio") for blk-mq drivers to bio based drivers as all the same reasons apply for them as well. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-core.c | 7 +++++-- block/blk-mq.c | 11 ----------- block/blk.h | 11 +++++++++++ 3 files changed, 16 insertions(+), 13 deletions(-)