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

[Max Reitz]

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 <address@hidden>
> > ---
> > 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