| Message ID | 20170913181910.29688-14-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/mirror: Add active-sync mirroring | expand |
Am 13.09.2017 um 20:19 hat Max Reitz geschrieben: > The owner of the mirror BDS might retire its write permission; but there > may still be pending mirror operations so the mirror BDS cannot > necessarily retire its write permission for its child then. > > Signed-off-by: Max Reitz <mreitz@redhat.com> I'm confused. The child of mirror_top_bs is the source, but don't mirror operations only write to the target? Kevin
On 2017-10-10 11:58, Kevin Wolf wrote: > Am 13.09.2017 um 20:19 hat Max Reitz geschrieben: >> The owner of the mirror BDS might retire its write permission; but there >> may still be pending mirror operations so the mirror BDS cannot >> necessarily retire its write permission for its child then. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > I'm confused. The child of mirror_top_bs is the source, but don't mirror > operations only write to the target? I do know that the iotests added in the end fails without this patch, if it helps. :-) Right, for some reason I never thought about this... OK, so the issue is that if you create a BB, submit a request and then delete it (the only BB), permissions requirements for the mirror BDS are dropped and then it in turn also drops its permissions on the source. The issue now occurs whenever the BB is deleted before the write request checks the permissions on the source. In passive mode, this does not happen because nothing yields before the permission check. In active mode, however, active_write_prepare() may yield due to mirror_wait_on_conflicts(). Since active_write_prepare() also creates an operation (before yielding), this patch "fixes" the issue. I think the real bug fix would be to also have a counter of (write) operations running on the source (incremented/decremented in bdrv_mirror_top_pwritev()) and to evaluate that in bdrv_mirror_top_child_perm(). Max
diff --git a/block/mirror.c b/block/mirror.c index 05410c94ca..612fab660e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1236,6 +1236,7 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c, uint64_t *nperm, uint64_t *nshared) { MirrorBDSOpaque *s = bs->opaque; + bool ops_in_flight = s->job && !QTAILQ_EMPTY(&s->job->ops_in_flight); if (s->job && s->job->exiting) { *nperm = 0; @@ -1243,9 +1244,10 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c, return; } - /* Must be able to forward guest writes to the real image */ + /* Must be able to forward both new and pending guest writes to + * the real image */ *nperm = 0; - if (perm & BLK_PERM_WRITE) { + if ((perm & BLK_PERM_WRITE) || ops_in_flight) { *nperm |= BLK_PERM_WRITE; }
The owner of the mirror BDS might retire its write permission; but there may still be pending mirror operations so the mirror BDS cannot necessarily retire its write permission for its child then. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/mirror.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)