Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

17.06.2019 15:09, Kevin Wolf wrote:
> Am 17.06.2019 um 13:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 06.06.2019 17:07, Vladimir Sementsov-Ogievskiy wrote:
>>> 06.06.2019 16:55, Eric Blake wrote:
>>>> On 6/6/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all!
>>>>>
>>>>> Here is small new io API: blk_co_pcache, which does copy-on-read without
>>>>> extra buffer for read data. This means that only parts that needs COR
>>>>> will be actually read and only corresponding buffers allocated, no more.
>>>>>
>>>>> This allows to improve a bit block-stream and NBD_CMD_CACHE
>>>>
>>>> I'd really like to see qemu-io gain access to calling this command, so
>>>> that we can add iotests coverage of this new feature. Note that the
>>>> in-development libnbd
>>>> (https://github.com/libguestfs/libnbd/commits/master) is also usable as
>>>> an NBD client that can drive NBD_CMD_CACHE, although it's still new
>>>> enough that we probably don't want to rely on it being available yet.
>>>>
>>>
>>> Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on 
>>> nbd driver?
>>> I didn't implement it. But may be I should..
>>>
>>> May aim was only to avoid extra allocation and unnecessary reads. But if we 
>>> implement
>>> full-featured io request, what should it do?
>>>
>>> On qcow2 with backing it should pull data from backing to top, like in 
>>> copy-on-read.
>>> And for nbd it will send NBD_CMD_CACHE?
>>> These semantics seems different, but why not?
>>>
>>
>> Any opinions here? Should I resend or could we use it as a first step,
>> not touching external API but improving things a little bit?
> 
> I'm not opposed to making only a first step now. The interface seems to
> make sense to me; the only thing that I don't really like is the naming
> both of the operation (blk_co_pcache) and of the flag (BDRV_REQ_CACHE)
> because to me, "cache" doesn't mean "read, but ignore the result".
> 
> The operation only results in something being cached if the block graph
> is configured to cache things. And indeed, the most importatn use case
> for the flag is not populating a cache, but triggering copy-on-read. So
> I think calling this operation "cache" is misleading.
> 
> Now, I don't have very good ideas for better names either. I guess
> BDRV_REQ_NO_DATA could work, though it's not perfect. (Also, not sure if
> a blk_co_preadv_no_read wrapper is even needed when you can just call
> blk_co_preadv with the right flag.)
> 
> I'm open for good naming ideas.
> 

My first try (not published) was BDRV_REQ_FAKE_READ, passed as flag to 
blk_co_preadv,
without separate io request function.

I decided to make it to be Cache request inspired by NBD_CMD_CACHE, which was 
created
to do exactly copy-on-read operation. So if we call it cache it will correspond 
to
NBD protocol.

_NO_DATA also works for me, not a problem to resend with this flag and without 
additional
wrapper, as a first step.


-- 
Best regards,
Vladimir