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

[Kevin Wolf]

Am 18.06.2019 um 09:38 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.06.2019 16:20, Kevin Wolf wrote:
> > Am 17.06.2019 um 15:09 hat Eric Blake geschrieben:
> >> On 6/17/19 7:09 AM, Kevin Wolf wrote:
> >>
> >>>>>
> >>>>> 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.
> >>
> >> Would 'prefetch' be a more useful name? The name NBD_CMD_CACHE was
> >> chosen in the NBD project, but we are not stuck to that name if we think
> >> something better makes more sense.
> > 
> > Whether 'prefetch' is entirely accurate really depends on the graph
> > configuration, too. But at least it gives me the right idea of "read
> > something, but don't return it yet", so yes, I think that would work for
> > me.
> 
> Do you mean BDRV_REQ_PREFETCH + blk_co_pprefetch, or only the flag?
> Hmm, doubled 'p' in blk_co_pprefetch looks strange, bit it should be
> here for consistency with other requests..

I think I would only do the flag because the wrapper is so trivial, but
this is a matter of taste. The kind of thing that is decided by whoever
writes the patch.

Kevin