From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:53517) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hDRIs-0003rG-IF for qemu-devel@nongnu.org; Mon, 08 Apr 2019 06:15:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hDRIr-000842-9d for qemu-devel@nongnu.org; Mon, 08 Apr 2019 06:15:02 -0400 Date: Mon, 8 Apr 2019 12:14:49 +0200 From: Kevin Wolf Message-ID: <20190408101449.GB11997@linux.fritz.box> References: <1554474244-553661-1-git-send-email-andrey.shinkevich@virtuozzo.com> <468cc6c9-c338-a4fe-57b1-b4df5b2b24c0@virtuozzo.com> <20190408100442.GA11997@linux.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190408100442.GA11997@linux.fritz.box> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] block/io.c: fix for the allocation failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrey Shinkevich Cc: John Snow , "qemu-devel@nongnu.org" , "qemu-block@nongnu.org" , "fam@euphon.net" , Vladimir Sementsov-Ogievskiy , "mreitz@redhat.com" , "stefanha@redhat.com" , Denis Lunev Am 08.04.2019 um 12:04 hat Kevin Wolf geschrieben: > Am 08.04.2019 um 11:44 hat Andrey Shinkevich geschrieben: > > > > > > On 06/04/2019 01:50, John Snow wrote: > > > > > > > > > On 4/5/19 10:24 AM, Andrey Shinkevich wrote: > > >> On a file system used by the customer, fallocate() returns an error > > >> if the block is not properly aligned. So, bdrv_co_pwrite_zeroes() > > >> fails. We can handle that case the same way as it is done for the > > >> unsupported cases, namely, call to bdrv_driver_pwritev() that writes > > >> zeroes to an image for the unaligned chunk of the block. > > >> > > >> Suggested-by: Denis V. Lunev > > >> Signed-off-by: Andrey Shinkevich > > >> --- > > >> block/io.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/block/io.c b/block/io.c > > >> index dfc153b..0412a51 100644 > > >> --- a/block/io.c > > >> +++ b/block/io.c > > >> @@ -1516,7 +1516,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, > > >> assert(!bs->supported_zero_flags); > > >> } > > >> > > >> - if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) { > > >> + if (ret < 0 && !(flags & BDRV_REQ_NO_FALLBACK)) { > > >> /* Fall back to bounce buffer if write zeroes is unsupported */ > > >> BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE; > > >> > > >> > > > > > > I suppose that if fallocate fails for any reason and we're allowing > > > fallback, we're either going to succeed ... or fail again very soon > > > thereafter. > > > > > > Are there any cases where it is vital to not ignore the first fallocate > > > failure? I'm a little wary of ignoring the return code from > > > bdrv_co_pwrite_zeroes, but I am assuming that if there is a "real" > > > failure here that the following bounce writes will also fail "safely." > > > > > > I'm not completely confident, but I have no tangible objections: > > > Reviewed-by: John Snow > > > > > > > Thank you for your review, John! > > > > Let me clarify the circumstances and quote the bug report: > > "Customer had Win-2012 VM with 50GB system disk which was later resized > > to 256GB without resizing the partition inside VM. > > Now, while trying to resize to 50G, the following error will appear > > 'Failed to reduce the number of L2 tables: Invalid argument' > > It was found that it is possible to shrink the disk to 128G and any size > > above that number, but size below 128G will bring the mentioned error." > > > > The fallocate() returns no error on that file system if the offset and > > the (offset + bytes) parameters of the bdrv_co_do_pwrite_zeroes() both > > are aligned to 4K. > > What is the return value you get from this file system? > > Maybe turning that into ENOTSUP in file-posix would be less invasive. > Just falling back for any error gives me the vague feeling that it could > cause problems sooner or later. Also, which file system is this? This seems to be a file system bug. fallocate() isn't documented to possibly have alignment restrictions for FALLOC_FL_ZERO_RANGE (if this is the operation you're talking about). FALLOC_FL_PUNCH_HOLE even explicitly mentions the behaviour for partial blocks, so there is no doubt that operations for partial blocks are considered valid. Operations that may impose restrictions are explicitly documented as such. So in any case, this would only be a workaround for a buggy file system. The real bug needs to be fixed there. Kevin From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4B47CC10F13 for ; Mon, 8 Apr 2019 10:16:15 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1E31C20880 for ; Mon, 8 Apr 2019 10:16:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1E31C20880 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([127.0.0.1]:50588 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hDRK2-0004Tu-E9 for qemu-devel@archiver.kernel.org; Mon, 08 Apr 2019 06:16:14 -0400 Received: from eggs.gnu.org ([209.51.188.92]:53517) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hDRIs-0003rG-IF for qemu-devel@nongnu.org; Mon, 08 Apr 2019 06:15:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hDRIr-000842-9d for qemu-devel@nongnu.org; Mon, 08 Apr 2019 06:15:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47566) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hDRIo-000828-Mj; Mon, 08 Apr 2019 06:14:58 -0400 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 151308124D; Mon, 8 Apr 2019 10:14:57 +0000 (UTC) Received: from linux.fritz.box (ovpn-117-165.ams2.redhat.com [10.36.117.165]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E04791001DEA; Mon, 8 Apr 2019 10:14:50 +0000 (UTC) Date: Mon, 8 Apr 2019 12:14:49 +0200 From: Kevin Wolf To: Andrey Shinkevich Message-ID: <20190408101449.GB11997@linux.fritz.box> References: <1554474244-553661-1-git-send-email-andrey.shinkevich@virtuozzo.com> <468cc6c9-c338-a4fe-57b1-b4df5b2b24c0@virtuozzo.com> <20190408100442.GA11997@linux.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <20190408100442.GA11997@linux.fritz.box> User-Agent: Mutt/1.11.3 (2019-02-01) X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 08 Apr 2019 10:14:57 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] block/io.c: fix for the allocation failure X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "fam@euphon.net" , Vladimir Sementsov-Ogievskiy , Denis Lunev , "qemu-block@nongnu.org" , "qemu-devel@nongnu.org" , "mreitz@redhat.com" , "stefanha@redhat.com" , John Snow Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Message-ID: <20190408101449.gSqeBHJfUXVRVvAKQ3P-xOV3t8oR561Nqsf5xN8U238@z> Am 08.04.2019 um 12:04 hat Kevin Wolf geschrieben: > Am 08.04.2019 um 11:44 hat Andrey Shinkevich geschrieben: > > > > > > On 06/04/2019 01:50, John Snow wrote: > > > > > > > > > On 4/5/19 10:24 AM, Andrey Shinkevich wrote: > > >> On a file system used by the customer, fallocate() returns an error > > >> if the block is not properly aligned. So, bdrv_co_pwrite_zeroes() > > >> fails. We can handle that case the same way as it is done for the > > >> unsupported cases, namely, call to bdrv_driver_pwritev() that writes > > >> zeroes to an image for the unaligned chunk of the block. > > >> > > >> Suggested-by: Denis V. Lunev > > >> Signed-off-by: Andrey Shinkevich > > >> --- > > >> block/io.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/block/io.c b/block/io.c > > >> index dfc153b..0412a51 100644 > > >> --- a/block/io.c > > >> +++ b/block/io.c > > >> @@ -1516,7 +1516,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, > > >> assert(!bs->supported_zero_flags); > > >> } > > >> > > >> - if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) { > > >> + if (ret < 0 && !(flags & BDRV_REQ_NO_FALLBACK)) { > > >> /* Fall back to bounce buffer if write zeroes is unsupported */ > > >> BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE; > > >> > > >> > > > > > > I suppose that if fallocate fails for any reason and we're allowing > > > fallback, we're either going to succeed ... or fail again very soon > > > thereafter. > > > > > > Are there any cases where it is vital to not ignore the first fallocate > > > failure? I'm a little wary of ignoring the return code from > > > bdrv_co_pwrite_zeroes, but I am assuming that if there is a "real" > > > failure here that the following bounce writes will also fail "safely." > > > > > > I'm not completely confident, but I have no tangible objections: > > > Reviewed-by: John Snow > > > > > > > Thank you for your review, John! > > > > Let me clarify the circumstances and quote the bug report: > > "Customer had Win-2012 VM with 50GB system disk which was later resized > > to 256GB without resizing the partition inside VM. > > Now, while trying to resize to 50G, the following error will appear > > 'Failed to reduce the number of L2 tables: Invalid argument' > > It was found that it is possible to shrink the disk to 128G and any size > > above that number, but size below 128G will bring the mentioned error." > > > > The fallocate() returns no error on that file system if the offset and > > the (offset + bytes) parameters of the bdrv_co_do_pwrite_zeroes() both > > are aligned to 4K. > > What is the return value you get from this file system? > > Maybe turning that into ENOTSUP in file-posix would be less invasive. > Just falling back for any error gives me the vague feeling that it could > cause problems sooner or later. Also, which file system is this? This seems to be a file system bug. fallocate() isn't documented to possibly have alignment restrictions for FALLOC_FL_ZERO_RANGE (if this is the operation you're talking about). FALLOC_FL_PUNCH_HOLE even explicitly mentions the behaviour for partial blocks, so there is no doubt that operations for partial blocks are considered valid. Operations that may impose restrictions are explicitly documented as such. So in any case, this would only be a workaround for a buggy file system. The real bug needs to be fixed there. Kevin