[Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Max Reitz]

If a block device is unbacked, a streaming blockjob should immediately
finish instead of beginning to try to stream, then noticing the backing
file does not contain even the first sector (since it does not exist)
and then finishing normally.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/stream.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/stream.c b/block/stream.c
index 694fd42..46bec7d 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -88,6 +88,11 @@ static void coroutine_fn stream_run(void *opaque)
     int n = 0;
     void *buf;
 
+    if (!bs->backing_hd) {
+        block_job_completed(&s->common, 0);
+        return;
+    }
+
     s->common.len = bdrv_getlength(bs);
     if (s->common.len < 0) {
         block_job_completed(&s->common, s->common.len);
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Wenchao Xia]

于 2013/11/14 3:37, Max Reitz 写道:
> If a block device is unbacked, a streaming blockjob should immediately
> finish instead of beginning to try to stream, then noticing the backing
> file does not contain even the first sector (since it does not exist)
> and then finishing normally.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/stream.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 694fd42..46bec7d 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -88,6 +88,11 @@ static void coroutine_fn stream_run(void *opaque)
>       int n = 0;
>       void *buf;
> 
> +    if (!bs->backing_hd) {
> +        block_job_completed(&s->common, 0);
> +        return;
> +    }
> +
>       s->common.len = bdrv_getlength(bs);
>       if (s->common.len < 0) {
>           block_job_completed(&s->common, s->common.len);
> 

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Kevin Wolf]

Am 13.11.2013 um 20:37 hat Max Reitz geschrieben:
> If a block device is unbacked, a streaming blockjob should immediately
> finish instead of beginning to try to stream, then noticing the backing
> file does not contain even the first sector (since it does not exist)
> and then finishing normally.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Thanks, applied to the block-next branch for 1.8.

Kevin

Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Stefan Hajnoczi]

On Wed, Nov 13, 2013 at 08:37:58PM +0100, Max Reitz wrote:
> If a block device is unbacked, a streaming blockjob should immediately
> finish instead of beginning to try to stream, then noticing the backing
> file does not contain even the first sector (since it does not exist)
> and then finishing normally.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/stream.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 694fd42..46bec7d 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -88,6 +88,11 @@ static void coroutine_fn stream_run(void *opaque)
>      int n = 0;
>      void *buf;
>  
> +    if (!bs->backing_hd) {
> +        block_job_completed(&s->common, 0);
> +        return;
> +    }
> +

Thanks for raising this, it's a bug that we don't verify that the image
has a backing file.

I'd rather return an error that the user attempted to do something
pointless.  It was a mistake on their part and it helps to bring this to
their attention right away.

CCing Eric Blake because strictly speaking, adding a error where we
previously used to return success breaks API compatibility.  Any
suggestions (I guess QEMU could just log a warning instead of failing
the command)?

Stefan

Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Eric Blake]

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

On 11/14/2013 06:41 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 13, 2013 at 08:37:58PM +0100, Max Reitz wrote:
>> If a block device is unbacked, a streaming blockjob should immediately
>> finish instead of beginning to try to stream, then noticing the backing
>> file does not contain even the first sector (since it does not exist)
>> and then finishing normally.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/stream.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 694fd42..46bec7d 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -88,6 +88,11 @@ static void coroutine_fn stream_run(void *opaque)
>>      int n = 0;
>>      void *buf;
>>  
>> +    if (!bs->backing_hd) {
>> +        block_job_completed(&s->common, 0);
>> +        return;
>> +    }
>> +
> 
> Thanks for raising this, it's a bug that we don't verify that the image
> has a backing file.
> 
> I'd rather return an error that the user attempted to do something
> pointless.  It was a mistake on their part and it helps to bring this to
> their attention right away.

Conversely, we can argue that streaming a file that has no backing
should SUCCEED as a no-op (there's nothing further to stream, so we are
done right away).

> 
> CCing Eric Blake because strictly speaking, adding a error where we
> previously used to return success breaks API compatibility.  Any
> suggestions (I guess QEMU could just log a warning instead of failing
> the command)?

Turning this into an error would be a regression, and should not be
done.  Based on previous behavior, we have to keep it successful.  But
if I'm reading the patch correctly, it appears that all you are trying
to do is optimize the no-op case to be even more of a no-op (faster
success return, rather than setting up a lot of structure just to find
no further work to do), and not turn a previous success case into an
error.  Adding a log message about a suspicious may make sense, but if
there is no error, will anyone think to check the log?

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Paolo Bonzini]

Il 14/11/2013 14:41, Stefan Hajnoczi ha scritto:
> Thanks for raising this, it's a bug that we don't verify that the image
> has a backing file.
> 
> I'd rather return an error that the user attempted to do something
> pointless.  It was a mistake on their part and it helps to bring this to
> their attention right away.

I like this patch because it avoids the risk of NULL-dereferencing
bs->backing_hd.

See here:

         /* Copy if allocated in the intermediate images.  Limit to the
          * known-unallocated area [sector_num, sector_num+n).  */
         ret = bdrv_is_allocated_above(bs->backing_hd, base,
                                       sector_num, n, &n);

where it's not at all documented that the first argument of
bdrv_is_allocated_above can be NULL.

But such a no-op streaming is valid, just like it is valid to
enable copy-on-read without a backing file.  Think of a system that
starts streaming a disk as soon as the VM starts, because *if* there's a
backing file it knows it is just a template on a remote/slow filesytem.
 It's easier for such a system to invoke the command even if there's no
backing file.

Paolo

Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Stefan Hajnoczi]

On Thu, Nov 14, 2013 at 03:07:09PM +0100, Paolo Bonzini wrote:
> Il 14/11/2013 14:41, Stefan Hajnoczi ha scritto:
> But such a no-op streaming is valid, just like it is valid to
> enable copy-on-read without a backing file.  Think of a system that
> starts streaming a disk as soon as the VM starts, because *if* there's a
> backing file it knows it is just a template on a remote/slow filesytem.
>  It's easier for such a system to invoke the command even if there's no
> backing file.

Since making it an error breaks the API now, let's take this patch and
move on.

I still think it could help users that are issuing the command
incorrectly, but there's not much we can do about that now.

Stefan