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

[Stefan Hajnoczi <stefanha@redhat.com>]

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?

Stefan