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

[Hanna Reitz <hreitz@redhat.com>]

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