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

[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