Re: [Qemu-block] [PATCH v10 08/10] Implement new driver for block replication

[Stefan Hajnoczi]

On Tue, Oct 27, 2015 at 10:57:42AM +0800, Wen Congyang wrote:
> On 10/16/2015 07:37 PM, Stefan Hajnoczi wrote:
> > On Fri, Oct 16, 2015 at 10:22:05AM +0800, Wen Congyang wrote:
> >> On 10/15/2015 10:55 PM, Stefan Hajnoczi wrote:
> >>> On Thu, Oct 15, 2015 at 10:19:17AM +0800, Wen Congyang wrote:
> >>>> On 10/14/2015 10:27 PM, Stefan Hajnoczi wrote:
> >>>>> On Tue, Oct 13, 2015 at 05:08:17PM +0800, Wen Congyang wrote:
> >>>>>> On 10/13/2015 12:27 AM, Stefan Hajnoczi wrote:
> >>>>>>> On Fri, Sep 25, 2015 at 02:17:36PM +0800, Wen Congyang wrote:
> >>>>>>>> +        /* start backup job now */
> >>>>>>>> +        bdrv_op_unblock(s->hidden_disk, BLOCK_OP_TYPE_BACKUP_TARGET,
> >>>>>>>> +                        s->active_disk->backing_blocker);
> >>>>>>>> +        bdrv_op_unblock(s->secondary_disk, 
> >>>>>>>> BLOCK_OP_TYPE_BACKUP_SOURCE,
> >>>>>>>> +                        s->hidden_disk->backing_blocker);
> >>>>>>>
> >>>>>>> Why is it safe to unblock these operations?
> >>>>>>>
> >>>>>>> Why do they have to be blocked for non-replication users?
> >>>>>>
> >>>>>> hidden_disk and secondary disk are opened as backing file, so it is 
> >>>>>> blocked for
> >>>>>> non-replication users.
> >>>>>> What can I do if I don't unblock it and want to do backup?
> >>>>>
> >>>>> CCing Jeff Cody, block jobs maintainer
> >>>>>
> >>>>> You need to explain why it is safe remove this protection.  We can't
> >>>>> merge code that may be unsafe.
> >>>>>
> >>>>> I think we can investigate further by asking: when does QEMU code assume
> >>>>> the backing file is read-only?
> >>>>
> >>>> The backing file is opened in read-only mode. I want to reopen it in 
> >>>> read-write
> >>>> mode here in the next version(So the patch 1 will be dropped)
> >>>>
> >>>>>
> >>>>> I haven't checked but these cases come to mind:
> >>>>>
> >>>>> Operations that move data between BDS in the backing chain (e.g. commit
> >>>>> and stream block jobs) will lose or overwrite data if the backing file
> >>>>> is being written to by another coroutine.
> >>>>>
> >>>>> We need to prevent users from running these operations at the same time.
> >>>>
> >>>> Yes, but qemu doesn't provide such API.
> >>>
> >>> This series can't be merged unless it is safe.
> >>>
> >>> Have you looked at op blockers and thought about how to prevent unsafe
> >>> operations?
> >>
> >> What about this solution:
> >> 1. unblock it in bdrv_set_backing_hd()
> >> 2. block it in qmp_block_commit(), qmp_block_stream(), 
> >> qmp_block_backup()..., to
> >>    prevent unsafe operations
> > 
> > Come to think of it, currently QEMU only supports 1 block job per BDS.
> > 
> > This means that as long as COLO has a backup job running, no other block
> > jobs can interfere.
> > 
> > There still might be a risk with monitor commands like 'commit'.
> 
> What about this?
> diff --git a/block.c b/block.c
> index e9f40dc..b181d67 100644
> --- a/block.c
> +++ b/block.c
> @@ -1162,6 +1162,24 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>      /* Otherwise we won't be able to commit due to check in bdrv_commit */
>      bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
>                      bs->backing_blocker);
> +    /*
> +     * We do backup in 3 ways:
> +     * 1. drive backup
> +     *    The target bs is new opened, and the source is top BDS
> +     * 2. blockdev backup
> +     *    Both the source and the target are top BDSes.
> +     * 3. internal backup(used for block replication)
> +     *    Both the source and the target are backing file
> +     *
> +     * In case 1, and 2, the backing file is neither the source nor
> +     * the target.
> +     * In case 3, we will block the top BDS, so there is only one block
> +     * job for the top BDS and its backing chain.
> +     */
> +    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_SOURCE,
> +                    bs->backing_blocker);

BLOCK_OP_TYPE_BACKUP_SOURCE does not modify the image so this should be
safe.

> +    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_TARGET,
> +                    bs->backing_blocker);

This one is trickier since it means write access, but
BLOCK_OP_TYPE_COMMIT_TARGET is already unblocked above.  At least it
should be no worse than allowing BLOCK_OP_TYPE_COMMIT_TARGET.

Jeff?

Stefan