[Qemu-devel] [PATCH for-2.10] Revert "block/io: Comment out permission assertions"

[Qemu-devel] [PATCH for-2.10] Revert "block/io: Comment out permission assertions"

[Max Reitz]

This reverts commit e3e0003a8f6570aba1421ef99a0b383a43371a74.

This commit was necessary for the 2.9 release because we were unable to
fix the underlying issue(s) in time. However, we will be for 2.10.

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

diff --git a/block.c b/block.c
index 1fbbb8d606..086a12df97 100644
--- a/block.c
+++ b/block.c
@@ -3274,11 +3274,7 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
     BlockDriver *drv = bs->drv;
     int ret;
 
-    /* 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);
+    assert(child->perm & BLK_PERM_RESIZE);
 
     if (!drv)
         return -ENOMEDIUM;
diff --git a/block/io.c b/block/io.c
index 8706bfa578..bae6947032 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1345,16 +1345,8 @@ 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);
-    /* 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);
+    assert(child->perm & BLK_PERM_WRITE);
+    assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
 
     ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);
 
-- 
2.12.2

Re: [Qemu-devel] [PATCH for-2.10] Revert "block/io: Comment out permission assertions"

[Fam Zheng]

On Tue, 04/11 17:52, Max Reitz wrote:
> This reverts commit e3e0003a8f6570aba1421ef99a0b383a43371a74.
> 
> This commit was necessary for the 2.9 release because we were unable to
> fix the underlying issue(s) in time. However, we will be for 2.10.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c    |  6 +-----
>  block/io.c | 12 ++----------
>  2 files changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1fbbb8d606..086a12df97 100644
> --- a/block.c
> +++ b/block.c
> @@ -3274,11 +3274,7 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
>      BlockDriver *drv = bs->drv;
>      int ret;
>  
> -    /* 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);
> +    assert(child->perm & BLK_PERM_RESIZE);
>  
>      if (!drv)
>          return -ENOMEDIUM;
> diff --git a/block/io.c b/block/io.c
> index 8706bfa578..bae6947032 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1345,16 +1345,8 @@ 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);
> -    /* 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);
> +    assert(child->perm & BLK_PERM_WRITE);
> +    assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
>  
>      ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);
>  
> -- 
> 2.12.2
> 
> 

Acked-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.10] Revert "block/io: Comment out permission assertions"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2773 bytes --]

On Tue, Apr 11, 2017 at 05:52:26PM +0200, Max Reitz wrote:
> This reverts commit e3e0003a8f6570aba1421ef99a0b383a43371a74.
> 
> This commit was necessary for the 2.9 release because we were unable to
> fix the underlying issue(s) in time. However, we will be for 2.10.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c    |  6 +-----
>  block/io.c | 12 ++----------
>  2 files changed, 3 insertions(+), 15 deletions(-)

Should we merge a fix before enabling the assertion again?  It's a known
issue.  Let people using qemu.git live a little and have fun without the
inevitable SIGABRT coredumps.  We don't benefit if more people encounter
this crash and duplicate work debugging it.

> diff --git a/block.c b/block.c
> index 1fbbb8d606..086a12df97 100644
> --- a/block.c
> +++ b/block.c
> @@ -3274,11 +3274,7 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
>      BlockDriver *drv = bs->drv;
>      int ret;
>  
> -    /* 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);
> +    assert(child->perm & BLK_PERM_RESIZE);
>  
>      if (!drv)
>          return -ENOMEDIUM;
> diff --git a/block/io.c b/block/io.c
> index 8706bfa578..bae6947032 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1345,16 +1345,8 @@ 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);
> -    /* 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);
> +    assert(child->perm & BLK_PERM_WRITE);
> +    assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
>  
>      ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);
>  
> -- 
> 2.12.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH for-2.10] Revert "block/io: Comment out permission assertions"

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

On 13.04.2017 15:28, Stefan Hajnoczi wrote:
> On Tue, Apr 11, 2017 at 05:52:26PM +0200, Max Reitz wrote:
>> This reverts commit e3e0003a8f6570aba1421ef99a0b383a43371a74.
>>
>> This commit was necessary for the 2.9 release because we were unable to
>> fix the underlying issue(s) in time. However, we will be for 2.10.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c    |  6 +-----
>>  block/io.c | 12 ++----------
>>  2 files changed, 3 insertions(+), 15 deletions(-)
> 
> Should we merge a fix before enabling the assertion again?  It's a known
> issue.  Let people using qemu.git live a little and have fun without the
> inevitable SIGABRT coredumps.  We don't benefit if more people encounter
> this crash and duplicate work debugging it.

Yes, we should probably merge the fixes we know about before. But after
that, I'd rather merge this patch as soon as possible so we do have a
chance of getting more reports (if anything else is broken) before the
next freeze. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] Revert "block/io: Comment out permission assertions"

[John Snow]

On 04/13/2017 10:34 AM, Max Reitz wrote:
> On 13.04.2017 15:28, Stefan Hajnoczi wrote:
>> On Tue, Apr 11, 2017 at 05:52:26PM +0200, Max Reitz wrote:
>>> This reverts commit e3e0003a8f6570aba1421ef99a0b383a43371a74.
>>>
>>> This commit was necessary for the 2.9 release because we were unable to
>>> fix the underlying issue(s) in time. However, we will be for 2.10.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  block.c    |  6 +-----
>>>  block/io.c | 12 ++----------
>>>  2 files changed, 3 insertions(+), 15 deletions(-)
>>
>> Should we merge a fix before enabling the assertion again?  It's a known
>> issue.  Let people using qemu.git live a little and have fun without the
>> inevitable SIGABRT coredumps.  We don't benefit if more people encounter
>> this crash and duplicate work debugging it.
> 
> Yes, we should probably merge the fixes we know about before. But after
> that, I'd rather merge this patch as soon as possible so we do have a
> chance of getting more reports (if anything else is broken) before the
> next freeze. :-)
> 
> Max
> 

It's nice to have a working tree, but I think Max has a good point about
wanting to see the reports sooner rather than later, especially if we
want to roll a 2.9.1 very shortly after release.

(Which I think we should.)

--js

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] Revert "block/io: Comment out permission assertions"

[Stefan Hajnoczi]

On Fri, Apr 14, 2017 at 2:10 AM, John Snow <jsnow@redhat.com> wrote:
> On 04/13/2017 10:34 AM, Max Reitz wrote:
>> On 13.04.2017 15:28, Stefan Hajnoczi wrote:
>>> On Tue, Apr 11, 2017 at 05:52:26PM +0200, Max Reitz wrote:
>>>> This reverts commit e3e0003a8f6570aba1421ef99a0b383a43371a74.
>>>>
>>>> This commit was necessary for the 2.9 release because we were unable to
>>>> fix the underlying issue(s) in time. However, we will be for 2.10.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  block.c    |  6 +-----
>>>>  block/io.c | 12 ++----------
>>>>  2 files changed, 3 insertions(+), 15 deletions(-)
>>>
>>> Should we merge a fix before enabling the assertion again?  It's a known
>>> issue.  Let people using qemu.git live a little and have fun without the
>>> inevitable SIGABRT coredumps.  We don't benefit if more people encounter
>>> this crash and duplicate work debugging it.
>>
>> Yes, we should probably merge the fixes we know about before. But after
>> that, I'd rather merge this patch as soon as possible so we do have a
>> chance of getting more reports (if anything else is broken) before the
>> next freeze. :-)
>>
>> Max
>>
>
> It's nice to have a working tree, but I think Max has a good point about
> wanting to see the reports sooner rather than later, especially if we
> want to roll a 2.9.1 very shortly after release.
>
> (Which I think we should.)

I interpreted Max's reply to mean "okay, let's merge a fix first and
then immediately enable the assertion again".

Your reply seems to interpret Max's email as "the assertion might
catch other bugs so it should be enabled immediately without a fix".

Those two are not the same :).

Stefan