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

[Damien Le Moal <dlemoal@kernel.org>]

On 6/19/24 13:14, 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). So this seems incorrect to me...

I meant to say, why not checking the sector alignment for these BIOs as well ?
Something like:

static bool bio_unaligned(const struct bio *bio,
		          const struct request_queue *q)
{
	unsigned int bs_mask = queue_logical_block_size(q) - 1;

	return (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.
>> @@ -2966,6 +2981,15 @@ void blk_mq_submit_bio(struct bio *bio)
>>  			return;
>>  	}
>>  
>> +	/*
>> +	 * Device reconfiguration may change logical block size, so alignment
>> +	 * check has to be done with queue usage counter held
>> +	 */
>> +	if (unlikely(bio_unaligned(bio, q))) {
>> +		bio_io_error(bio);
>> +		goto queue_exit;
>> +	}
>> +
>>  	if (unlikely(bio_may_exceed_limits(bio, &q->limits))) {
>>  		bio = __bio_split_to_limits(bio, &q->limits, &nr_segs);
>>  		if (!bio)
> 

-- 
Damien Le Moal
Western Digital Research