Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] block: use internal filter node in backup

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

On Wed, Aug 16, 2017 at 11:11:44PM +0300, Manos Pitsidianakis wrote:
> On Wed, Aug 16, 2017 at 02:25:44PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Aug 15, 2017 at 11:18:53AM +0300, Manos Pitsidianakis wrote:
> > > @@ -3142,7 +3174,7 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
> > >          return false;
> > >      }
> > > 
> > > -    if (c->role == &child_backing) {
> > > +    if (c->role == &child_backing || c->role == &child_file) {
> > >          /* If @from is a backing file of @to, ignore the child to avoid
> > >           * creating a loop. We only want to change the pointer of other
> > >           * parents. */
> > 
> > This may have unwanted side-effects.  I think you're using is so that
> > bdrv_set_file() + bdrv_replace_node() does not create a loop in the
> > graph.  That is okay if there is only one parent with child_file.  If
> > there are multiple parents with that role then it's not clear to me that
> > they should all be skipped.
> 
> I am afraid I don't understand what you're saying. What is the difference
> with the child_backing scenario here?  In both cases we should update all
> from->parents children unless they also happen to be a child of `to`. If
> there are multiple parents with child_file, they are not skipped except for
> the ones where `to` is the parent.

Okay, I'll have to look at this again.  Thanks!

> > > +static BlockDriver backup_top = {
> > > +    .format_name                        =   "backup-top",
> > > +    .instance_size                      =   sizeof(BackupBlockJob *),
> > > +
> > > +    .bdrv_open                          =   backup_top_open,
> > 
> > .bdrv_open may be NULL.  There's no need to define this function.
> > 
> > > +    .bdrv_close                         =   backup_top_close,
> > > +
> > > +    .bdrv_co_flush                      =   backup_top_co_flush,
> > > +    .bdrv_co_preadv                     =   backup_top_co_preadv,
> > > +    .bdrv_co_pwritev                    =   backup_top_co_pwritev,
> > > +    .bdrv_co_pwrite_zeroes              =   backup_top_co_pwrite_zeroes,
> > > +    .bdrv_co_pdiscard                   =   backup_top_co_pdiscard,
> > > +
> > > +    .bdrv_getlength                     =   backup_top_getlength,
> > > +    .bdrv_child_perm                    =   bdrv_filter_default_perms,
> > > +    .bdrv_recurse_is_first_non_filter   =   backup_recurse_is_first_non_filter,
> > 
> > I think this is dead code since .is_filter = true.  It will not be
> > called.
> 
> bdrv_recurse_is_first_non_filter() is only called if is_filter is true and
> the driver implements it to allow recursion to its children.

You are right, I misread the code.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]