* [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices
@ 2013-11-13 19:37 Max Reitz
2013-11-14 3:00 ` Wenchao Xia
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Max Reitz @ 2013-11-13 19:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices
2013-11-13 19:37 [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices Max Reitz
@ 2013-11-14 3:00 ` Wenchao Xia
2013-11-14 12:15 ` Kevin Wolf
2013-11-14 13:41 ` Stefan Hajnoczi
2 siblings, 0 replies; 7+ messages in thread
From: Wenchao Xia @ 2013-11-14 3:00 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini, Stefan Hajnoczi
于 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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices
2013-11-13 19:37 [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices Max Reitz
2013-11-14 3:00 ` Wenchao Xia
@ 2013-11-14 12:15 ` Kevin Wolf
2013-11-14 13:41 ` Stefan Hajnoczi
2 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2013-11-14 12:15 UTC (permalink / raw)
To: Max Reitz; +Cc: Paolo Bonzini, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices
2013-11-13 19:37 [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices Max Reitz
2013-11-14 3:00 ` Wenchao Xia
2013-11-14 12:15 ` Kevin Wolf
@ 2013-11-14 13:41 ` Stefan Hajnoczi
2013-11-14 14:00 ` Eric Blake
2013-11-14 14:07 ` Paolo Bonzini
2 siblings, 2 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2013-11-14 13:41 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices
2013-11-14 13:41 ` Stefan Hajnoczi
@ 2013-11-14 14:00 ` Eric Blake
2013-11-14 14:07 ` Paolo Bonzini
1 sibling, 0 replies; 7+ messages in thread
From: Eric Blake @ 2013-11-14 14:00 UTC (permalink / raw)
To: Stefan Hajnoczi, Max Reitz
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices
2013-11-14 13:41 ` Stefan Hajnoczi
2013-11-14 14:00 ` Eric Blake
@ 2013-11-14 14:07 ` Paolo Bonzini
2013-11-15 8:18 ` Stefan Hajnoczi
1 sibling, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2013-11-14 14:07 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Max Reitz
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices
2013-11-14 14:07 ` Paolo Bonzini
@ 2013-11-15 8:18 ` Stefan Hajnoczi
0 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2013-11-15 8:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Max Reitz
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-11-15 8:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-13 19:37 [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices Max Reitz
2013-11-14 3:00 ` Wenchao Xia
2013-11-14 12:15 ` Kevin Wolf
2013-11-14 13:41 ` Stefan Hajnoczi
2013-11-14 14:00 ` Eric Blake
2013-11-14 14:07 ` Paolo Bonzini
2013-11-15 8:18 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.