[for-2.9?] block/io: Comment out permission assertions
diff mbox

Message ID 20170411145050.31290-1-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz April 11, 2017, 2:50 p.m. UTC
In case of block migration, there may be writes to BlockBackends that do
not have the write permission taken. Before this issue is fixed (which
is not going to happen in 2.9), we therefore cannot assert that this is
the case.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c    |  6 +++++-
 block/io.c | 12 ++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Max Reitz April 11, 2017, 2:52 p.m. UTC | #1
On 11.04.2017 16:50, Max Reitz wrote:
> In case of block migration, there may be writes to BlockBackends that do
> not have the write permission taken. Before this issue is fixed (which
> is not going to happen in 2.9), we therefore cannot assert that this is
> the case.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c    |  6 +++++-
>  block/io.c | 12 ++++++++++--
>  2 files changed, 15 insertions(+), 3 deletions(-)

I'll send a revert for 2.10 if/when this is merged.

Max
Kevin Wolf April 11, 2017, 2:58 p.m. UTC | #2
Am 11.04.2017 um 16:50 hat Max Reitz geschrieben:
> In case of block migration, there may be writes to BlockBackends that do
> not have the write permission taken. Before this issue is fixed (which
> is not going to happen in 2.9), we therefore cannot assert that this is
> the case.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

I tested block migration (migrate -b). Leaving postcopy migration, which
is apparently also broken, to Laurent.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Laurent Vivier April 11, 2017, 3:07 p.m. UTC | #3
On 11/04/2017 16:58, Kevin Wolf wrote:
> Am 11.04.2017 um 16:50 hat Max Reitz geschrieben:
>> In case of block migration, there may be writes to BlockBackends that do
>> not have the write permission taken. Before this issue is fixed (which
>> is not going to happen in 2.9), we therefore cannot assert that this is
>> the case.
>>
>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> I tested block migration (migrate -b). Leaving postcopy migration, which
> is apparently also broken, to Laurent.
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Tested-by: Kevin Wolf <kwolf@redhat.com>
> 

With postcopy migration

Tested-by: Laurent Vivier <lvivier@redhat.com>
Peter Maydell April 11, 2017, 3:47 p.m. UTC | #4
On 11 April 2017 at 15:50, Max Reitz <mreitz@redhat.com> wrote:
> In case of block migration, there may be writes to BlockBackends that do
> not have the write permission taken. Before this issue is fixed (which
> is not going to happen in 2.9), we therefore cannot assert that this is
> the case.
>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Applied, thanks.

-- PMM

Patch
diff mbox

diff --git a/block.c b/block.c
index 086a12df97..1fbbb8d606 100644
--- a/block.c
+++ b/block.c
@@ -3274,7 +3274,11 @@  int bdrv_truncate(BdrvChild *child, int64_t offset)
     BlockDriver *drv = bs->drv;
     int ret;
 
-    assert(child->perm & BLK_PERM_RESIZE);
+    /* FIXME: Some format block drivers use this function instead of implicitly
+     *        growing their file by writing beyond its end.
+     *        See bdrv_aligned_pwritev() for an explanation why we currently
+     *        cannot assert this permission in that case. */
+    // assert(child->perm & BLK_PERM_RESIZE);
 
     if (!drv)
         return -ENOMEDIUM;
diff --git a/block/io.c b/block/io.c
index bae6947032..8706bfa578 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1345,8 +1345,16 @@  static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     assert(!waited || !req->serialising);
     assert(req->overlap_offset <= offset);
     assert(offset + bytes <= req->overlap_offset + req->overlap_bytes);
-    assert(child->perm & BLK_PERM_WRITE);
-    assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
+    /* FIXME: Block migration uses the BlockBackend of the guest device at a
+     *        point when it has not yet taken write permissions. This will be
+     *        fixed by a future patch, but for now we have to bypass this
+     *        assertion for block migration to work. */
+    // assert(child->perm & BLK_PERM_WRITE);
+    /* FIXME: Because of the above, we also cannot guarantee that all format
+     *        BDS take the BLK_PERM_RESIZE permission on their file BDS, since
+     *        they are not obligated to do so if they do not have any parent
+     *        that has taken the permission to write to them. */
+    // assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
 
     ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);