| Message ID | 20170419094356.19826-7-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/backup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/backup.c b/block/backup.c > index a4fb288..546c5c5 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -636,7 +636,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, > } > > /* The target must match the source in size, so no resize here either */ > - job->target = blk_new(BLK_PERM_WRITE, > + job->target = blk_new(BLK_PERM_WRITE | BLK_PERM_AIO_CONTEXT_CHANGE, > BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | > BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD); > ret = blk_insert_bs(job->target, target, errp); > -- > 2.9.3 > >
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. Fam
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! Stefan
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. Fam
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!
On Thu, 06/01 14:26, Stefan Hajnoczi wrote: > 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; > } > } Yeah, this is the current way (before this series), and is incomplete in some cases but too strict in others, for obvious reasons. It is changed to always create a BB in patch 7. Fam
diff --git a/block/backup.c b/block/backup.c index a4fb288..546c5c5 100644 --- a/block/backup.c +++ b/block/backup.c @@ -636,7 +636,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, } /* The target must match the source in size, so no resize here either */ - job->target = blk_new(BLK_PERM_WRITE, + job->target = blk_new(BLK_PERM_WRITE | BLK_PERM_AIO_CONTEXT_CHANGE, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD); ret = blk_insert_bs(job->target, target, errp);
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. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/backup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)