From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44842) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VK8kx-0003OP-7U for qemu-devel@nongnu.org; Thu, 12 Sep 2013 11:24:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VK8kr-000416-7r for qemu-devel@nongnu.org; Thu, 12 Sep 2013 11:24:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34090) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VK8kq-000411-W0 for qemu-devel@nongnu.org; Thu, 12 Sep 2013 11:24:25 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8CFOO93030715 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 12 Sep 2013 11:24:24 -0400 Message-ID: <5231DCA6.7090004@redhat.com> Date: Thu, 12 Sep 2013 17:24:22 +0200 From: Max Reitz MIME-Version: 1.0 References: <1378106712-29856-1-git-send-email-mreitz@redhat.com> <5231D673.6050907@redhat.com> In-Reply-To: <5231D673.6050907@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 0/8] Add metadata overlap checks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 2013-09-12 16:57, Eric Blake wrote: > On 09/02/2013 01:25 AM, Max Reitz wrote: >> If a qcow2 image file becomes corrupted, any write may inadvertently >> overwrite important metadata structures such as the L1 table. This >> series adds functionality for detecting, preventing and (to some exten= t) >> repairing such collisions. >> >> v5: >> - fixed patch 6 (forgot to update the event_names array for the new >> event BLKDBG_REFTABLE_UPDATE); no other changes >> >> v4: >> - fixed handling of preallocated zero clusters in patch 4 >> - moved OFLAG_COPIED checks into a separate function (this affects >> patches 4 and 5); functionality remains unchanged >> - patches 1, 2, 3, 6, 7 and 8 remain unmodified (except for line >> numbers in block/qcow2-refcount.c) > Just now looking at this series, and I have several questions. > > It looks like Kevin applied v4 rather than v5; have we fixed that up? Seems like it =E2=80=93 I have the change (from v4 to v5) in master (see=20 block/blkdebug.c:171). > Next, what sort of overhead do these new checks add to the write case? > Is it something that would be a noticeable slowdown? I'd love to see > some benchmark numbers (hopefully, the default set of checks are in the > noise compared to the overhead of actual I/O). I'll do my best to get some benchmarking done. > Also, is there a way to tune the set of checks used at runtime, or are > we stuck with the compiled-in default? That is, can a user opt in to > more expensive tests for robustness, or opt out of default tests for > speed, via a runtime command, or is it something where they have to > recompile to choose a different QCOW2_OL_DEFAULT value? > Right now, it's obviously a compile-time constant. However, I think we=20 could also define QCOW2_OL_DEFAULT to be a runtime variable, however,=20 that variable would most probably be stored in the BDRVQcowState=20 structure. We could take advantage of that structure's instance being=20 named "s" almost universally, therefore we could define QCOW2_OL_DEFAULT=20 to be s->default_overlap_check or something like that. It would=20 obviously be kind of ugly, but it should work pretty well. On the other hand, we could replace QCOW2_OL_DEFAULT manually everywhere=20 by s->default_overlap_check; or, we change the macro to take s as a=20 parameter. Either way, I see three answers to your question: First, right now, we're basically stuck with a compile time constant. Second, if a user really wants to, he could always define the macro to=20 represent some variable. This should work pretty well already, although=20 this variable has to be defined first, of course. Third, it wouldn't be too much of a problem to write a follow-up patch=20 manually replacing every QCOW2_OL_DEFAULT occurrence by a true variable=20 (such as default_overlap_check from the current BDRVQcowState=20 structure). I'd just undefine the macro and work down every compiler=20 error. ;-) On the other hand, now, that I think about it, we could also invert the=20 current program logic: qcow2_check_metadata_overlap would then no longer=20 receive a bitmask of structures to check for overlaps, but rather a=20 bitmask of structures to ignore, since it can figure out=20 s->default_overlap_check by itself. Max