Re: [Qemu-devel] [PATCH v2 07/12] block/json: Add bdrv_co_get_block_status()

[Max Reitz <mreitz@redhat.com>]

On 08.04.2014 22:53, Peter Lieven wrote:
> Am 08.04.2014 um 15:15 schrieb Max Reitz <mreitz@redhat.com>:
>
>> On 07.03.2014 23:55, Max Reitz wrote:
>>> Implement this function in the same way as raw_bsd does: Acknowledge
>>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
>>> and BDRV_BLOCK_DATA and derive the offset directly from the sector
>>> index) and add BDRV_BLOCK_RAW to the returned value.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block/json.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>> Ping – Benoît is unsure of BDRV_BLOCK_RAW, therefore he elected not to give a reviewed-by for this patch.
>>
>> The commit introducing BDRV_BLOCK_RAW (92bc50a5ad7fbc9a0bd17240eaea5027a100ca79) is signed-off-by Peter, reviewed-by Eric and signed-off-by Kevin (as the committer, I suppose). Could anyone of you comment on this patch? The question is whether to use BDRV_BLOCK_RAW or a recursive call to bdrv_get_block_status() here. I mean, I could just replace the BDRV_BLOCK_RAW by a recursive call to bdrv_get_block_status() and Benoît would probably approve, but obviously that would be cheating.
> Sorry, I missed Benoits earlier email. I have not fully looked through the block/json patch set, but as far as I understand its a filter that can be on top of any format/protocol combination.
> Therefore the only right solution can be to pass the bdrv_co_get_block_status call to the underlying format driver.
>
> As for the BDRV_BLOCK_RAW flag we introduced this for the special case of the raw format which guarantees a linear mapping of the whole drive to the underlaying protocol e.g.
> file, iscsi, host_device, nfs… Therefore we can derive the file offset from the sector. The allocation status has to be queried from the protocol driver. In fact in the raw format
> case it would also work to pass the call to bs->file, but this would result in 2 calls to bs->file->drv->bdrv_get_block_status for unallocated blocks. Remember that bdrv_get_block_status
> can be an expensive call e.g. in the iSCSI case. This is why I made commit 92bc50a5.
>
> Peter

Thank you very much. I'll send a v3 without BDRV_BLOCK_RAW then.

Max