Re: [Qemu-devel] [Qemu-block] [PATCH v2 06/16] backup: Request BLK_PERM_AIO_CONTEXT_CHANGE on target

[Stefan Hajnoczi]

On Wed, May 31, 2017 at 05:57:46PM +0800, Fam Zheng wrote:
> On Wed, 05/31 10:39, Stefan Hajnoczi wrote:
> > On Wed, May 24, 2017 at 10:18:44AM +0800, Fam Zheng wrote:
> > > On Thu, 05/11 15:41, Stefan Hajnoczi wrote:
> > > > On Wed, Apr 19, 2017 at 05:43:46PM +0800, Fam Zheng wrote:
> > > > > What's done in the source's context change notifier is moving the
> > > > > target's context to follow the new one, so we request this permission
> > > > > here.
> > > > 
> > > > It's true that the backup block job must be able to set target's
> > > > AioContext, but does this change also allow other users to set target's
> > > > AioContext while the backup job is running?  If yes, then we need to
> > > > handle that.
> > > 
> > > If through job->target, yes, but I don't think there is any user of 
> > > job->target.
> > > Otherwise, it's not allowed, because the second parameter of blk_new 
> > > doesn't
> > > have BLK_PERM_AIO_CONTEXT_CHANGE.
> > > 
> > > So it's okay.
> > 
> > What about blockdev-backup?  It allows the user to specify 'target'.
> > Therefore the user can also run other monitor commands on target.  Some
> > of them could change the AioContext and the backup job wouldn't know!
> 
> That will be rejected.
> 
> The contract is that any code that wants to change the AioContext of a BDS, in
> this case the "target BDS", must do this:
> 
> 1) create its own BB with perm.BLK_PERM_AIO_CONTEXT_CHANGE
> 
> 2) attach BDS to this BB
> 
> 3) call blk_set_aio_context and change the AioContext
> 
> This is basically how all users of a BDS coordinate through Kevin's new op
> blocker API, and in your concerned case, when a user runs a second monitor
> command that changes AioContext, step 2 will fail, because as in this patch, 
> the
> first job->target BB didn't set shared_perm.BLK_PERM_AIO_CONTEXT_CHANGE.

I was wondering how that works since do_blockdev_backup() does not use
BB to access target, but it does check whether a BB is already attached:

    target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
    if (!target_bs) {
        goto out;
    }

    if (bdrv_get_aio_context(target_bs) != aio_context) {
        if (!bdrv_has_blk(target_bs)) {   <----- fails when job is running
            /* The target BDS is not attached, we can safely move it to another
             * AioContext. */
            bdrv_set_aio_context(target_bs, aio_context);
        } else {
            error_setg(errp, "Target is attached to a different thread from "
                             "source.");
            goto out;
        }
    }

Thanks!

signature.asc
Description: PGP signature