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;
}
