* [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices
@ 2013-10-07 5:59 Peter Lieven
2013-10-07 8:25 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Peter Lieven @ 2013-10-07 5:59 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.
Signed-off-by: Peter Lieven <pl@kamp.de>
---
v5: add a generic get_lba_status function in the raw driver which
adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
handle the callout to bs->file then.
v4: use a flag to detect the raw driver instead of the strncmp
hack
block.c | 4 ++++
block/raw_bsd.c | 3 ++-
include/block/block.h | 4 ++++
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 93e113a..38a589e 100644
--- a/block.c
+++ b/block.c
@@ -3147,6 +3147,10 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
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)) {
ret |= BDRV_BLOCK_ZERO;
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);
}
static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index f808550..003699e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -84,6 +84,9 @@ 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 answered by the raw driver and that one
+ * should look in bs->file directly.
*
* 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 +108,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] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices
2013-10-07 5:59 [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices Peter Lieven
@ 2013-10-07 8:25 ` Paolo Bonzini
2013-10-07 9:25 ` Peter Lieven
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Paolo Bonzini @ 2013-10-07 8:25 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, anthony, stefanha, qemu-devel, ronniesahlberg
Il 07/10/2013 07:59, Peter Lieven ha scritto:
> 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.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> v5: add a generic get_lba_status function in the raw driver which
> adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
> handle the callout to bs->file then.
>
> v4: use a flag to detect the raw driver instead of the strncmp
> hack
>
> block.c | 4 ++++
> block/raw_bsd.c | 3 ++-
> include/block/block.h | 4 ++++
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 93e113a..38a589e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3147,6 +3147,10 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> return ret;
> }
>
> + if (ret & BDRV_BLOCK_RAW) {
> + return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
Strictly speaking, this should probably do something like this:
assert(ret & BDRV_BLOCK_OFFSET_VALID);
return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
nb_sectors, pnum);
Or alternatively the raw driver should return just "BDRV_BLOCK_RAW".
As a third option, the raw driver could also return not just
BDRV_BLOCK_RAW and BDRV_BLOCK_OFFSET_VALID, but also BDRV_BLOCK_DATA (so
that the answer makes some sense even without going down to bs->file).
But I'll let the block maintainers decide what to do.
Paolo
> + }
> +
> if (!(ret & BDRV_BLOCK_DATA)) {
> if (bdrv_has_zero_init(bs)) {
> ret |= BDRV_BLOCK_ZERO;
> 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);
> }
>
> static int coroutine_fn raw_co_write_zeroes(BlockDriverState *bs,
> diff --git a/include/block/block.h b/include/block/block.h
> index f808550..003699e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -84,6 +84,9 @@ 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 answered by the raw driver and that one
> + * should look in bs->file directly.
> *
> * 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 +108,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 {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices
2013-10-07 8:25 ` Paolo Bonzini
@ 2013-10-07 9:25 ` Peter Lieven
2013-10-08 11:13 ` Stefan Hajnoczi
2013-10-08 12:05 ` Peter Lieven
2 siblings, 0 replies; 6+ messages in thread
From: Peter Lieven @ 2013-10-07 9:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, anthony, stefanha, qemu-devel, ronniesahlberg
On 07.10.2013 10:25, Paolo Bonzini wrote:
> Il 07/10/2013 07:59, Peter Lieven ha scritto:
>> 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.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> v5: add a generic get_lba_status function in the raw driver which
>> adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
>> handle the callout to bs->file then.
>>
>> v4: use a flag to detect the raw driver instead of the strncmp
>> hack
>>
>> block.c | 4 ++++
>> block/raw_bsd.c | 3 ++-
>> include/block/block.h | 4 ++++
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 93e113a..38a589e 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3147,6 +3147,10 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>> return ret;
>> }
>>
>> + if (ret & BDRV_BLOCK_RAW) {
>> + return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
> Strictly speaking, this should probably do something like this:
>
> assert(ret & BDRV_BLOCK_OFFSET_VALID);
> return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> nb_sectors, pnum);
>
> Or alternatively the raw driver should return just "BDRV_BLOCK_RAW".
>
> As a third option, the raw driver could also return not just
> BDRV_BLOCK_RAW and BDRV_BLOCK_OFFSET_VALID, but also BDRV_BLOCK_DATA (so
> that the answer makes some sense even without going down to bs->file).
>
> But I'll let the block maintainers decide what to do.
Okay, I will wait for their feedback.
Peter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices
2013-10-07 8:25 ` Paolo Bonzini
2013-10-07 9:25 ` Peter Lieven
@ 2013-10-08 11:13 ` Stefan Hajnoczi
2013-10-08 12:05 ` Peter Lieven
2 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-10-08 11:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, Stefan Hajnoczi, Peter Lieven, qemu-devel,
ronnie sahlberg, Anthony Liguori
On Mon, Oct 7, 2013 at 10:25 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 07/10/2013 07:59, Peter Lieven ha scritto:
>> 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.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> v5: add a generic get_lba_status function in the raw driver which
>> adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
>> handle the callout to bs->file then.
>>
>> v4: use a flag to detect the raw driver instead of the strncmp
>> hack
>>
>> block.c | 4 ++++
>> block/raw_bsd.c | 3 ++-
>> include/block/block.h | 4 ++++
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 93e113a..38a589e 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3147,6 +3147,10 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>> return ret;
>> }
>>
>> + if (ret & BDRV_BLOCK_RAW) {
>> + return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
>
> Strictly speaking, this should probably do something like this:
>
> assert(ret & BDRV_BLOCK_OFFSET_VALID);
> return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> nb_sectors, pnum);
>
> Or alternatively the raw driver should return just "BDRV_BLOCK_RAW".
>
> As a third option, the raw driver could also return not just
> BDRV_BLOCK_RAW and BDRV_BLOCK_OFFSET_VALID, but also BDRV_BLOCK_DATA (so
> that the answer makes some sense even without going down to bs->file).
>
> But I'll let the block maintainers decide what to do.
The return value is a bitfield, so let's use those bits and take Option 3.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices
2013-10-07 8:25 ` Paolo Bonzini
2013-10-07 9:25 ` Peter Lieven
2013-10-08 11:13 ` Stefan Hajnoczi
@ 2013-10-08 12:05 ` Peter Lieven
2013-10-08 12:36 ` Paolo Bonzini
2 siblings, 1 reply; 6+ messages in thread
From: Peter Lieven @ 2013-10-08 12:05 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, anthony, stefanha, qemu-devel, ronniesahlberg
On 07.10.2013 10:25, Paolo Bonzini wrote:
> Il 07/10/2013 07:59, Peter Lieven ha scritto:
>> 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.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> v5: add a generic get_lba_status function in the raw driver which
>> adds the BDRV_BLOCK_RAW flag. bdrv_co_get_block_status will
>> handle the callout to bs->file then.
>>
>> v4: use a flag to detect the raw driver instead of the strncmp
>> hack
>>
>> block.c | 4 ++++
>> block/raw_bsd.c | 3 ++-
>> include/block/block.h | 4 ++++
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 93e113a..38a589e 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3147,6 +3147,10 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>> return ret;
>> }
>>
>> + if (ret & BDRV_BLOCK_RAW) {
>> + return bdrv_get_block_status(bs->file, sector_num, nb_sectors, pnum);
> Strictly speaking, this should probably do something like this:
>
> assert(ret & BDRV_BLOCK_OFFSET_VALID);
> return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> nb_sectors, pnum);
shouldn't the last line be:
return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
*pnum, pnum);
This would of course require *pnum = nb_sectors in raw_co_get_block_status
?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices
2013-10-08 12:05 ` Peter Lieven
@ 2013-10-08 12:36 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2013-10-08 12:36 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, anthony, stefanha, qemu-devel, ronniesahlberg
Il 08/10/2013 14:05, Peter Lieven ha scritto:
>> Strictly speaking, this should probably do something like this:
>>
>> assert(ret & BDRV_BLOCK_OFFSET_VALID);
>> return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
>> nb_sectors, pnum);
> shouldn't the last line be:
>
> return bdrv_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> *pnum, pnum);
>
>
> This would of course require *pnum = nb_sectors in raw_co_get_block_status
> ?
Yes for both questions.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-08 12:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-07 5:59 [Qemu-devel] [PATCHv5] block/get_block_status: avoid redundant callouts on raw devices Peter Lieven
2013-10-07 8:25 ` Paolo Bonzini
2013-10-07 9:25 ` Peter Lieven
2013-10-08 11:13 ` Stefan Hajnoczi
2013-10-08 12:05 ` Peter Lieven
2013-10-08 12:36 ` Paolo Bonzini
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).