Re: [Qemu-devel] [PATCH v3 for-2.2 3/3] raw-posix: The SEEK_HOLE code is flawed, rewrite it

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]

On 11/17/2014 03:18 AM, Markus Armbruster wrote:
> On systems where SEEK_HOLE in a trailing hole seeks to EOF (Solaris,
> but not Linux), try_seek_hole() reports trailing data instead.

Maybe worth a comment that this is not fatal, but also not optimal.

> 
> Additionally, unlikely lseek() failures are treated badly:
> 
> * When SEEK_HOLE fails, try_seek_hole() reports trailing data.  For
>   -ENXIO, there's in fact a trailing hole.  Can happen only when
>   something truncated the file since we opened it.
> 
> * When SEEK_HOLE succeeds, SEEK_DATA fails, and SEEK_END succeeds,
>   then try_seek_hole() reports a trailing hole.  This is okay only
>   when SEEK_DATA failed with -ENXIO (which means the non-trailing hole
>   found by SEEK_HOLE has since become trailing somehow).  For other
>   failures (unlikely), it's wrong.
> 
> * When SEEK_HOLE succeeds, SEEK_DATA fails, SEEK_END fails (unlikely),
>   then try_seek_hole() reports bogus data [-1,start), which its caller
>   raw_co_get_block_status() turns into zero sectors of data.  Could
>   theoretically lead to infinite loops in code that attempts to scan
>   data vs. hole forward.
> 
> Rewrite from scratch, with very careful comments.

Thanks for the careful commit message as well as the careful comments :)

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/raw-posix.c | 111 +++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 85 insertions(+), 26 deletions(-)
> 

> @@ -1542,25 +1600,26 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>          nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
>      }
>  

> +    } else if (data == start) {
>          /* On a data extent, compute sectors to the end of the extent.  */
>          *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE);

I think we are safe for now that no file system supports holes smaller
than 512 bytes, so (hole - start) should always be a non-zero multiple
of sectors.  Similarly for the hole case of (data - start).  Maybe it's
worth assert(*pnum > 0) to ensure that we never hit a situation where we
go into an infinite loop where we aren't progressing because pnum is
never advancing to the next sector?  But that would be okay as a
separate patch, and I don't want to delay getting _this_ patch into 2.2.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]