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 <mre...@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
