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(-)

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;
     }
 
-- 
2.13.5

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