Re: [PATCH] dm-delay: Prevent zoned write reordering on suspend

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Apr 10, 2025 at 05:02:00PM +0900, Damien Le Moal wrote:
> On 4/10/25 4:54 PM, Christoph Hellwig wrote:
> > On Thu, Apr 10, 2025 at 01:33:11PM +0900, Damien Le Moal wrote:
> >> When a dm-delay devie is being suspended, the .presuspend() operation is
> > 
> > s/devie/device/
> > 
> >> first executed (delay_presuspend()) to immediately issue all the BIOs
> >> present in the delayed list of the device and also sets the device
> >> may_delay boolean to false. At the same time, any new BIO is issued to
> >> the device will not be delayed and immediately issued with delay_bio()
> >> returning DM_MAPIO_REMAPPED. This creates a situation where potentially
> >> 2 different contexts may be issuing write BIOs to the same zone of a
> >> zone device without respecting the issuing order from the user, that is,
> >> a newly issued write BIO may be issued before other write BIOs for the
> >> same target zone that are in the device delayed list. If such situation
> >> occurs, write BIOs may be failed by the underlying zoned device due to
> >> an unaligned write error.
> >>
> >> Prevent this situation from happening by always handling newly issued
> >> write BIOs using the delayed list of BIOs, even when the device is being
> >> suspended. This is done by forcing the use of the worker kthread for
> >> zoned devices, and by modifying flush_worker_fn() to always flush all
> >> delayed BIOs if the device may_delay boolean is false.
> > 
> > Is that scenario specific to dm-delay?  I think the same applies to
> > any other target that supports passing on bios to zoned devices.
> 
> Don't think so. The delayed BIO list is a dm-delay thing only. dm-linear,
> dm-error or dm-crypt do not delay BIOs like dm-delay so we do not have BIOs
> being issued in the pre-suspend context.
> 
> > 
> >>  	spin_lock(&dc->delayed_bios_lock);
> >>  	if (unlikely(!dc->may_delay)) {
> >> -		spin_unlock(&dc->delayed_bios_lock);
> >> -		return DM_MAPIO_REMAPPED;
> >> +		/*
> >> +		 * Issue the BIO immediately if the device is not zoned. FOr a
> >> +		 * zoned device, preserver the ordering of write operations by
> >> +		 * using the delay list.
> >> +		 */
> >> +		if (!bdev_is_zoned(c->dev->bdev) || c != &dc->write) {
> >> +			spin_unlock(&dc->delayed_bios_lock);
> >> +			return DM_MAPIO_REMAPPED;
> > 
> > Don't we only need to do that if anything is queued up due to a
> > suspension?  Then again having a different patch might be premature
> > optimization, it's not like anyone cares about dm-delay performance
> > almost by definition :)
> 
> True. I can add a list_empty check here. Will send a v2 with that.

I may be pointing out the obvious, but we can't just check if
dc->delayed_bios is empty, since we pull the bios off the list before we
submit them. We can only skip adding the bios if the list is empty and
flush_delay_bios() isn't currently processing any bios.

-Ben

> 
> 
> -- 
> Damien Le Moal
> Western Digital Research