* [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices
@ 2013-10-02 14:20 Peter Lieven
2013-10-02 15:06 ` Stefan Hajnoczi
2013-10-02 15:08 ` Eric Blake
0 siblings, 2 replies; 6+ messages in thread
From: Peter Lieven @ 2013-10-02 14:20 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, ronniesahlberg, Peter Lieven, stefanha, anthony, pbonzini
if a raw device like an iscsi target or host device is used
the current implementation makes a second call out to get
the block status of bs->file. however, the raw driver already
has called bdrv_get_block_status on bs->file.
v4: use a flag to detect the raw driver instead of the strncmp
hack.
Signed-off-by: Peter Lieven <pl@kamp.de>
---
block.c | 4 ++--
block/raw_bsd.c | 6 +++++-
include/block/block.h | 3 +++
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 93e113a..7fa2e43 100644
--- a/block.c
+++ b/block.c
@@ -3161,7 +3161,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
if (bs->file &&
(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
- (ret & BDRV_BLOCK_OFFSET_VALID)) {
+ (ret & BDRV_BLOCK_OFFSET_VALID) && !(ret & BDRV_BLOCK_RAW)) {
ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
*pnum, pnum);
if (ret2 >= 0) {
@@ -3172,7 +3172,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
}
}
- return ret;
+ return ret & ~BDRV_BLOCK_RAW;
}
/* Coroutine wrapper for bdrv_get_block_status() */
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index d4ace60..a9e0209 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -62,7 +62,11 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
int64_t sector_num,
int nb_sectors, int *pnum)
{
- return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
+ int64_t ret = bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
+ if (ret < 0) {
+ return ret;
+ }
+ return ret | BDRV_BLOCK_RAW;
}
static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index f808550..cb7019b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -84,6 +84,8 @@ typedef struct BlockDevOps {
/* BDRV_BLOCK_DATA: data is read from bs->file or another file
* BDRV_BLOCK_ZERO: sectors read as zero
* BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
+ * BDRV_BLOCK_RAW: used internally to indicate that the request
+ * was piped through the raw driver
*
* If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
* bs->file where sector data can be read from as raw data.
@@ -105,6 +107,7 @@ typedef struct BlockDevOps {
#define BDRV_BLOCK_DATA 1
#define BDRV_BLOCK_ZERO 2
#define BDRV_BLOCK_OFFSET_VALID 4
+#define BDRV_BLOCK_RAW 8
#define BDRV_BLOCK_OFFSET_MASK BDRV_SECTOR_MASK
typedef enum {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices
2013-10-02 14:20 [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices Peter Lieven
@ 2013-10-02 15:06 ` Stefan Hajnoczi
2013-10-02 15:13 ` Paolo Bonzini
2013-10-02 15:08 ` Eric Blake
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-10-02 15:06 UTC (permalink / raw)
To: Peter Lieven
Cc: kwolf, stefanha, qemu-devel, anthony, pbonzini, ronniesahlberg
On Wed, Oct 02, 2013 at 04:20:36PM +0200, Peter Lieven wrote:
> if a raw device like an iscsi target or host device is used
> the current implementation makes a second call out to get
> the block status of bs->file. however, the raw driver already
> has called bdrv_get_block_status on bs->file.
>
> v4: use a flag to detect the raw driver instead of the strncmp
> hack.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> block.c | 4 ++--
> block/raw_bsd.c | 6 +++++-
> include/block/block.h | 3 +++
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 93e113a..7fa2e43 100644
> --- a/block.c
> +++ b/block.c
> @@ -3161,7 +3161,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>
> if (bs->file &&
> (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
> - (ret & BDRV_BLOCK_OFFSET_VALID)) {
> + (ret & BDRV_BLOCK_OFFSET_VALID) && !(ret & BDRV_BLOCK_RAW)) {
> ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> *pnum, pnum);
> if (ret2 >= 0) {
> @@ -3172,7 +3172,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> }
> }
>
> - return ret;
> + return ret & ~BDRV_BLOCK_RAW;
> }
>
> /* Coroutine wrapper for bdrv_get_block_status() */
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index d4ace60..a9e0209 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -62,7 +62,11 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
> int64_t sector_num,
> int nb_sectors, int *pnum)
> {
> - return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
> + int64_t ret = bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
> + if (ret < 0) {
> + return ret;
> + }
> + return ret | BDRV_BLOCK_RAW;
> }
>
> static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
> diff --git a/include/block/block.h b/include/block/block.h
> index f808550..cb7019b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -84,6 +84,8 @@ typedef struct BlockDevOps {
> /* BDRV_BLOCK_DATA: data is read from bs->file or another file
> * BDRV_BLOCK_ZERO: sectors read as zero
> * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
> + * BDRV_BLOCK_RAW: used internally to indicate that the request
> + * was piped through the raw driver
Sorry I didn't review this earlier but this flag looks hacky and I'm not
confident about merging the patch yet.
The patch makes me wonder if the raw_bsd driver should avoid calling
bs->file itself:
return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
Let block.c:bdrv_co_get_block_status() call down into bs->file.
The problem is then the protocol cannot report unallocated sectors with
this approach.
I think we want to preserve bs' offset while taking the other flags from
bs->file (DATA, ZERO).
Peter, Paolo: What do you think of this approach?
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices
2013-10-02 14:20 [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices Peter Lieven
2013-10-02 15:06 ` Stefan Hajnoczi
@ 2013-10-02 15:08 ` Eric Blake
1 sibling, 0 replies; 6+ messages in thread
From: Eric Blake @ 2013-10-02 15:08 UTC (permalink / raw)
To: Peter Lieven
Cc: kwolf, ronniesahlberg, stefanha, qemu-devel, anthony, pbonzini
[-- Attachment #1: Type: text/plain, Size: 893 bytes --]
On 10/02/2013 08:20 AM, Peter Lieven wrote:
> if a raw device like an iscsi target or host device is used
> the current implementation makes a second call out to get
> the block status of bs->file. however, the raw driver already
> has called bdrv_get_block_status on bs->file.
>
> v4: use a flag to detect the raw driver instead of the strncmp
> hack.
The v4 designation...
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
...should appear after the --- separator (useful to reviewers, but
doesn't need to be part of the git history).
> block.c | 4 ++--
> block/raw_bsd.c | 6 +++++-
> include/block/block.h | 3 +++
> 3 files changed, 10 insertions(+), 3 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
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] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices
2013-10-02 15:06 ` Stefan Hajnoczi
@ 2013-10-02 15:13 ` Paolo Bonzini
2013-10-02 15:34 ` Peter Lieven
2013-10-02 16:02 ` Peter Lieven
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2013-10-02 15:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, anthony, Peter Lieven, qemu-devel, stefanha,
ronniesahlberg
Il 02/10/2013 17:06, Stefan Hajnoczi ha scritto:
> Sorry I didn't review this earlier but this flag looks hacky and I'm not
> confident about merging the patch yet.
>
> The patch makes me wonder if the raw_bsd driver should avoid calling
> bs->file itself:
>
> return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
> (sector_num << BDRV_SECTOR_BITS);
>
> Let block.c:bdrv_co_get_block_status() call down into bs->file.
>
> The problem is then the protocol cannot report unallocated sectors with
> this approach.
>
> I think we want to preserve bs' offset while taking the other flags from
> bs->file (DATA, ZERO).
This would cause other changes. For example, a qcow2 with full metadata
preallocation (i.e. all L2 tables are there but it points to holes)
would not return DATA anymore. I think this is wrong, and especially a
change from the old is_allocated API.
However, a variant on this idea could be to return
BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
(sector_num << BDRV_SECTOR_BITS);
and then BDRV_BLOCK_RAW would mean "take DATA and ZERO from bs->file".
Paolo
> Peter, Paolo: What do you think of this approach?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices
2013-10-02 15:13 ` Paolo Bonzini
@ 2013-10-02 15:34 ` Peter Lieven
2013-10-02 16:02 ` Peter Lieven
1 sibling, 0 replies; 6+ messages in thread
From: Peter Lieven @ 2013-10-02 15:34 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kwolf, stefanha, Stefan Hajnoczi, qemu-devel, anthony,
ronniesahlberg
Am 02.10.2013 17:13, schrieb Paolo Bonzini:
> Il 02/10/2013 17:06, Stefan Hajnoczi ha scritto:
>> Sorry I didn't review this earlier but this flag looks hacky and I'm not
>> confident about merging the patch yet.
>>
>> The patch makes me wonder if the raw_bsd driver should avoid calling
>> bs->file itself:
>>
>> return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
>> (sector_num << BDRV_SECTOR_BITS);
>>
>> Let block.c:bdrv_co_get_block_status() call down into bs->file.
>>
>> The problem is then the protocol cannot report unallocated sectors with
>> this approach.
>>
>> I think we want to preserve bs' offset while taking the other flags from
>> bs->file (DATA, ZERO).
> This would cause other changes. For example, a qcow2 with full metadata
> preallocation (i.e. all L2 tables are there but it points to holes)
> would not return DATA anymore. I think this is wrong, and especially a
> change from the old is_allocated API.
>
> However, a variant on this idea could be to return
>
> BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> (sector_num << BDRV_SECTOR_BITS);
>
> and then BDRV_BLOCK_RAW would mean "take DATA and ZERO from bs->file".
I am fine with that.
Peter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices
2013-10-02 15:13 ` Paolo Bonzini
2013-10-02 15:34 ` Peter Lieven
@ 2013-10-02 16:02 ` Peter Lieven
1 sibling, 0 replies; 6+ messages in thread
From: Peter Lieven @ 2013-10-02 16:02 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kwolf, stefanha, Stefan Hajnoczi, qemu-devel, anthony,
ronniesahlberg
Am 02.10.2013 17:13, schrieb Paolo Bonzini:
> Il 02/10/2013 17:06, Stefan Hajnoczi ha scritto:
>> Sorry I didn't review this earlier but this flag looks hacky and I'm not
>> confident about merging the patch yet.
>>
>> The patch makes me wonder if the raw_bsd driver should avoid calling
>> bs->file itself:
>>
>> return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
>> (sector_num << BDRV_SECTOR_BITS);
>>
>> Let block.c:bdrv_co_get_block_status() call down into bs->file.
>>
>> The problem is then the protocol cannot report unallocated sectors with
>> this approach.
>>
>> I think we want to preserve bs' offset while taking the other flags from
>> bs->file (DATA, ZERO).
> This would cause other changes. For example, a qcow2 with full metadata
> preallocation (i.e. all L2 tables are there but it points to holes)
> would not return DATA anymore. I think this is wrong, and especially a
> change from the old is_allocated API.
>
> However, a variant on this idea could be to return
>
> BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
> (sector_num << BDRV_SECTOR_BITS);
>
> and then BDRV_BLOCK_RAW would mean "take DATA and ZERO from bs->file".
Like this?
diff --git a/block.c b/block.c
index 93e113a..71fab1f 100644
--- a/block.c
+++ b/block.c
@@ -3146,6 +3146,10 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
*pnum = 0;
return ret;
}
+
+ if (ret & BDRV_BLOCK_RAW) {
+ return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
+ }
if (!(ret & BDRV_BLOCK_DATA)) {
if (bdrv_has_zero_init(bs)) {
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index d4ace60..308d605 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -62,7 +62,8 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
int64_t sector_num,
int nb_sectors, int *pnum)
{
- return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
+ return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+ (sector_num << BDRV_SECTOR_BITS);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-02 16:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-02 14:20 [Qemu-devel] [PATCHv4] block/get_block_status: avoid redundant callouts on raw devices Peter Lieven
2013-10-02 15:06 ` Stefan Hajnoczi
2013-10-02 15:13 ` Paolo Bonzini
2013-10-02 15:34 ` Peter Lieven
2013-10-02 16:02 ` Peter Lieven
2013-10-02 15:08 ` Eric Blake
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).