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

[Kevin Wolf]

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.

Kevin