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

[Changlong Xie <xiecl.fnst@cn.fujitsu.com>]

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

>