From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35190) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqeN2-0002RZ-7M for qemu-devel@nongnu.org; Tue, 18 Nov 2014 03:42:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqeMw-0000QP-2D for qemu-devel@nongnu.org; Tue, 18 Nov 2014 03:42:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54256) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqeMv-0000Q5-RC for qemu-devel@nongnu.org; Tue, 18 Nov 2014 03:42:38 -0500 Message-ID: <546B0675.2090007@redhat.com> Date: Tue, 18 Nov 2014 09:42:29 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416219514-22530-1-git-send-email-armbru@redhat.com> <1416219514-22530-4-git-send-email-armbru@redhat.com> <546A25A2.3030801@redhat.com> In-Reply-To: <546A25A2.3030801@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 for-2.2 3/3] raw-posix: The SEEK_HOLE code is flawed, rewrite it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, famz@redhat.com, tony@bakeyournoodle.com, stefanha@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 >> --- >> 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