Re: [PATCH] block: check bio alignment in blk_mq_submit_bio

[Ming Lei <ming.lei@redhat.com>]

On Wed, Jun 19, 2024 at 01:14:02PM +0900, Damien Le Moal wrote:
> On 6/19/24 12:34, Ming Lei wrote:
> > IO logical block size is one fundamental queue limit, and every IO has
> > to be aligned with logical block size because our bio split can't deal
> > with unaligned bio.
> > 
> > The check has to be done with queue usage counter grabbed because device
> > reconfiguration may change logical block size, and we can prevent the
> > reconfiguration from happening by holding queue usage counter.
> > 
> > logical_block_size stays in the 1st cache line of queue_limits, and this
> > cache line is always fetched in fast path via bio_may_exceed_limits(),
> > so IO perf won't be affected by this check.
> > 
> > Cc: Yi Zhang <yi.zhang@redhat.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Ye Bin <yebin10@huawei.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 3b4df8e5ac9e..7bb50b6b9567 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -2914,6 +2914,21 @@ 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,
> > +		const struct request_queue *q)
> > +{
> > +	unsigned int bs = queue_logical_block_size(q);
> > +
> > +	if (bio->bi_iter.bi_size & (bs - 1))
> > +		return true;
> > +
> > +	if (bio->bi_iter.bi_size &&
> > +	    ((bio->bi_iter.bi_sector << SECTOR_SHIFT) & (bs - 1)))
> 
> Hmmm... Some BIO operations have a 0 size but do specify a sector (e.g. zone
> management operations).

If we add the check for all type of IO, it requires ->bi_sector to
be meaningful for zero size bio. I am not sure if it is always true,
such as RESET_ALL.

> So this seems incorrect to me...

It is correct, but only cover bio with real ->bi_sector & ->bi_size.


Thanks,
Ming