From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33993) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WyH3W-0002cA-L2 for qemu-devel@nongnu.org; Sat, 21 Jun 2014 04:53:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WyH3R-0006pB-Ky for qemu-devel@nongnu.org; Sat, 21 Jun 2014 04:53:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13942) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WyH3R-0006oM-D4 for qemu-devel@nongnu.org; Sat, 21 Jun 2014 04:53:45 -0400 Date: Sat, 21 Jun 2014 16:53:58 +0800 From: Fam Zheng Message-ID: <20140621085358.GA11607@T430.redhat.com> References: <1403208081-18247-1-git-send-email-benoit.canet@irqsave.net> <1403208081-18247-2-git-send-email-benoit.canet@irqsave.net> <53A34460.8010302@redhat.com> <20140619202043.GA18306@irqsave.net> <20140620050106.GB15938@T430.redhat.com> <53A453AF.3@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <53A453AF.3@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block: Make op blocker recursive List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Fri, 06/20 09:30, Eric Blake wrote: > On 06/19/2014 11:01 PM, Fam Zheng wrote: > > On Thu, 06/19 22:20, Beno=EEt Canet wrote: > >> The Thursday 19 Jun 2014 =E0 14:13:20 (-0600), Eric Blake wrote : > >>> On 06/19/2014 02:01 PM, Beno=EEt Canet wrote: > >>>> As the code will start to operate on arbitratry nodes we need the = op blocker > >>> > >>> s/arbitratry/arbitrary/ > >>> > >>>> to recursively block or unblock whole BDS subtrees. > >=20 > > I don't get the reason, can you elaborate? >=20 > Consider what happens if I have: >=20 > base <- snap1 <- active >=20 > then I start a fleecing NBD server on the state as it was at snap1: >=20 > base <- snap1 <- active > \- fleecing >=20 > then I do a blockpull into active: >=20 > base <- snap1 <- fleecing > active >=20 > at this point, base and snap1 are no longer tied to active, but they > STILL must be protected from operations that would modify their content= s > in a way that would break the fleecing operation. The solution we are > looking at is making BDS blockers recursive to every element of the > chain, not just the top-level device. This would already have been protected by backing blocker of fleecing tar= get. >=20 > Another example: consider: >=20 > base <- snap1 <- active >=20 > then someone uses Jeff's proposed new change-backing-file QMP command t= o > rewrite the snap1 metadata to point to base via a relative name instead > of an absolute name. It shouldn't matter whether active is blocked, bu= t > only whether snap1 is blocked. But to know if snap1 is blocked, we hav= e > to propagate the blockers of active down recursively to its backing fil= es. Why do we need to block changging of metadata? I think this operation is = safe in most cases. Correct me if I'm missing anything, but even if snap1 _is_ blocked, it wo= uld be because snap1 is serving as backing of active. In this case, the actual b= locker should be active->backing_blocker. >=20 > >> What would be a cleaner solution ? > >=20 > > What is the question to solve? >=20 > I think Jeff's idea is on target - rather than blocking by operation, w= e > should instead be blocking on access patterns (various operations > trigger several access patterns): > https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg04752.html >=20 > Jeff's initial list included: >=20 > > So if I think of operations that are done on block devices from a > > block job, and chuck them into categories, I think we have: > >=20 > > 1) Read of guest-visible data > > 2) Write of guest-visible data > > 3) Read of host-visible data (e.g. image file metadata) > > 4) Write of host-visible data (e.g. image file metadata, such as > > the backing-file) > > 5) Block chain manipulations (e.g. movement of a BDS, change to r/w > > instead of r/o, etc..) > > 6) I/O attribute changes (e.g. throttling, etc..) Most operations looks safe to me, given the way how IOThreads and corouti= ne work now. It's only the chain manpulations in long running block jobs tha= t are exclusive, and by nature it should be checked per chain. Can we set some= op blockers on the bottom BDS and check it each time, to prevent user from starting a second chain manipulator? Fam