* [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
@ 2025-04-11 0:04 Damien Le Moal
2025-04-11 21:01 ` Benjamin Marzinski
2025-04-14 13:19 ` Mikulas Patocka
0 siblings, 2 replies; 9+ messages in thread
From: Damien Le Moal @ 2025-04-11 0:04 UTC (permalink / raw)
To: dm-devel, Mike Snitzer, Mikulas Patocka
Cc: Christoph Hellwig, Benjamin Marzinski
When a dm-delay device is being suspended, the .presuspend() operation
is 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, if 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.
Reported-by: Benjamin Marzinski <bmarzins@redhat.com>
Fixes: d43929ef65a6 ("dm-delay: support zoned devices")
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
Changes from v1:
- Fixed typo in commit message
- Added reported-by tag
drivers/md/dm-delay.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index d4cf0ac2a7aa..c665b2ab1115 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -128,7 +128,7 @@ static int flush_worker_fn(void *data)
struct delay_c *dc = data;
while (!kthread_should_stop()) {
- flush_delayed_bios(dc, false);
+ flush_delayed_bios(dc, !dc->may_delay);
spin_lock(&dc->delayed_bios_lock);
if (unlikely(list_empty(&dc->delayed_bios))) {
set_current_state(TASK_INTERRUPTIBLE);
@@ -213,6 +213,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
struct delay_c *dc;
int ret;
unsigned int max_delay;
+ bool is_zoned = false;
if (argc != 3 && argc != 6 && argc != 9) {
ti->error = "Requires exactly 3, 6 or 9 arguments";
@@ -236,6 +237,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
if (ret)
goto bad;
max_delay = dc->read.delay;
+ is_zoned = bdev_is_zoned(dc->read.dev->bdev);
if (argc == 3) {
ret = delay_class_ctr(ti, &dc->write, argv);
@@ -251,6 +253,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
if (ret)
goto bad;
max_delay = max(max_delay, dc->write.delay);
+ is_zoned = is_zoned || bdev_is_zoned(dc->write.dev->bdev);
if (argc == 6) {
ret = delay_class_ctr(ti, &dc->flush, argv + 3);
@@ -263,13 +266,16 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
if (ret)
goto bad;
max_delay = max(max_delay, dc->flush.delay);
+ is_zoned = is_zoned || bdev_is_zoned(dc->flush.dev->bdev);
out:
- if (max_delay < 50) {
- /*
- * In case of small requested delays, use kthread instead of
- * timers and workqueue to achieve better latency.
- */
+ /*
+ * In case of small requested delays, use kthread instead of timers and
+ * workqueue to achieve better latency. Also use a kthread for a zoned
+ * device so that we can preserve the order of write operations during
+ * suspend.
+ */
+ if (max_delay < 50 || is_zoned) {
dc->worker = kthread_run(&flush_worker_fn, dc, "dm-delay-flush-worker");
if (IS_ERR(dc->worker)) {
ret = PTR_ERR(dc->worker);
@@ -313,8 +319,15 @@ static int delay_bio(struct delay_c *dc, struct delay_class *c, struct bio *bio)
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;
+ }
}
c->ops++;
list_add_tail(&delayed->list, &dc->delayed_bios);
--
2.49.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-11 0:04 [PATCH v2] dm-delay: Prevent zoned write reordering on suspend Damien Le Moal
@ 2025-04-11 21:01 ` Benjamin Marzinski
2025-04-14 13:19 ` Mikulas Patocka
1 sibling, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2025-04-11 21:01 UTC (permalink / raw)
To: Damien Le Moal; +Cc: dm-devel, Mike Snitzer, Mikulas Patocka, Christoph Hellwig
On Fri, Apr 11, 2025 at 09:04:35AM +0900, Damien Le Moal wrote:
> When a dm-delay device is being suspended, the .presuspend() operation
> is 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, if 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.
>
> Reported-by: Benjamin Marzinski <bmarzins@redhat.com>
> Fixes: d43929ef65a6 ("dm-delay: support zoned devices")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
This looks fine, other than the issue that flush_worker_fn() busy-waits
for an IO to be ready, and now it could do that for much longer. But
I just submitted a patch to fix that, so:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> Changes from v1:
> - Fixed typo in commit message
> - Added reported-by tag
>
> drivers/md/dm-delay.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
> index d4cf0ac2a7aa..c665b2ab1115 100644
> --- a/drivers/md/dm-delay.c
> +++ b/drivers/md/dm-delay.c
> @@ -128,7 +128,7 @@ static int flush_worker_fn(void *data)
> struct delay_c *dc = data;
>
> while (!kthread_should_stop()) {
> - flush_delayed_bios(dc, false);
> + flush_delayed_bios(dc, !dc->may_delay);
> spin_lock(&dc->delayed_bios_lock);
> if (unlikely(list_empty(&dc->delayed_bios))) {
> set_current_state(TASK_INTERRUPTIBLE);
> @@ -213,6 +213,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
> struct delay_c *dc;
> int ret;
> unsigned int max_delay;
> + bool is_zoned = false;
>
> if (argc != 3 && argc != 6 && argc != 9) {
> ti->error = "Requires exactly 3, 6 or 9 arguments";
> @@ -236,6 +237,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
> if (ret)
> goto bad;
> max_delay = dc->read.delay;
> + is_zoned = bdev_is_zoned(dc->read.dev->bdev);
>
> if (argc == 3) {
> ret = delay_class_ctr(ti, &dc->write, argv);
> @@ -251,6 +253,7 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
> if (ret)
> goto bad;
> max_delay = max(max_delay, dc->write.delay);
> + is_zoned = is_zoned || bdev_is_zoned(dc->write.dev->bdev);
>
> if (argc == 6) {
> ret = delay_class_ctr(ti, &dc->flush, argv + 3);
> @@ -263,13 +266,16 @@ static int delay_ctr(struct dm_target *ti, unsigned int argc, char **argv)
> if (ret)
> goto bad;
> max_delay = max(max_delay, dc->flush.delay);
> + is_zoned = is_zoned || bdev_is_zoned(dc->flush.dev->bdev);
>
> out:
> - if (max_delay < 50) {
> - /*
> - * In case of small requested delays, use kthread instead of
> - * timers and workqueue to achieve better latency.
> - */
> + /*
> + * In case of small requested delays, use kthread instead of timers and
> + * workqueue to achieve better latency. Also use a kthread for a zoned
> + * device so that we can preserve the order of write operations during
> + * suspend.
> + */
> + if (max_delay < 50 || is_zoned) {
> dc->worker = kthread_run(&flush_worker_fn, dc, "dm-delay-flush-worker");
> if (IS_ERR(dc->worker)) {
> ret = PTR_ERR(dc->worker);
> @@ -313,8 +319,15 @@ static int delay_bio(struct delay_c *dc, struct delay_class *c, struct bio *bio)
>
> 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;
> + }
> }
> c->ops++;
> list_add_tail(&delayed->list, &dc->delayed_bios);
> --
> 2.49.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-11 0:04 [PATCH v2] dm-delay: Prevent zoned write reordering on suspend Damien Le Moal
2025-04-11 21:01 ` Benjamin Marzinski
@ 2025-04-14 13:19 ` Mikulas Patocka
2025-04-15 8:08 ` Damien Le Moal
1 sibling, 1 reply; 9+ messages in thread
From: Mikulas Patocka @ 2025-04-14 13:19 UTC (permalink / raw)
To: Damien Le Moal
Cc: dm-devel, Mike Snitzer, Christoph Hellwig, Benjamin Marzinski
On Fri, 11 Apr 2025, Damien Le Moal wrote:
> When a dm-delay device is being suspended, the .presuspend() operation
> is 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, if 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.
>
> Reported-by: Benjamin Marzinski <bmarzins@redhat.com>
> Fixes: d43929ef65a6 ("dm-delay: support zoned devices")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
> Changes from v1:
> - Fixed typo in commit message
> - Added reported-by tag
>
> drivers/md/dm-delay.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
Hi
I looked at the generic device mapper code and it seems that ordering of
write bios is not guaranteed with any target in case of suspend/resume.
* we suspend the device:
* received bios are added to md->deferred in queue_io
* we resume the device:
* __dm_resume calls dm_queue_flush
* dm_queue_flush clears DMF_BLOCK_IO_FOR_SUSPEND and submits work item
&md->work (dm_wq_work)
* dm_resume clears DMF_SUSPENDED
* the device starts accepting new bios in dm_submit_bio
* dm_wq_work runs concurrently with new bios that are received, so
ordering of bios is not preserved
So it doesn't make much sense to try to fix it in dm-delay, if it isn't
supposed to work at all.
Mikulas
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-14 13:19 ` Mikulas Patocka
@ 2025-04-15 8:08 ` Damien Le Moal
2025-04-16 12:45 ` Mikulas Patocka
0 siblings, 1 reply; 9+ messages in thread
From: Damien Le Moal @ 2025-04-15 8:08 UTC (permalink / raw)
To: Mikulas Patocka
Cc: dm-devel, Mike Snitzer, Christoph Hellwig, Benjamin Marzinski
On 4/14/25 10:19 PM, Mikulas Patocka wrote:
>
>
> On Fri, 11 Apr 2025, Damien Le Moal wrote:
>
>> When a dm-delay device is being suspended, the .presuspend() operation
>> is 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, if 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.
>>
>> Reported-by: Benjamin Marzinski <bmarzins@redhat.com>
>> Fixes: d43929ef65a6 ("dm-delay: support zoned devices")
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>> Changes from v1:
>> - Fixed typo in commit message
>> - Added reported-by tag
>>
>> drivers/md/dm-delay.c | 29 +++++++++++++++++++++--------
>> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> Hi
>
> I looked at the generic device mapper code and it seems that ordering of
> write bios is not guaranteed with any target in case of suspend/resume.
>
> * we suspend the device:
> * received bios are added to md->deferred in queue_io
>
> * we resume the device:
> * __dm_resume calls dm_queue_flush
> * dm_queue_flush clears DMF_BLOCK_IO_FOR_SUSPEND and submits work item
> &md->work (dm_wq_work)
> * dm_resume clears DMF_SUSPENDED
> * the device starts accepting new bios in dm_submit_bio
> * dm_wq_work runs concurrently with new bios that are received, so
> ordering of bios is not preserved
>
> So it doesn't make much sense to try to fix it in dm-delay, if it isn't
> supposed to work at all.
Just need to fix the generic DM resume code then. This patch fixing dm-delay is
still relevant even with DM generic resume fixes.
I can resend the dm-delay fix together with DM core resume fixes. And Benjamin
can re-send the dm-delay kthread timer cleanup independently (I will rebase) or
on top of that fix series. Does that work for you ?
>
> Mikulas
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-15 8:08 ` Damien Le Moal
@ 2025-04-16 12:45 ` Mikulas Patocka
2025-04-17 22:15 ` Damien Le Moal
0 siblings, 1 reply; 9+ messages in thread
From: Mikulas Patocka @ 2025-04-16 12:45 UTC (permalink / raw)
To: Damien Le Moal
Cc: dm-devel, Mike Snitzer, Christoph Hellwig, Benjamin Marzinski
On Tue, 15 Apr 2025, Damien Le Moal wrote:
> > Hi
> >
> > I looked at the generic device mapper code and it seems that ordering of
> > write bios is not guaranteed with any target in case of suspend/resume.
> >
> > * we suspend the device:
> > * received bios are added to md->deferred in queue_io
> >
> > * we resume the device:
> > * __dm_resume calls dm_queue_flush
> > * dm_queue_flush clears DMF_BLOCK_IO_FOR_SUSPEND and submits work item
> > &md->work (dm_wq_work)
> > * dm_resume clears DMF_SUSPENDED
> > * the device starts accepting new bios in dm_submit_bio
> > * dm_wq_work runs concurrently with new bios that are received, so
> > ordering of bios is not preserved
> >
> > So it doesn't make much sense to try to fix it in dm-delay, if it isn't
> > supposed to work at all.
>
> Just need to fix the generic DM resume code then. This patch fixing dm-delay is
> still relevant even with DM generic resume fixes.
>
> I can resend the dm-delay fix together with DM core resume fixes. And Benjamin
> can re-send the dm-delay kthread timer cleanup independently (I will rebase) or
> on top of that fix series. Does that work for you ?
I would like to know why is this needed. If you have a zoned device, you
can send one big write bio, wait for the big bio to finish, send another
big write bio, wait for it to finish and so on. Then, there will be at
most one write bio oustanding and you don't have to care about kernel
reordering in-flight bios.
It seems that you want to send many small overlapping write bios - the
question is why? Why can't the application accumulate the content and send
it as one big bio?
I'm a bit worried that supporting this ordering will just bloat the kernel
with marginal benefit.
Mikulas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-16 12:45 ` Mikulas Patocka
@ 2025-04-17 22:15 ` Damien Le Moal
2025-04-22 11:27 ` Mikulas Patocka
0 siblings, 1 reply; 9+ messages in thread
From: Damien Le Moal @ 2025-04-17 22:15 UTC (permalink / raw)
To: Mikulas Patocka
Cc: dm-devel, Mike Snitzer, Christoph Hellwig, Benjamin Marzinski
On 4/16/25 21:45, Mikulas Patocka wrote:
>
>
> On Tue, 15 Apr 2025, Damien Le Moal wrote:
>
>>> Hi
>>>
>>> I looked at the generic device mapper code and it seems that ordering of
>>> write bios is not guaranteed with any target in case of suspend/resume.
>>>
>>> * we suspend the device:
>>> * received bios are added to md->deferred in queue_io
>>>
>>> * we resume the device:
>>> * __dm_resume calls dm_queue_flush
>>> * dm_queue_flush clears DMF_BLOCK_IO_FOR_SUSPEND and submits work item
>>> &md->work (dm_wq_work)
>>> * dm_resume clears DMF_SUSPENDED
>>> * the device starts accepting new bios in dm_submit_bio
>>> * dm_wq_work runs concurrently with new bios that are received, so
>>> ordering of bios is not preserved
>>>
>>> So it doesn't make much sense to try to fix it in dm-delay, if it isn't
>>> supposed to work at all.
>>
>> Just need to fix the generic DM resume code then. This patch fixing dm-delay is
>> still relevant even with DM generic resume fixes.
>>
>> I can resend the dm-delay fix together with DM core resume fixes. And Benjamin
>> can re-send the dm-delay kthread timer cleanup independently (I will rebase) or
>> on top of that fix series. Does that work for you ?
>
> I would like to know why is this needed. If you have a zoned device, you
> can send one big write bio, wait for the big bio to finish, send another
> big write bio, wait for it to finish and so on. Then, there will be at
> most one write bio oustanding and you don't have to care about kernel
> reordering in-flight bios.
Except for the "big" adjective in you remark, what you are describing is zone
write plugging, which will limit the number of in-flight write commands to at
most 1 per zone. That is already done for all zoned block devices at the low
level. For DM, we enable zone write plugging if and only if the DM target driver
ask for zone append emulation because the target driver cannot support native
zone append operations. E.g. dm-crypt does that so that all zone append
operations are turned into regular writes so that we can have the usual IV for
encryption be set to the written sector.
As for "send one big write bio", the "big" here completely depends on the device
user. We can only process what the user issues (FS or userland). The block layer
does not do write buffering.
> It seems that you want to send many small overlapping write bios - the
> question is why? Why can't the application accumulate the content and send
> it as one big bio?
That is the application problem. On HDDs at least, small IOs will hurt
performance. SMR or not, same problem. Intellignet applications will try to
shape their workload to optimize performance. But that point is irrelevant here.
The kernel porvides a service: process write requests, regardless of how big
these requests are, if they are correct (i.e. for zoned devices, they must be
issued in order by the user), we must correctly execute the writes.
> I'm a bit worried that supporting this ordering will just bloat the kernel
> with marginal benefit.
Bloat ? everything is already in place to preserve the order of write operations
to zoned devices, since a long time ago. What has not been covered are cases
like suspend/resume which may, depending on what they do, break the ordering
guarantees that we have for write requests. The only reason this has not been
fixed is because I completely overlooked these cases as zoned block devices were
in the past mostly used in enterprise systems where suspend/resume is not really
used at all. But we have zoned UFS devices these days (smart phones), so
properly supporting DM suspend/resume is important I think.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-17 22:15 ` Damien Le Moal
@ 2025-04-22 11:27 ` Mikulas Patocka
2025-04-22 12:20 ` Damien Le Moal
0 siblings, 1 reply; 9+ messages in thread
From: Mikulas Patocka @ 2025-04-22 11:27 UTC (permalink / raw)
To: Damien Le Moal
Cc: dm-devel, Mike Snitzer, Christoph Hellwig, Benjamin Marzinski
On Fri, 18 Apr 2025, Damien Le Moal wrote:
> > It seems that you want to send many small overlapping write bios - the
> > question is why? Why can't the application accumulate the content and send
> > it as one big bio?
>
> That is the application problem. On HDDs at least, small IOs will hurt
> performance. SMR or not, same problem. Intellignet applications will try to
> shape their workload to optimize performance. But that point is irrelevant here.
> The kernel porvides a service: process write requests, regardless of how big
> these requests are, if they are correct (i.e. for zoned devices, they must be
> issued in order by the user), we must correctly execute the writes.
>
> > I'm a bit worried that supporting this ordering will just bloat the kernel
> > with marginal benefit.
>
> Bloat ?
We would need three states instead of two: normal, suspended, resuming (so
it would bloat all the device mapper logic with another state). There's
dm_wq_work using submit_bio_noacct, which wouldn't work, as it would
immediatelly enqueue the bio for suspend again, so we would need some
other path to submit the bio.
dm_wq_work would have to transition the device from the "resuming" state
to the "normal" state when it processes all the bios, but it is called for
various other reasons too.
> everything is already in place to preserve the order of write operations
> to zoned devices, since a long time ago.
What if the controller doesn't preserve the order of writes? I think that
there was some bit for that, but I forgot its name. So we can simply not
set the bit for device mapper - and the applications will have to deal
with it by using write plugging.
> What has not been covered are cases
> like suspend/resume which may, depending on what they do, break the ordering
> guarantees that we have for write requests. The only reason this has not been
> fixed is because I completely overlooked these cases as zoned block devices were
> in the past mostly used in enterprise systems where suspend/resume is not really
> used at all. But we have zoned UFS devices these days (smart phones), so
> properly supporting DM suspend/resume is important I think.
Do you mean zoned flash devices? I've never heard of them.
Mikulas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-22 11:27 ` Mikulas Patocka
@ 2025-04-22 12:20 ` Damien Le Moal
2025-04-22 18:42 ` Mikulas Patocka
0 siblings, 1 reply; 9+ messages in thread
From: Damien Le Moal @ 2025-04-22 12:20 UTC (permalink / raw)
To: Mikulas Patocka
Cc: dm-devel, Mike Snitzer, Christoph Hellwig, Benjamin Marzinski
On 4/22/25 20:27, Mikulas Patocka wrote:
>
>
> On Fri, 18 Apr 2025, Damien Le Moal wrote:
>
>>> It seems that you want to send many small overlapping write bios - the
>>> question is why? Why can't the application accumulate the content and send
>>> it as one big bio?
>>
>> That is the application problem. On HDDs at least, small IOs will hurt
>> performance. SMR or not, same problem. Intellignet applications will try to
>> shape their workload to optimize performance. But that point is irrelevant here.
>> The kernel porvides a service: process write requests, regardless of how big
>> these requests are, if they are correct (i.e. for zoned devices, they must be
>> issued in order by the user), we must correctly execute the writes.
>>
>>> I'm a bit worried that supporting this ordering will just bloat the kernel
>>> with marginal benefit.
>>
>> Bloat ?
>
> We would need three states instead of two: normal, suspended, resuming (so
> it would bloat all the device mapper logic with another state). There's
> dm_wq_work using submit_bio_noacct, which wouldn't work, as it would
> immediatelly enqueue the bio for suspend again, so we would need some
> other path to submit the bio.
>
> dm_wq_work would have to transition the device from the "resuming" state
> to the "normal" state when it processes all the bios, but it is called for
> various other reasons too.
It is not because you do not see a clean solution that there is not one. So
unless you have completely made up your mind already and are not willing to
accept any change in this area to improve things, I will dig into this and find
a solution that is not "bloat".
>> everything is already in place to preserve the order of write operations
>> to zoned devices, since a long time ago.
>
> What if the controller doesn't preserve the order of writes? I think that
> there was some bit for that, but I forgot its name. So we can simply not
> set the bit for device mapper - and the applications will have to deal
> with it by using write plugging.
I do not understand what you are talking about. A zoned DM device is zoned
because it is on top of a zoned device. That bottom zoned device may be another
DM target or a real zoned device. For the real zoned device, zone write plugging
is always used so it does not matter if the host controller does or does not
preserve command order. There will always be at most 1 in-flight write per zone,
which makes reordering of commands completely irrelevant for write commands success.
For DM, it is up to the target driver to determine if it is OK without zone
write plugging or if that will be needed, as the driver knows if it will
preserve (issue) writes in the same order it received them. E.g. dm-crypt does
not, so it sets the emulate zone append flag to use zone append emulation and
zone write plugging (note that these 2 aspects are aggregated into a single flag
because there was no need to control them separately for the existing DM targets
that support zones).
So I do not understand your point.
There are literally tens of millions of SMR drives running in production, a lot
of them using DM (e.g. dm-crypt). I would know if that was not working fine.
>
>> What has not been covered are cases
>> like suspend/resume which may, depending on what they do, break the ordering
>> guarantees that we have for write requests. The only reason this has not been
>> fixed is because I completely overlooked these cases as zoned block devices were
>> in the past mostly used in enterprise systems where suspend/resume is not really
>> used at all. But we have zoned UFS devices these days (smart phones), so
>> properly supporting DM suspend/resume is important I think.
>
> Do you mean zoned flash devices? I've never heard of them.
They exist and are gathering interest and use cases.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] dm-delay: Prevent zoned write reordering on suspend
2025-04-22 12:20 ` Damien Le Moal
@ 2025-04-22 18:42 ` Mikulas Patocka
0 siblings, 0 replies; 9+ messages in thread
From: Mikulas Patocka @ 2025-04-22 18:42 UTC (permalink / raw)
To: Damien Le Moal
Cc: dm-devel, Mike Snitzer, Christoph Hellwig, Benjamin Marzinski
On Tue, 22 Apr 2025, Damien Le Moal wrote:
> It is not because you do not see a clean solution that there is not one. So
> unless you have completely made up your mind already and are not willing to
I haven't made up my mind completely.
> I do not understand what you are talking about. A zoned DM device is zoned
> because it is on top of a zoned device. That bottom zoned device may be another
> DM target or a real zoned device. For the real zoned device, zone write plugging
> is always used so it does not matter if the host controller does or does not
> preserve command order.
I understand that. I don't understand why can't DM devices also use write
plugging. What kind of problem are you trying to fix - a performance
problem or a correctness problem?
> There will always be at most 1 in-flight write per zone, which makes
> reordering of commands completely irrelevant for write commands success.
If reordering is irrelevant for the underlying device, why is it relevant
for DM?
If the host controller may reorder commands (as you say), why can't DM do
it?
Mikulas
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-04-22 18:43 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-11 0:04 [PATCH v2] dm-delay: Prevent zoned write reordering on suspend Damien Le Moal
2025-04-11 21:01 ` Benjamin Marzinski
2025-04-14 13:19 ` Mikulas Patocka
2025-04-15 8:08 ` Damien Le Moal
2025-04-16 12:45 ` Mikulas Patocka
2025-04-17 22:15 ` Damien Le Moal
2025-04-22 11:27 ` Mikulas Patocka
2025-04-22 12:20 ` Damien Le Moal
2025-04-22 18:42 ` Mikulas Patocka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.