extend the bio alignment check to bio based drivers

extend the bio alignment check to bio based drivers

[Christoph Hellwig]

Hi Jens,

this series extends the bio alignment check in the block layer to cover
bio based drivers in addition to the blk-mq based path, and then drops
the equivalent debug checks in brd.

Diffstat:
 block/blk-core.c    |    7 +++++--
 block/blk-mq.c      |   11 -----------
 block/blk.h         |   11 +++++++++++
 drivers/block/brd.c |    4 ----
 4 files changed, 16 insertions(+), 17 deletions(-)

[PATCH 1/2] block: also check bio alignment for bio based drivers

[Christoph Hellwig]

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(-)

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();
-- 
2.43.0

[PATCH 2/2] brd: remove sector alignment checks

[Christoph Hellwig]

The block layer now takes care of these.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/brd.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 2fd1ed1017481b..42545e4ddfac4b 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -256,10 +256,6 @@ static void brd_submit_bio(struct bio *bio)
 		unsigned int len = bvec.bv_len;
 		int err;
 
-		/* Don't support un-aligned buffer */
-		WARN_ON_ONCE((bvec.bv_offset & (SECTOR_SIZE - 1)) ||
-				(len & (SECTOR_SIZE - 1)));
-
 		err = brd_do_bvec(brd, bvec.bv_page, len, bvec.bv_offset,
 				  bio->bi_opf, sector);
 		if (err) {
-- 
2.43.0

Re: [PATCH 1/2] block: also check bio alignment for bio based drivers

[Ming Lei]

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

Re: [PATCH 1/2] block: also check bio alignment for bio based drivers

[John Garry]

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();

Re: [PATCH 1/2] block: also check bio alignment for bio based drivers

[Christoph Hellwig]

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.

Re: [PATCH 1/2] block: also check bio alignment for bio based drivers

[Christoph Hellwig]

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..

Re: [PATCH 1/2] block: also check bio alignment for bio based drivers

[Ming Lei]

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

Re: [PATCH 1/2] block: also check bio alignment for bio based drivers

[Christoph Hellwig]

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.