[Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[John Snow]

If a device still has an attached BDS because the medium has not yet
been removed, we will be unable to migrate to a new host because
blk_flush will return an error for that backend.

Replace the call to blk_is_available to blk_is_inserted to weaken
the check and allow flushes from the backend to work, while still
disallowing flushes from the frontend/device model to work.

This fixes a regression present in 2.6.0 caused by the following commit:
fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
block: Move some bdrv_*_all() functions to BB

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 34500e6..d1e875e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
 
 int blk_flush(BlockBackend *blk)
 {
-    if (!blk_is_available(blk)) {
+    if (!blk_is_inserted(blk)) {
         return -ENOMEDIUM;
     }
 
-- 
2.4.11

Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[Eric Blake]

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

On 06/10/2016 03:59 PM, John Snow wrote:
> If a device still has an attached BDS because the medium has not yet
> been removed, we will be unable to migrate to a new host because
> blk_flush will return an error for that backend.
> 
> Replace the call to blk_is_available to blk_is_inserted to weaken
> the check and allow flushes from the backend to work, while still
> disallowing flushes from the frontend/device model to work.
> 
> This fixes a regression present in 2.6.0 caused by the following commit:
> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
> block: Move some bdrv_*_all() functions to BB
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/block-backend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Worth testsuite coverage to prevent future regressions?

At any rate,
Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 34500e6..d1e875e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
>  
>  int blk_flush(BlockBackend *blk)
>  {
> -    if (!blk_is_available(blk)) {
> +    if (!blk_is_inserted(blk)) {
>          return -ENOMEDIUM;
>      }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[Fam Zheng]

On Fri, 06/10 17:59, John Snow wrote:
> If a device still has an attached BDS because the medium has not yet
> been removed, we will be unable to migrate to a new host because
> blk_flush will return an error for that backend.
> 
> Replace the call to blk_is_available to blk_is_inserted to weaken
> the check and allow flushes from the backend to work, while still
> disallowing flushes from the frontend/device model to work.
> 
> This fixes a regression present in 2.6.0 caused by the following commit:
> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
> block: Move some bdrv_*_all() functions to BB
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/block-backend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 34500e6..d1e875e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
>  
>  int blk_flush(BlockBackend *blk)
>  {
> -    if (!blk_is_available(blk)) {
> +    if (!blk_is_inserted(blk)) {
>          return -ENOMEDIUM;
>      }
>  
> -- 
> 2.4.11
> 
> 

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

Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[John Snow]

On 06/10/2016 06:42 PM, Eric Blake wrote:
> On 06/10/2016 03:59 PM, John Snow wrote:
>> If a device still has an attached BDS because the medium has not yet
>> been removed, we will be unable to migrate to a new host because
>> blk_flush will return an error for that backend.
>>
>> Replace the call to blk_is_available to blk_is_inserted to weaken
>> the check and allow flushes from the backend to work, while still
>> disallowing flushes from the frontend/device model to work.
>>
>> This fixes a regression present in 2.6.0 caused by the following commit:
>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
>> block: Move some bdrv_*_all() functions to BB
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/block-backend.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Worth testsuite coverage to prevent future regressions?
> 

I'll look into it.

(Ugh, migration tests. wah. etc.)

--js

> At any rate,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 34500e6..d1e875e 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
>>  
>>  int blk_flush(BlockBackend *blk)
>>  {
>> -    if (!blk_is_available(blk)) {
>> +    if (!blk_is_inserted(blk)) {
>>          return -ENOMEDIUM;
>>      }
>>  
>>
> 

Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[Max Reitz]

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

On 10.06.2016 23:59, John Snow wrote:
> If a device still has an attached BDS because the medium has not yet
> been removed, we will be unable to migrate to a new host because
> blk_flush will return an error for that backend.
> 
> Replace the call to blk_is_available to blk_is_inserted to weaken
> the check and allow flushes from the backend to work, while still
> disallowing flushes from the frontend/device model to work.
> 
> This fixes a regression present in 2.6.0 caused by the following commit:
> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
> block: Move some bdrv_*_all() functions to BB
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/block-backend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'm still not sure we shouldn't do the same for blk_{co,aio}_flush(). I
guess you exclude them here because you specifically want to fix the
issue mentioned in the commit message, but then we could just make
blk_flush_all() ignore an -ENOMEDIUM.

I personally think we should make all blk_*flush() functions use
blk_is_inserted() instead of blk_is_available(). As we have discussed on
IRC, there are probably not that many cases a guest can flush a medium
in an open tray anyway (because the main use case are read-only
CD-ROMs), and even if so, that wouldn't change any data, so even if the
guest can actually flush something on an open tray, I don't think anyone
would complain.

Max

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 34500e6..d1e875e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
>  
>  int blk_flush(BlockBackend *blk)
>  {
> -    if (!blk_is_available(blk)) {
> +    if (!blk_is_inserted(blk)) {
>          return -ENOMEDIUM;
>      }
>  
> 



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

Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[John Snow]

On 06/14/2016 09:19 AM, Max Reitz wrote:
> On 10.06.2016 23:59, John Snow wrote:
>> If a device still has an attached BDS because the medium has not yet
>> been removed, we will be unable to migrate to a new host because
>> blk_flush will return an error for that backend.
>>
>> Replace the call to blk_is_available to blk_is_inserted to weaken
>> the check and allow flushes from the backend to work, while still
>> disallowing flushes from the frontend/device model to work.
>>
>> This fixes a regression present in 2.6.0 caused by the following commit:
>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
>> block: Move some bdrv_*_all() functions to BB
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/block-backend.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I'm still not sure we shouldn't do the same for blk_{co,aio}_flush(). I
> guess you exclude them here because you specifically want to fix the
> issue mentioned in the commit message, but then we could just make
> blk_flush_all() ignore an -ENOMEDIUM.

Yeah, I didn't investigate the full path. Just making the minimal fixes.
Is there a concern that this may still leave certain pathways broken
when the CDROM tray is open?

I don't know of any immediately without digging again.

> 
> I personally think we should make all blk_*flush() functions use
> blk_is_inserted() instead of blk_is_available(). As we have discussed on
> IRC, there are probably not that many cases a guest can flush a medium
> in an open tray anyway (because the main use case are read-only
> CD-ROMs), and even if so, that wouldn't change any data, so even if the
> guest can actually flush something on an open tray, I don't think anyone
> would complain.
> 
> Max
> 

I have difficulty making pragmatic arguments when purity is at stake,
but I've already wandered outside of my device model, so I will defer to
your judgment.

>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 34500e6..d1e875e 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
>>  
>>  int blk_flush(BlockBackend *blk)
>>  {
>> -    if (!blk_is_available(blk)) {
>> +    if (!blk_is_inserted(blk)) {
>>          return -ENOMEDIUM;
>>      }
>>  
>>
> 
> 

Is this a NACK unless I attempt to address the wider design issue?

Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[Max Reitz]

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

On 14.06.2016 17:54, John Snow wrote:
> 
> 
> On 06/14/2016 09:19 AM, Max Reitz wrote:
>> On 10.06.2016 23:59, John Snow wrote:
>>> If a device still has an attached BDS because the medium has not yet
>>> been removed, we will be unable to migrate to a new host because
>>> blk_flush will return an error for that backend.
>>>
>>> Replace the call to blk_is_available to blk_is_inserted to weaken
>>> the check and allow flushes from the backend to work, while still
>>> disallowing flushes from the frontend/device model to work.
>>>
>>> This fixes a regression present in 2.6.0 caused by the following commit:
>>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
>>> block: Move some bdrv_*_all() functions to BB
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  block/block-backend.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> I'm still not sure we shouldn't do the same for blk_{co,aio}_flush(). I
>> guess you exclude them here because you specifically want to fix the
>> issue mentioned in the commit message, but then we could just make
>> blk_flush_all() ignore an -ENOMEDIUM.
> 
> Yeah, I didn't investigate the full path. Just making the minimal fixes.
> Is there a concern that this may still leave certain pathways broken
> when the CDROM tray is open?
> 
> I don't know of any immediately without digging again.
> 
>>
>> I personally think we should make all blk_*flush() functions use
>> blk_is_inserted() instead of blk_is_available(). As we have discussed on
>> IRC, there are probably not that many cases a guest can flush a medium
>> in an open tray anyway (because the main use case are read-only
>> CD-ROMs), and even if so, that wouldn't change any data, so even if the
>> guest can actually flush something on an open tray, I don't think anyone
>> would complain.
>>
>> Max
>>
> 
> I have difficulty making pragmatic arguments when purity is at stake,
> but I've already wandered outside of my device model, so I will defer to
> your judgment.
> 
>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>> index 34500e6..d1e875e 100644
>>> --- a/block/block-backend.c
>>> +++ b/block/block-backend.c
>>> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
>>>  
>>>  int blk_flush(BlockBackend *blk)
>>>  {
>>> -    if (!blk_is_available(blk)) {
>>> +    if (!blk_is_inserted(blk)) {
>>>          return -ENOMEDIUM;
>>>      }
>>>  
>>>
>>
>>
> 
> Is this a NACK unless I attempt to address the wider design issue?

I just don't see a point in using blk_is_inserted() here but
blk_is_available() in the other blk_*flush() functions. If
blk_is_inserted() is correct for blk_flush(), it should be correct for
blk_co_flush() and blk_aio_flush(), too.

Maybe I should emphasize that I decided between is_available() and
is_inserted() basically on what felt right to me. There's not really
that much research behind it, so changing it is completely fine.

Max


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

Re: [Qemu-devel] [PATCH] block-backend: allow flush on devices with open tray

[Kevin Wolf]

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

Am 14.06.2016 um 18:13 hat Max Reitz geschrieben:
> On 14.06.2016 17:54, John Snow wrote:
> > 
> > 
> > On 06/14/2016 09:19 AM, Max Reitz wrote:
> >> On 10.06.2016 23:59, John Snow wrote:
> >>> If a device still has an attached BDS because the medium has not yet
> >>> been removed, we will be unable to migrate to a new host because
> >>> blk_flush will return an error for that backend.
> >>>
> >>> Replace the call to blk_is_available to blk_is_inserted to weaken
> >>> the check and allow flushes from the backend to work, while still
> >>> disallowing flushes from the frontend/device model to work.
> >>>
> >>> This fixes a regression present in 2.6.0 caused by the following commit:
> >>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
> >>> block: Move some bdrv_*_all() functions to BB
> >>>
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>> ---
> >>>  block/block-backend.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> I'm still not sure we shouldn't do the same for blk_{co,aio}_flush(). I
> >> guess you exclude them here because you specifically want to fix the
> >> issue mentioned in the commit message, but then we could just make
> >> blk_flush_all() ignore an -ENOMEDIUM.
> > 
> > Yeah, I didn't investigate the full path. Just making the minimal fixes.
> > Is there a concern that this may still leave certain pathways broken
> > when the CDROM tray is open?
> > 
> > I don't know of any immediately without digging again.
> > 
> >>
> >> I personally think we should make all blk_*flush() functions use
> >> blk_is_inserted() instead of blk_is_available(). As we have discussed on
> >> IRC, there are probably not that many cases a guest can flush a medium
> >> in an open tray anyway (because the main use case are read-only
> >> CD-ROMs), and even if so, that wouldn't change any data, so even if the
> >> guest can actually flush something on an open tray, I don't think anyone
> >> would complain.
> >>
> >> Max
> >>
> > 
> > I have difficulty making pragmatic arguments when purity is at stake,
> > but I've already wandered outside of my device model, so I will defer to
> > your judgment.
> > 
> >>> diff --git a/block/block-backend.c b/block/block-backend.c
> >>> index 34500e6..d1e875e 100644
> >>> --- a/block/block-backend.c
> >>> +++ b/block/block-backend.c
> >>> @@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
> >>>  
> >>>  int blk_flush(BlockBackend *blk)
> >>>  {
> >>> -    if (!blk_is_available(blk)) {
> >>> +    if (!blk_is_inserted(blk)) {
> >>>          return -ENOMEDIUM;
> >>>      }
> >>>  
> >>>
> >>
> >>
> > 
> > Is this a NACK unless I attempt to address the wider design issue?
> 
> I just don't see a point in using blk_is_inserted() here but
> blk_is_available() in the other blk_*flush() functions. If
> blk_is_inserted() is correct for blk_flush(), it should be correct for
> blk_co_flush() and blk_aio_flush(), too.

I agree, if we can, we should keep the behaviour consistent between all
interfaces types (sync/AIO/coroutine, byte-based/sector-based) for the
same operation.

Eric also rightfully said that we need a test cases, so a v2 would be
good anyway.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]