Re: [Qemu-block] [Qemu-devel] [PATCH v15 8/9] Implement new driver for block replication

[Changlong Xie]

On 03/05/2016 01:39 AM, Stefan Hajnoczi wrote:
> On Fri, Feb 05, 2016 at 12:18:07PM +0800, Changlong Xie wrote:
> > +static void replication_start(ReplicationState *rs, ReplicationMode mode,
> > +                              Error **errp)
> > +{
> > +    BlockDriverState *bs = rs->opaque;
> > +    BDRVReplicationState *s = bs->opaque;
> > +    int64_t active_length, hidden_length, disk_length;
> > +    AioContext *aio_context;
> > +    Error *local_err = NULL;
> > +
> > +    if (s->replication_state != BLOCK_REPLICATION_NONE) {
>
> Dereferencing bs and s is not thread-safe since the AioContext isn't
> acquired here.  Unless you have other locking rules, the default is that
> everything in BlockDriverState and bs->opaque must be accessed with the
> AioContext acquired.
>
> Please apply this comment to the rest of the patch series, too.
> Functions that are not part of BlockDriver are probably called outside
> the AioContext and must acquire it before they are allowed to access
> anything else in bs.
>
> > +        s->top_bs = get_top_bs(bs);
> > +        if (!s->top_bs) {
> > +            error_setg(errp, "Cannot get the top block driver state to do"
> > +                       " internal backup");
> > +            return;
> > +        }
>
> Traversing the BDS graph using the parent pointer is not safe - you
> cannot stash the parent pointer because it changes if a parent node is
> inserted/removed.
>
> I suggest passing the drive name as an argument so that bdrv_lookup_bs()
> can be used when installing the op blocker.  Then the BdrvChild.parent
> pointer patch and get_top_bs() can be deleted.
>
> Jeff Cody is currently working on a new op blocker system.  Hopefully
> this system will allow you to install blockers on bs instead of on the
> drive's top BDS.  But let's not worry about that for now and just use
> the drive name as an argument.
>
> > +        /*
> > +         * Must protect backup target if backup job was stopped/cancelled
> > +         * unexpectedly
> > +         */
> > +        bdrv_ref(s->hidden_disk->bs);
>
> Where is the matching bdrv_unref() call?

Two conditions
1. job is cancelled, so "local_err != 0"

449         if (local_err) {
450             error_propagate(errp, local_err);
451             backup_job_cleanup(s);
452             bdrv_unref(s->hidden_disk->bs);
453             return;
454         }

2. backup completed
        backup_complete
                bdrv_unref(s->target);

If i'm wrong, pls correct me.

Thanks
        -Xie

>