Re: [PATCH v3 2/6] block: block-status cache for data regions

[Hanna Reitz]

On 16.08.21 23:38, Eric Blake wrote:
> On Thu, Aug 12, 2021 at 10:41:44AM +0200, Hanna Reitz wrote:
> > As we have attempted before
> > (https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06451.html,
> > "file-posix: Cache lseek result for data regions";
> > https://lists.nongnu.org/archive/html/qemu-block/2021-02/msg00934.html,
> > "file-posix: Cache next hole"), this patch seeks to reduce the number of
> > SEEK_DATA/HOLE operations the file-posix driver has to perform.  The
> > main difference is that this time it is implemented as part of the
> > general block layer code.
> >
> > We only use the cache for nodes without children (i.e. protocol nodes),
> > because that is where the problem is: Drivers that rely on block-status
> > implementations outside of qemu (e.g. SEEK_DATA/HOLE).
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/307
> > Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> > ---
> > +++ b/block.c
> > +/**
> > + * Check whether [offset, offset + bytes) overlaps with the cached
> > + * block-status data region.
> > + *
> > + * If so, and @pnum is not NULL, set *pnum to `bsc.data_end - offset`,
> > + * which is what bdrv_bsc_is_data()'s interface needs.
> > + * Otherwise, *pnum is not touched.
> Why duplicate this comment,...

I don’t think it can be duplicated, because this is a static function.  
It is very similar to bdrv_bsc_is_data()’s interface, I admit, but it 
isn’t exactly the same (besides the _locked suffix, the only difference 
is that bdrv_bsc_is_data() is for a single byte, while this function 
checks a range).

> > + */
> > +static bool bdrv_bsc_range_overlaps_locked(BlockDriverState *bs,
> > +                                           int64_t offset, int64_t bytes,
> > +                                           int64_t *pnum)
> > +{
> > +    BdrvBlockStatusCache *bsc = qatomic_rcu_read(&bs->block_status_cache);
> > +    bool overlaps;
> > +
> > +    overlaps =
> > +        qatomic_read(&bsc->valid) &&
> > +        ranges_overlap(offset, bytes, bsc->data_start,
> > +                       bsc->data_end - bsc->data_start);
> > +
> > +    if (overlaps && pnum) {
> > +        *pnum = bsc->data_end - offset;
> > +    }
> > +
> > +    return overlaps;
> > +}
> > +
> > +/**
> > + * See block_int.h for this function's documentation.
> > + */
> > +bool bdrv_bsc_is_data(BlockDriverState *bs, int64_t offset, int64_t *pnum)
> > +{
> > +    RCU_READ_LOCK_GUARD();
> > +
> > +    return bdrv_bsc_range_overlaps_locked(bs, offset, 1, pnum);
> > +}
> > +
> > +/**
> > + * See block_int.h for this function's documentation.
> ...but not these?

These are not static functions, and so I can point to the header where 
they’re declared.

(We have a wild mix of where functions are described in qemu, and it’s 
often in their C files.  I prefer having descriptions in the header, but 
because we have the precedent of explaining interfaces in C files, I 
thought I can’t get around adding at least pointers in the C file.)

> But that's minor.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Hanna