Re: [Qemu-devel] [PATCH] block/raw-posix: Fall back to SEEK_HOLE/SEEK_DATA

[Max Reitz <mreitz@redhat.com>]

On 06.05.2014 13:49, Stefan Hajnoczi wrote:
> On Mon, May 05, 2014 at 10:01:39PM +0200, Max Reitz wrote:
>> The current version of raw-posix always uses ioctl(FS_IOC_FIEMAP) if
>> FIEMAP is available; lseek with SEEK_HOLE/SEEK_DATA are not even
>> compiled in in this case. However, there may be implementations which
>> support the latter but not the former (e.g., NFSv4.2). In this case,
>> raw-posix should fall back to lseek with SEEK_HOLE/SEEK_DATA if FIEMAP
>> does not work.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> As Linux does not yet implement NFSv4.2 (I don't even know whether the
>> specification is complete), I have no way of testing whether this
>> actually works for the proposed case. But as it doesn't break any of the
>> existing test cases, it should be fine.
>> ---
>>   block/raw-posix.c | 27 +++++++++++++++++++++------
>>   1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 3ce026d..e523633 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -146,6 +146,9 @@ typedef struct BDRVRawState {
>>       bool has_discard:1;
>>       bool has_write_zeroes:1;
>>       bool discard_zeroes:1;
>> +#if defined CONFIG_FIEMAP && defined SEEK_HOLE && defined SEEK_DATA
>> +    bool use_seek_hole_data;
>> +#endif
>>   } BDRVRawState;
>>   
>>   typedef struct BDRVRawReopenState {
>> @@ -1291,6 +1294,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>>                                               int64_t sector_num,
>>                                               int nb_sectors, int *pnum)
>>   {
>> +    BDRVRawState *s = bs->opaque;
>>       off_t start, data, hole;
>>       int64_t ret;
>>   
>> @@ -1303,22 +1307,31 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>>       ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
>>   
>>   #ifdef CONFIG_FIEMAP
>> -
>> -    BDRVRawState *s = bs->opaque;
>>       struct {
>>           struct fiemap fm;
>>           struct fiemap_extent fe;
>>       } f;
>>   
>> +#if defined SEEK_HOLE && defined SEEK_DATA
>> +    if (s->use_seek_hole_data) {
>> +        goto try_seek_hole_data;
>> +    }
>> +#endif
> This is becoming hard to read due to the ifdefs and their relationships.
>
> A minor simplification is to change the bool field:
> #if defined CONFIG_FIEMAP
> bool use_fiemap;
> #endif
>
> ...
>
> #if defined CONFIG_FIEMAP
> if (s->use_fiemap) {
>      ...try fiemap...
> }
> #endif /* CONFIG_FIEMAP */
>
> use_fiemap is not dependent on SEEK_HOLE/SEEK_DATA.  That reduces the
> amount of ifdefs needed.
>
> A bigger cleanup is extracting the FIEMAP and SEEK_HOLE/SEEK_DATA
> implementations into their own static functions.  Then
> raw_co_get_block_status() becomes simpler and doesn't need ifdefs:
>
> ret = try_fiemap(...);
> if (ret < 0) {
>      ret = try_seekhole(...);
> }
> if (ret < 0) {
>      ...report every block allocated by default....
> }
>
> In other words, let normal C control flow describe the relationships
> between these code paths.  Use ifdef only to nop out try_fiemap() and
> try_seekhole().
>
> What do you think?

Great idea! I thought about using normal if's or something, but didn't 
think of this.

Max