Re: [PATCH v2 4/4] block/mq-deadline: Prevent zoned write reordering due to I/O prioritization

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

On 12/20/23 02:42, Bart Van Assche wrote:
> On 12/19/23 04:10, Christoph Hellwig wrote:
>> On Mon, Dec 18, 2023 at 01:13:42PM -0800, Bart Van Assche wrote:
>>> Assigning I/O priorities with the ioprio cgroup policy may cause
>>> different I/O priorities to be assigned to write requests for the same
>>> zone. Prevent that this causes unaligned write errors by adding zoned
>>> writes for the same zone in the same priority queue as prior zoned
>>> writes.
>>
>> I still think this is fundamentally the wrong thing to do.  If you set
>> different priorities, you want I/O to be reordered, so ignoring that
>> is a bad thing.
> 
> Hi Christoph,
> 
> How about not setting the I/O priority of sequential zoned writes as in
> the (untested) patch below?
> 
> Thanks,
> 
> Bart.
> 
> 
> [PATCH] block: Do not set the I/O priority for sequential zoned writes
> 
> ---
>   block/blk-mq.c         |  7 +++++++
>   include/linux/blk-mq.h | 17 +++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index c11c97afa0bc..668888103a47 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2922,6 +2922,13 @@ static bool blk_mq_can_use_cached_rq(struct request *rq, struct blk_plug *plug,
> 
>   static void bio_set_ioprio(struct bio *bio)
>   {
> +	/*
> +	 * Do not set the I/O priority of sequential zoned write bios because
> +	 * this could lead to reordering and hence to unaligned write errors.
> +	 */
> +	if (blk_bio_is_seq_zoned_write(bio))
> +		return;

That is not acceptable as that will ignore priorities passed for async direct
IOs through aio->aio_reqprio. That one is a perfectly acceptable use case and we
should not ignore it.

> +
>   	/* Nobody set ioprio so far? Initialize it based on task's nice value */
>   	if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE)
>   		bio->bi_ioprio = get_current_ioprio();
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 1ab3081c82ed..e7fa81170b7c 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -1149,6 +1149,18 @@ static inline unsigned int blk_rq_zone_no(struct request *rq)
>   	return disk_zone_no(rq->q->disk, blk_rq_pos(rq));
>   }
> 
> +/**
> + * blk_bio_is_seq_zoned_write() - Check if @bio requires write serialization.
> + * @bio: Bio to examine.
> + *
> + * Note: REQ_OP_ZONE_APPEND bios do not require serialization.
> + */
> +static inline bool blk_bio_is_seq_zoned_write(struct bio *bio)
> +{
> +	return disk_zone_is_seq(bio->bi_bdev->bd_disk, bio->bi_iter.bi_sector) &&
> +		op_needs_zoned_write_locking(bio_op(bio));
> +}
> +
>   static inline unsigned int blk_rq_zone_is_seq(struct request *rq)
>   {
>   	return disk_zone_is_seq(rq->q->disk, blk_rq_pos(rq));
> @@ -1196,6 +1208,11 @@ static inline bool blk_req_can_dispatch_to_zone(struct request *rq)
>   	return !blk_req_zone_is_write_locked(rq);
>   }
>   #else /* CONFIG_BLK_DEV_ZONED */
> +static inline bool blk_bio_is_seq_zoned_write(struct bio *bio)
> +{
> +	return false;
> +}
> +
>   static inline bool blk_rq_is_seq_zoned_write(struct request *rq)
>   {
>   	return false;
> 

-- 
Damien Le Moal
Western Digital Research