Message ID | 1372862071-28225-17-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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
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 --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);
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(-)