Re: [PATCH v2 08/11] block: mq-deadline: Fix a race condition related to zoned writes

[Christoph Hellwig <hch@lst.de>]

On Tue, Apr 18, 2023 at 03:39:59PM -0700, Bart Van Assche wrote:
> Let deadline_next_request() only consider the first zoned write per
> zone. This patch fixes a race condition between deadline_next_request()
> and completion of zoned writes.

Can you explain the condition in a bit more detail?

>   * For the specified data direction, return the next request to
> @@ -386,9 +388,25 @@ deadline_next_request(struct deadline_data *dd, struct dd_per_prio *per_prio,
>  	 */
>  	spin_lock_irqsave(&dd->zone_lock, flags);
>  	while (rq) {
> +		unsigned int zno __maybe_unused;
> +
>  		if (blk_req_can_dispatch_to_zone(rq))
>  			break;
> +
> +#ifdef CONFIG_BLK_DEV_ZONED
> +		zno = blk_rq_zone_no(rq);
> +
>  		rq = deadline_skip_seq_writes(dd, rq);
> +
> +		/*
> +		 * Skip all other write requests for the zone with zone number
> +		 * 'zno'. This prevents that this function selects a zoned write
> +		 * that is not the first write for a given zone.
> +		 */
> +		while (rq && blk_rq_zone_no(rq) == zno &&
> +		       blk_rq_is_seq_zoned_write(rq))
> +			rq = deadline_latter_request(rq);
> +#endif

The ifdefere and extra checks are a bit ugly here.

I'd suggest to:

 - move all the zoned related logic in deadline_next_request into
   a separate helper that is stubbed out for !CONFIG_BLK_DEV_ZONED
 - merge the loop in deadline_skip_seq_writes and
   added here into one single loop.  Something like:

static struct request *
deadline_zoned_next_request(struct deadline_data *dd, struct request *rq)
{
        unsigned long flags;

	spin_lock_irqsave(&dd->zone_lock, flags);
	while (!blk_req_can_dispatch_to_zone(rq)) {
		unsigned int zone_no = blk_rq_zone_no(rq);
		sector_t pos = blk_rq_pos(rq);

		while (blk_rq_is_seq_zoned_write(rq)) {
			// XXX: good comment explaining the check here
			if (blk_rq_pos(rq) != pos &&
			    blk_rq_zone_no(rq) != zone_no)
				break;
			pos += blk_rq_sectors(rq);
			rq = deadline_latter_request(rq);
			if (!rq)
				goto out_unlock;
		}
	}
out_unlock:
 	spin_unlock_irqrestore(&dd->zone_lock, flags);
	return rq;
}