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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-17 at 17:43, Eric Blake wrote:
> 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?

That's something the callers of bdrv_get_block_status() have to take 
care of. See commit 4b25bbc4c22cf39350b75bd250d568a4d975f7c5, for example.

Max

> 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>