[PATCH] block/mirror: inherit the source node discard flag in mirror_top

[PATCH] block/mirror: inherit the source node discard flag in mirror_top

[Haoqian He]

mirror_start_job() inserts a mirror_top filter above the source node
for both mirror and active commit jobs. Previously, the filter was
opened with BDRV_O_RDWR only, even when the source had BDRV_O_UNMAP
enabled.

The block layer checks BDRV_O_UNMAP before invoking a node's discard
callback. Without this flag on mirror_top, discard requests return
success without reaching bdrv_mirror_top_pdiscard(). Consequently,
starting a job can silently suppress guest UNMAP/TRIM requests that
would otherwise reach the discard-enabled source.

Initialize the filter's open flags with BDRV_O_RDWR and conditionally
add BDRV_O_UNMAP from bs->open_flags. Pass the resulting flags to
bdrv_new_open_driver() so that mirror_top follows the source node's
discard setting.

This allows discard requests to reach the existing filter callback,
which forwards them to the source and either tracks the affected
range in the dirty bitmap or synchronously propagates the discard
to the target, depending on the copy mode.

Signed-off-by: Haoqian He <haoqian.he@smartx.com>
---
 block/mirror.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 089856f4a8..a551d392c6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1854,6 +1854,7 @@ static BlockJob *mirror_start_job(
     BlockDriverState *mirror_top_bs;
     bool target_is_backing;
     uint64_t target_perms, target_shared_perms;
+    int flags;
     int ret;
 
     GLOBAL_STATE_CODE();
@@ -1886,8 +1887,13 @@ static BlockJob *mirror_start_job(
     /* In the case of active commit, add dummy driver to provide consistent
      * reads on the top, while disabling it in the intermediate nodes, and make
      * the backing chain writable. */
-    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
-                                         BDRV_O_RDWR, errp);
+    flags = BDRV_O_RDWR;
+    if (bs->open_flags & BDRV_O_UNMAP) {
+        flags |= BDRV_O_UNMAP;
+    }
+    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top,
+                                         filter_node_name,
+                                         flags, errp);
     if (mirror_top_bs == NULL) {
         return NULL;
     }
-- 
2.53.0

Re: [PATCH] block/mirror: inherit the source node discard flag in mirror_top

[Vladimir Sementsov-Ogievskiy]

On 13.10.26 09:05, Haoqian He wrote:
> mirror_start_job() inserts a mirror_top filter above the source node
> for both mirror and active commit jobs. Previously, the filter was
> opened with BDRV_O_RDWR only, even when the source had BDRV_O_UNMAP
> enabled.
> 
> The block layer checks BDRV_O_UNMAP before invoking a node's discard
> callback. Without this flag on mirror_top, discard requests return
> success without reaching bdrv_mirror_top_pdiscard(). Consequently,
> starting a job can silently suppress guest UNMAP/TRIM requests that
> would otherwise reach the discard-enabled source.
> 
> Initialize the filter's open flags with BDRV_O_RDWR and conditionally
> add BDRV_O_UNMAP from bs->open_flags. Pass the resulting flags to
> bdrv_new_open_driver() so that mirror_top follows the source node's
> discard setting.
> 
> This allows discard requests to reach the existing filter callback,
> which forwards them to the source and either tracks the affected
> range in the dirty bitmap or synchronously propagates the discard
> to the target, depending on the copy mode.
> 
> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
> ---
>   block/mirror.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 089856f4a8..a551d392c6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1854,6 +1854,7 @@ static BlockJob *mirror_start_job(
>       BlockDriverState *mirror_top_bs;
>       bool target_is_backing;
>       uint64_t target_perms, target_shared_perms;
> +    int flags;
>       int ret;
>   
>       GLOBAL_STATE_CODE();
> @@ -1886,8 +1887,13 @@ static BlockJob *mirror_start_job(
>       /* In the case of active commit, add dummy driver to provide consistent
>        * reads on the top, while disabling it in the intermediate nodes, and make
>        * the backing chain writable. */
> -    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
> -                                         BDRV_O_RDWR, errp);
> +    flags = BDRV_O_RDWR;
> +    if (bs->open_flags & BDRV_O_UNMAP) {
> +        flags |= BDRV_O_UNMAP;
> +    }
> +    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top,
> +                                         filter_node_name,
> +                                         flags, errp);
>       if (mirror_top_bs == NULL) {
>           return NULL;
>       }

Cool find!

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks for fixing, applied to my block branch.


-- 
Best regards,
Vladimir

Re: [PATCH] block/mirror: inherit the source node discard flag in mirror_top

[Kevin Wolf]

Am 13.10.2026 um 10:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 13.10.26 09:05, Haoqian He wrote:
> > mirror_start_job() inserts a mirror_top filter above the source node
> > for both mirror and active commit jobs. Previously, the filter was
> > opened with BDRV_O_RDWR only, even when the source had BDRV_O_UNMAP
> > enabled.
> > 
> > The block layer checks BDRV_O_UNMAP before invoking a node's discard
> > callback. Without this flag on mirror_top, discard requests return
> > success without reaching bdrv_mirror_top_pdiscard(). Consequently,
> > starting a job can silently suppress guest UNMAP/TRIM requests that
> > would otherwise reach the discard-enabled source.
> > 
> > Initialize the filter's open flags with BDRV_O_RDWR and conditionally
> > add BDRV_O_UNMAP from bs->open_flags. Pass the resulting flags to
> > bdrv_new_open_driver() so that mirror_top follows the source node's
> > discard setting.
> > 
> > This allows discard requests to reach the existing filter callback,
> > which forwards them to the source and either tracks the affected
> > range in the dirty bitmap or synchronously propagates the discard
> > to the target, depending on the copy mode.
> > 
> > Signed-off-by: Haoqian He <haoqian.he@smartx.com>
> > ---
> >   block/mirror.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 089856f4a8..a551d392c6 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -1854,6 +1854,7 @@ static BlockJob *mirror_start_job(
> >       BlockDriverState *mirror_top_bs;
> >       bool target_is_backing;
> >       uint64_t target_perms, target_shared_perms;
> > +    int flags;
> >       int ret;
> >       GLOBAL_STATE_CODE();
> > @@ -1886,8 +1887,13 @@ static BlockJob *mirror_start_job(
> >       /* In the case of active commit, add dummy driver to provide consistent
> >        * reads on the top, while disabling it in the intermediate nodes, and make
> >        * the backing chain writable. */
> > -    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
> > -                                         BDRV_O_RDWR, errp);
> > +    flags = BDRV_O_RDWR;
> > +    if (bs->open_flags & BDRV_O_UNMAP) {
> > +        flags |= BDRV_O_UNMAP;
> > +    }
> > +    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top,
> > +                                         filter_node_name,
> > +                                         flags, errp);
> >       if (mirror_top_bs == NULL) {
> >           return NULL;
> >       }
> 
> Cool find!
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Thanks for fixing, applied to my block branch.

This bug is something that should be relatively easily testable in
qemu-iotests. Can we please add a test at the same time?

Kevin

Re: [PATCH] block/mirror: inherit the source node discard flag in mirror_top

[Haoqian He]

> 2026年10月13日 21:53，Kevin Wolf <kwolf@redhat.com> 写道：
> 
> Am 13.10.2026 um 10:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> On 13.10.26 09:05, Haoqian He wrote:
>>> mirror_start_job() inserts a mirror_top filter above the source node
>>> for both mirror and active commit jobs. Previously, the filter was
>>> opened with BDRV_O_RDWR only, even when the source had BDRV_O_UNMAP
>>> enabled.
>>> 
>>> The block layer checks BDRV_O_UNMAP before invoking a node's discard
>>> callback. Without this flag on mirror_top, discard requests return
>>> success without reaching bdrv_mirror_top_pdiscard(). Consequently,
>>> starting a job can silently suppress guest UNMAP/TRIM requests that
>>> would otherwise reach the discard-enabled source.
>>> 
>>> Initialize the filter's open flags with BDRV_O_RDWR and conditionally
>>> add BDRV_O_UNMAP from bs->open_flags. Pass the resulting flags to
>>> bdrv_new_open_driver() so that mirror_top follows the source node's
>>> discard setting.
>>> 
>>> This allows discard requests to reach the existing filter callback,
>>> which forwards them to the source and either tracks the affected
>>> range in the dirty bitmap or synchronously propagates the discard
>>> to the target, depending on the copy mode.
>>> 
>>> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
>>> ---
>>>  block/mirror.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 089856f4a8..a551d392c6 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -1854,6 +1854,7 @@ static BlockJob *mirror_start_job(
>>>      BlockDriverState *mirror_top_bs;
>>>      bool target_is_backing;
>>>      uint64_t target_perms, target_shared_perms;
>>> +    int flags;
>>>      int ret;
>>>      GLOBAL_STATE_CODE();
>>> @@ -1886,8 +1887,13 @@ static BlockJob *mirror_start_job(
>>>      /* In the case of active commit, add dummy driver to provide consistent
>>>       * reads on the top, while disabling it in the intermediate nodes, and make
>>>       * the backing chain writable. */
>>> -    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
>>> -                                         BDRV_O_RDWR, errp);
>>> +    flags = BDRV_O_RDWR;
>>> +    if (bs->open_flags & BDRV_O_UNMAP) {
>>> +        flags |= BDRV_O_UNMAP;
>>> +    }
>>> +    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top,
>>> +                                         filter_node_name,
>>> +                                         flags, errp);
>>>      if (mirror_top_bs == NULL) {
>>>          return NULL;
>>>      }
>> 
>> Cool find!
>> 
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> 
>> Thanks for fixing, applied to my block branch.
> 
> This bug is something that should be relatively easily testable in
> qemu-iotests. Can we please add a test at the same time?
> 
> Kevin
> 

Okay, i will add qemu-iotests in the subsequent patch ASAP.

Haoqian

Re: [PATCH] block/mirror: inherit the source node discard flag in mirror_top

[Kevin Wolf]

Am 14.10.2026 um 04:25 hat Haoqian He geschrieben:
> > 2026年10月13日 21:53，Kevin Wolf <kwolf@redhat.com> 写道：
> > Am 13.10.2026 um 10:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> On 13.10.26 09:05, Haoqian He wrote:
> >>> mirror_start_job() inserts a mirror_top filter above the source node
> >>> for both mirror and active commit jobs. Previously, the filter was
> >>> opened with BDRV_O_RDWR only, even when the source had BDRV_O_UNMAP
> >>> enabled.
> >>> 
> >>> The block layer checks BDRV_O_UNMAP before invoking a node's discard
> >>> callback. Without this flag on mirror_top, discard requests return
> >>> success without reaching bdrv_mirror_top_pdiscard(). Consequently,
> >>> starting a job can silently suppress guest UNMAP/TRIM requests that
> >>> would otherwise reach the discard-enabled source.
> >>> 
> >>> Initialize the filter's open flags with BDRV_O_RDWR and conditionally
> >>> add BDRV_O_UNMAP from bs->open_flags. Pass the resulting flags to
> >>> bdrv_new_open_driver() so that mirror_top follows the source node's
> >>> discard setting.
> >>> 
> >>> This allows discard requests to reach the existing filter callback,
> >>> which forwards them to the source and either tracks the affected
> >>> range in the dirty bitmap or synchronously propagates the discard
> >>> to the target, depending on the copy mode.
> >>> 
> >>> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
> >>> ---
> >>>  block/mirror.c | 10 ++++++++--
> >>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/block/mirror.c b/block/mirror.c
> >>> index 089856f4a8..a551d392c6 100644
> >>> --- a/block/mirror.c
> >>> +++ b/block/mirror.c
> >>> @@ -1854,6 +1854,7 @@ static BlockJob *mirror_start_job(
> >>>      BlockDriverState *mirror_top_bs;
> >>>      bool target_is_backing;
> >>>      uint64_t target_perms, target_shared_perms;
> >>> +    int flags;
> >>>      int ret;
> >>>      GLOBAL_STATE_CODE();
> >>> @@ -1886,8 +1887,13 @@ static BlockJob *mirror_start_job(
> >>>      /* In the case of active commit, add dummy driver to provide consistent
> >>>       * reads on the top, while disabling it in the intermediate nodes, and make
> >>>       * the backing chain writable. */
> >>> -    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top, filter_node_name,
> >>> -                                         BDRV_O_RDWR, errp);
> >>> +    flags = BDRV_O_RDWR;
> >>> +    if (bs->open_flags & BDRV_O_UNMAP) {
> >>> +        flags |= BDRV_O_UNMAP;
> >>> +    }
> >>> +    mirror_top_bs = bdrv_new_open_driver(&bdrv_mirror_top,
> >>> +                                         filter_node_name,
> >>> +                                         flags, errp);
> >>>      if (mirror_top_bs == NULL) {
> >>>          return NULL;
> >>>      }
> >> 
> >> Cool find!
> >> 
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> 
> >> Thanks for fixing, applied to my block branch.
> > 
> > This bug is something that should be relatively easily testable in
> > qemu-iotests. Can we please add a test at the same time?
> > 
> > Kevin
> > 
> 
> Okay, i will add qemu-iotests in the subsequent patch ASAP.

Did you have time to work on this? Soft feature freeze for QEMU 11.2
will be on Monday, so it would be nice to get it merged by then.

Kevin

[PATCH] iotests: cover discard passthrough during background mirror

[Haoqian He]

A mirror_top node without BDRV_O_UNMAP silently ignores discard
requests.

Add a regression test to verify that discard requests are not
ignored during background mirror.

Signed-off-by: Haoqian He <haoqian.he@smartx.com>
---
 tests/qemu-iotests/tests/mirror-discard     | 73 +++++++++++++++++++++
 tests/qemu-iotests/tests/mirror-discard.out |  5 ++
 2 files changed, 78 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/mirror-discard
 create mode 100644 tests/qemu-iotests/tests/mirror-discard.out

diff --git a/tests/qemu-iotests/tests/mirror-discard b/tests/qemu-iotests/tests/mirror-discard
new file mode 100755
index 0000000000..4b89e9bc7d
--- /dev/null
+++ b/tests/qemu-iotests/tests/mirror-discard
@@ -0,0 +1,73 @@
+#!/usr/bin/env python3
+# group: rw auto quick
+#
+# Test that discard reaches the source through a blockdev-mirror filter.
+#
+# Copyright (C) 2026 SmartX, Inc.
+
+from pathlib import Path
+
+import iotests
+
+
+source = Path(iotests.test_dir, 'mirror-discard-source.qcow2')
+target = Path(iotests.test_dir, 'mirror-discard-target.qcow2')
+
+
+class TestMirrorDiscard(iotests.QMPTestCase):
+    def setUp(self):
+        for path in (source, target):
+            self.addCleanup(path.unlink, missing_ok=True)
+            iotests.qemu_img_create(
+                '-f', iotests.imgfmt,
+                '-o', 'compat=1.1,cluster_size=65536', str(path), '1M')
+
+        self.vm = iotests.VM()
+        # Cleanups run in reverse order: close the VM before removing images.
+        self.addCleanup(self.vm.shutdown)
+        self.vm.add_drive(str(source), opts='discard=unmap,node-name=src')
+        self.vm.add_blockdev(
+            f'driver={iotests.imgfmt},node-name=target,'
+            f'file.driver=file,file.filename={target}')
+        self.vm.launch()
+
+    def test_background_discard(self):
+        self.vm.cmd('x-qemu-io', device='drive0',
+                    command='write -q -P 0xab 0 192k')
+
+        # Check discard works before mirror_top is inserted, then restore data.
+        self.vm.cmd('x-qemu-io', device='drive0',
+                    command='discard -q 64k 64k')
+        self.vm.cmd('x-qemu-io', device='drive0',
+                    command='read -q -P 0x00 64k 64k')
+        self.vm.cmd('x-qemu-io', device='drive0',
+                    command='write -q -P 0xab 64k 64k')
+
+        self.vm.cmd('blockdev-mirror', job_id='mirror', device='drive0',
+                    target='target', sync='full', copy_mode='background')
+        self.wait_ready('mirror')
+        self.pause_job('mirror')
+
+        # Use the backend so the request passes through mirror_top to src.
+        self.vm.cmd('x-qemu-io', device='drive0',
+                    command='discard -q 64k 64k')
+        checks = (
+            'read -q -P 0x00 64k 64k',
+            'read -q -P 0xab 0 64k',
+            'read -q -P 0xab 128k 64k',
+        )
+        for command in checks:
+            self.vm.cmd('x-qemu-io', device='drive0', command=command)
+
+        self.vm.cmd('block-job-resume', device='mirror')
+        self.complete_and_wait('mirror', wait_ready=False)
+        self.vm.shutdown()
+
+        # Check both images independently after all writes have been flushed.
+        for path in (source, target):
+            for command in checks:
+                iotests.qemu_io('-f', iotests.imgfmt, '-c', command, str(path))
+
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'], supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/mirror-discard.out b/tests/qemu-iotests/tests/mirror-discard.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/mirror-discard.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
-- 
2.53.0