On 1/17/25 2:47 PM, Damien Le Moal wrote:
Also, patch 14 puts back a lot of the code that was recently removed. Not nice,
and its commit message is also far too short given the size of the patch. Please
spend more time explaining what the patches do and how they do it to facilitate
review.
Regarding the following part of patch 07/14:
- disk_zone_wplug_abort(zwplug);
- zwplug->flags |= BLK_ZONE_WPLUG_NEED_WP_UPDATE;
+ if (!disk->queue->limits.driver_preserves_write_order)
+ zwplug->flags |= BLK_ZONE_WPLUG_NEED_WP_UPDATE;
+ zwplug->flags |= BLK_ZONE_WPLUG_ERROR;
How about setting BLK_ZONE_WPLUG_ERROR only if
disk->queue->limits.driver_preserves_write_order has been set such that
the restored error handling code is only used if the storage controller
preserves the write order? This should be sufficient to preserve the
existing behavior of the blk-zoned.c code for SMR disks.
The reason I restored the error handling code is that the code in
blk-zoned.c does not support error handling inside block drivers if QD > 1
and because supporting the SCSI error handler is essential for UFS
devices.
I also fail to see why you treat request requeue as errors if you actually
expect them to happen...
I only expect requeues to happen in exceptional cases, e.g. because the
SCSI error handler has been activated or because a SCSI device reports a
unit attention. Requeues may happen in any order so I think that it is
essential to pause request submission after a requeue until all pending
zoned writes for the same zone have completed or failed.
I do not think you need error handling and tracking of
the completion wp position: when you get a requeue event, requeue all requests
in the plug and set the plug wp position to the lowest sector of all the
requeued requests. That is necessarily the current location of the write
pointer.
That's an interesting proposal. I will look into setting the WP to the
lowest sector of all requeued requests.
No need for re-introducing all the error handling for that, no ?
The error handling code has been restored because of another reason,
namely to support the SCSI error handler in case the queue depth is
larger than one.
I am going to wait for you to resend this with a better "big picture"
explanation of what you are trying to do so that I do not randomly comment on
things that I am not sure I completely understand. Thanks.
I will include a more detailed cover letter when I repost this series
(after the merge window has closed). While I work on this, feedback on
this version of this patch series is still welcome.
Thanks,
Bart.
