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

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

On 05/06/2016 11:46 PM, Stefan Hajnoczi wrote:
> On Fri, Apr 15, 2016 at 04:10:37PM +0800, Changlong Xie wrote:
>> +static void replication_close(BlockDriverState *bs)
>> +{
>> +    BDRVReplicationState *s = bs->opaque;
>> +
>> +    if (s->mode == REPLICATION_MODE_SECONDARY) {
>> +        g_free(s->top_id);
>> +    }
>> +
>> +    if (s->replication_state == BLOCK_REPLICATION_RUNNING) {
>> +        replication_stop(s->rs, false, NULL);
>> +    }
>
> There is a possible use-after-free with s->top_id.  If we free it above
> then replication_stop() must not call backup_job_cleanup().  I think it
> could call it from replication_stop().
>
> It would be safer to call replication_stop() before freeing s->top_id.
>

Yes, you are right.

>> +        top_bs = bdrv_lookup_bs(s->top_id, s->top_id, errp);
>
> Please check that bs is a child of top_bs.  If it is not a child then
> strange things could happen, for example the AioContexts might not match
> (meaning it's not thread-safe) so this should be forbidden.
>

Will fix in next version

>> +        if (!top_bs) {
>> +            aio_context_release(aio_context);
>> +            return;
>> +        }
>
> Error return paths after reopen_backing_file(s, true, &local_err) should
> undo the operation.

Will do.

>
>> +        bdrv_op_block_all(top_bs, s->blocker);
>> +        bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
>> +
>> +        /*
>> +         * Must protect backup target if backup job was stopped/cancelled
>> +         * unexpectedly
>> +         */
>> +        bdrv_ref(s->hidden_disk->bs);
>> +
>> +        backup_start(s->secondary_disk->bs, s->hidden_disk->bs, 0,
>> +                     MIRROR_SYNC_MODE_NONE, NULL, BLOCKDEV_ON_ERROR_REPORT,
>> +                     BLOCKDEV_ON_ERROR_REPORT, backup_job_completed,
>> +                     s, NULL, &local_err);
>
> Did you run stress tests where the primary is writing to the disk while
> the secondary reads from the same sectors?
>
> I thought about this some more and I'm wondering about the following
> scenario:
>
> NBD writes to secondary_disk and the guest reads from the disk at the
> same time.  There is a coroutine mutex in qcow2.c that protects both
> read and write requests, but only until they perform the data I/O.  It
> may be possible that the read request from the Secondary VM could be
> started before the NBD write but the preadv() syscall isn't entered
> because of CPU scheduling decisions.  In the meantime the
> secondary_disk->hidden_disk backup operation takes place.  With some
> unlucky timing it may be possible for the Secondary VM to read the new
> contents from secondary_disk instead of the old contents that were
> backed up into hidden_disk.

Thanks for your catch. I'll think about this scenario carefully.

>
> Extra serialization would be needed.
> block/backup.c:wait_for_overlapping_requests() and
> block/io.c:mark_request_serialising() are good starting points for
> solving this.
>
>> +    cleanup_imgs();
>
> Please use qtest_add_abrt_handler() so cleanup happens even when SIGABRT
> is received.
>

Surely.

Thanks
	-Xie