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 <arm...@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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
