diff mbox

[16/17] block: add default get_block_status implementation for protocols

Message ID 1372862071-28225-17-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini July 3, 2013, 2:34 p.m. UTC 
Protocols return raw data, so you can assume the offsets to pass
through unchanged.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Peter Lieven July 16, 2013, 6:47 a.m. UTC | #1 
On 03.07.2013 16:34, Paolo Bonzini wrote:
> Protocols return raw data, so you can assume the offsets to pass
> through unchanged.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index cd371cd..ff8ced7 100644
> --- a/block.c
> +++ b/block.c
> @@ -2977,7 +2977,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>   
>       if (!bs->drv->bdrv_co_get_block_status) {
>           *pnum = nb_sectors;
> -        return BDRV_BLOCK_DATA;
> +        ret = BDRV_BLOCK_DATA;
> +        if (bs->drv->protocol_name) {
> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
> +        }
> +        return ret;
>       }
>   
>       ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
I am curious if this is right. Doesn't this mean we say that at offset
sector_num * BDRV_SECTOR_SIZE are nb_sectors of linear data? This is
something we do not know for sure.

Peter
Paolo Bonzini July 16, 2013, 7:19 a.m. UTC | #2 
Il 16/07/2013 08:47, Peter Lieven ha scritto:
>>
>> @@ -2977,7 +2977,11 @@ static int64_t coroutine_fn
>> bdrv_co_get_block_status(BlockDriverState *bs,
>>         if (!bs->drv->bdrv_co_get_block_status) {
>>           *pnum = nb_sectors;
>> -        return BDRV_BLOCK_DATA;
>> +        ret = BDRV_BLOCK_DATA;
>> +        if (bs->drv->protocol_name) {
>> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num *
>> BDRV_SECTOR_SIZE);
>> +        }
>> +        return ret;
>>       }
>>         ret = bs->drv->bdrv_co_get_block_status(bs, sector_num,
>> nb_sectors, pnum);
> I am curious if this is right. Doesn't this mean we say that at offset
> sector_num * BDRV_SECTOR_SIZE are nb_sectors of linear data? This is
> something we do not know for sure.

Only for protocols.  In this case, we do know, or format=raw wouldn't
work.  This is not propagated up to the actual BDS for the image, except
for format=raw.

Paolo
Peter Lieven July 16, 2013, 7:54 a.m. UTC | #3 
On 16.07.2013 09:19, Paolo Bonzini wrote:
> Il 16/07/2013 08:47, Peter Lieven ha scritto:
>>> @@ -2977,7 +2977,11 @@ static int64_t coroutine_fn
>>> bdrv_co_get_block_status(BlockDriverState *bs,
>>>          if (!bs->drv->bdrv_co_get_block_status) {
>>>            *pnum = nb_sectors;
>>> -        return BDRV_BLOCK_DATA;
>>> +        ret = BDRV_BLOCK_DATA;
>>> +        if (bs->drv->protocol_name) {
>>> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num *
>>> BDRV_SECTOR_SIZE);
>>> +        }
>>> +        return ret;
>>>        }
>>>          ret = bs->drv->bdrv_co_get_block_status(bs, sector_num,
>>> nb_sectors, pnum);
>> I am curious if this is right. Doesn't this mean we say that at offset
>> sector_num * BDRV_SECTOR_SIZE are nb_sectors of linear data? This is
>> something we do not know for sure.
> Only for protocols.  In this case, we do know, or format=raw wouldn't
> work.  This is not propagated up to the actual BDS for the image, except
> for format=raw.
If bs->drv->protocol_name is only != NULL if format=raw, I am
fine with this.

Peter
Paolo Bonzini July 16, 2013, 9:37 a.m. UTC | #4 
Il 16/07/2013 09:54, Peter Lieven ha scritto:
> On 16.07.2013 09:19, Paolo Bonzini wrote:
>> Il 16/07/2013 08:47, Peter Lieven ha scritto:
>>>> @@ -2977,7 +2977,11 @@ static int64_t coroutine_fn
>>>> bdrv_co_get_block_status(BlockDriverState *bs,
>>>>          if (!bs->drv->bdrv_co_get_block_status) {
>>>>            *pnum = nb_sectors;
>>>> -        return BDRV_BLOCK_DATA;
>>>> +        ret = BDRV_BLOCK_DATA;
>>>> +        if (bs->drv->protocol_name) {
>>>> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num *
>>>> BDRV_SECTOR_SIZE);
>>>> +        }
>>>> +        return ret;
>>>>        }
>>>>          ret = bs->drv->bdrv_co_get_block_status(bs, sector_num,
>>>> nb_sectors, pnum);
>>> I am curious if this is right. Doesn't this mean we say that at offset
>>> sector_num * BDRV_SECTOR_SIZE are nb_sectors of linear data? This is
>>> something we do not know for sure.
>> Only for protocols.  In this case, we do know, or format=raw wouldn't
>> work.  This is not propagated up to the actual BDS for the image, except
>> for format=raw.
> If bs->drv->protocol_name is only != NULL if format=raw, I am
> fine with this.

No, bs->drv->protocol_name is only != NULL if it is a protocol (like
file or iscsi) rather than a format (like raw or qcow2). :)

But the user will never call bdrv_co_get_block_status on a protocol
(bs->file, roughly), only on a format (bs); and this information is only
passed from bs->file to bs for format=raw.  Other formats compute the
offsets themselves.

Paolo
Peter Lieven July 17, 2013, 10:26 a.m. UTC | #5 
Am 16.07.2013 um 09:19 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> Il 16/07/2013 08:47, Peter Lieven ha scritto:
>>> 
>>> @@ -2977,7 +2977,11 @@ static int64_t coroutine_fn
>>> bdrv_co_get_block_status(BlockDriverState *bs,
>>>        if (!bs->drv->bdrv_co_get_block_status) {
>>>          *pnum = nb_sectors;
>>> -        return BDRV_BLOCK_DATA;
>>> +        ret = BDRV_BLOCK_DATA;
>>> +        if (bs->drv->protocol_name) {
>>> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num *
>>> BDRV_SECTOR_SIZE);
>>> +        }
>>> +        return ret;
>>>      }
>>>        ret = bs->drv->bdrv_co_get_block_status(bs, sector_num,
>>> nb_sectors, pnum);
>> I am curious if this is right. Doesn't this mean we say that at offset
>> sector_num * BDRV_SECTOR_SIZE are nb_sectors of linear data? This is
>> something we do not know for sure.
> 
> Only for protocols.  In this case, we do know, or format=raw wouldn't
> work.  This is not propagated up to the actual BDS for the image, except
> for format=raw.

Wouldn't it be better to add this general tweak in to raw_co_is_allocated
rather than at the block level?

What you write above is true, but you will never know what will happen in the
future. For raw this tweak is right, but for anything developed in the future
it might not be and in the end nobody remembers to fix it at this position.

I was just thinking of the has_zero_init = 1 thing. At the time it was written
the code was correct and then came more and more extensions like
extends on a block device or iscsi and nobody remembered.

Peter
Paolo Bonzini July 17, 2013, 10:33 a.m. UTC | #6 
Il 17/07/2013 12:26, Peter Lieven ha scritto:
> 
> Am 16.07.2013 um 09:19 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
>> Il 16/07/2013 08:47, Peter Lieven ha scritto:
>>>>
>>>> @@ -2977,7 +2977,11 @@ static int64_t coroutine_fn
>>>> bdrv_co_get_block_status(BlockDriverState *bs,
>>>>        if (!bs->drv->bdrv_co_get_block_status) {
>>>>          *pnum = nb_sectors;
>>>> -        return BDRV_BLOCK_DATA;
>>>> +        ret = BDRV_BLOCK_DATA;
>>>> +        if (bs->drv->protocol_name) {
>>>> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num *
>>>> BDRV_SECTOR_SIZE);
>>>> +        }
>>>> +        return ret;
>>>>      }
>>>>        ret = bs->drv->bdrv_co_get_block_status(bs, sector_num,
>>>> nb_sectors, pnum);
>>> I am curious if this is right. Doesn't this mean we say that at offset
>>> sector_num * BDRV_SECTOR_SIZE are nb_sectors of linear data? This is
>>> something we do not know for sure.
>>
>> Only for protocols.  In this case, we do know, or format=raw wouldn't
>> work.  This is not propagated up to the actual BDS for the image, except
>> for format=raw.
> 
> Wouldn't it be better to add this general tweak in to raw_co_is_allocated
> rather than at the block level?

No, this is not a "general tweak".  It is a default implementation (it
is protected by "if (!bs->drv->bdrv_co_get_block_status)".  Other
implementations can do the same, for example raw-posix.c also returns
BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE).

Each layer must return complete data.  If a higher layer wants to query
bs->file, it must obviously filters those flags that still make sense.

In the case of format=raw, all flags make sense.  For other layers, some
flags may not make sense and have to be stripped.

> What you write above is true, but you will never know what will happen in the
> future. For raw this tweak is right, but for anything developed in the future
> it might not be and in the end nobody remembers to fix it at this position.

Does the above explain it better?

Paolo
diff mbox

Patch

diff --git a/block.c b/block.c
index cd371cd..ff8ced7 100644
--- a/block.c
+++ b/block.c
@@ -2977,7 +2977,11 @@  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
 
     if (!bs->drv->bdrv_co_get_block_status) {
         *pnum = nb_sectors;
-        return BDRV_BLOCK_DATA;
+        ret = BDRV_BLOCK_DATA;
+        if (bs->drv->protocol_name) {
+            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+        }
+        return ret;
     }
 
     ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);