From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzPd8-0006It-MX for qemu-devel@nongnu.org; Wed, 26 Oct 2016 10:56:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzPd7-0000kN-LD for qemu-devel@nongnu.org; Wed, 26 Oct 2016 10:56:38 -0400 References: <1475237406-26917-1-git-send-email-famz@redhat.com> <1475237406-26917-6-git-send-email-famz@redhat.com> <20161025063147.GE5427@lemon> <20161025134303.GB15298@lemon> From: Max Reitz Message-ID: <8ff55c60-1e0d-3ec2-ad87-62d32c23937e@redhat.com> Date: Wed, 26 Oct 2016 16:56:26 +0200 MIME-Version: 1.0 In-Reply-To: <20161025134303.GB15298@lemon> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xpVOaKVUAfLouKe3LDDLMCE78fJUvC4hH" Subject: Re: [Qemu-devel] [PATCH v8 05/36] raw-posix: Add image locking support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: qemu-devel@nongnu.org, berrange@redhat.com, John Snow , qemu-block@nongnu.org, Kevin Wolf , rjones@redhat.com, Jeff Cody , Markus Armbruster , stefanha@redhat.com, den@openvz.org, pbonzini@redhat.com, eblake@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --xpVOaKVUAfLouKe3LDDLMCE78fJUvC4hH From: Max Reitz To: Fam Zheng Cc: qemu-devel@nongnu.org, berrange@redhat.com, John Snow , qemu-block@nongnu.org, Kevin Wolf , rjones@redhat.com, Jeff Cody , Markus Armbruster , stefanha@redhat.com, den@openvz.org, pbonzini@redhat.com, eblake@redhat.com Message-ID: <8ff55c60-1e0d-3ec2-ad87-62d32c23937e@redhat.com> Subject: Re: [PATCH v8 05/36] raw-posix: Add image locking support References: <1475237406-26917-1-git-send-email-famz@redhat.com> <1475237406-26917-6-git-send-email-famz@redhat.com> <20161025063147.GE5427@lemon> <20161025134303.GB15298@lemon> In-Reply-To: <20161025134303.GB15298@lemon> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 25.10.2016 15:43, Fam Zheng wrote: > On Tue, 10/25 15:28, Max Reitz wrote: >> On 25.10.2016 08:31, Fam Zheng wrote: >>> On Sat, 10/22 01:40, Max Reitz wrote: >>>> On 30.09.2016 14:09, Fam Zheng wrote: >> >> [...] >> >>>>> +static int >>>>> +raw_reopen_upgrade(BDRVReopenState *state, >>>>> + RawReopenOperation op, >>>>> + ImageLockMode old_lock, >>>>> + ImageLockMode new_lock, >>>>> + Error **errp) >>>>> +{ >>>>> + BDRVRawReopenState *raw_s =3D state->opaque; >>>>> + BDRVRawState *s =3D state->bs->opaque; >>>>> + int ret =3D 0, ret2; >>>>> + >>>>> + assert(old_lock =3D=3D IMAGE_LOCK_MODE_SHARED); >>>>> + assert(new_lock =3D=3D IMAGE_LOCK_MODE_EXCLUSIVE); >>>>> + switch (op) { >>>>> + case RAW_REOPEN_PREPARE: >>>>> + ret =3D raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED= ); >>> >>> [1] >>> >>>>> + if (ret) { >>>>> + error_setg_errno(errp, -ret, "Failed to lock new fd (s= hared)"); >>>>> + break; >>>>> + } >>>>> + ret =3D raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK); >>> >>> [2] >>> >>>>> + if (ret) { >>>>> + error_setg_errno(errp, -ret, "Failed to unlock old fd"= ); >>>>> + goto restore; >>>>> + } >>>>> + ret =3D raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUS= IVE); >>> >>> [3] >>> >>>>> + if (ret) { >>>>> + error_setg_errno(errp, -ret, "Failed to lock new fd (e= xclusive)"); >>>>> + goto restore; >>>>> + } >>>>> + break; >>>>> + case RAW_REOPEN_COMMIT: >>>>> + break; >>>>> + case RAW_REOPEN_ABORT: >>>>> + raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED); >>>>> + ret =3D raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED); >>> >>> [4] >>> >>>>> + if (ret) { >>>>> + error_report("Failed to restore lock on old fd"); >>>> >>>> If we get here, s->lock_fd is still locked exclusively. The followin= g is >>>> a very personal opinion, but anyway: I think it would be be better f= or >>>> it to be unlocked. If it's locked too strictly, this can really brea= k >>>> something; but if it's just not locked (while it should be locked in= >>>> shared mode), everything's going to be fine unless the user makes a >>>> mistake. I think the latter is less bad. >>> >>> There are four lock states when we land on this "abort" branch: >>> >>> a) Lock "prepare" was not run, some other error happened earlier, s= o the lock >>> aren't changed compared to before the transaction starts: raw_s-= >lock_fd is >>> unlocked, s->lock_fd is shared locked. In this case raw_lock_fd = [4] cannot >>> fail, and even if it does, s->lock_fd is in the correct state. >>> >>> b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is= intact, so >>> we are good. >>> >>> c) The raw_lock_fd [2] failed. Again, similar to above. >>> >>> d) The raw_lock_fd [3] failed. Here raw_s->lock_fd is shared locked= , and >>> s->lock_fd is unlocked. The correct "abort" sequence is downgrad= e >>> raw_s->lock_fd and "shared lock" s->lock_fd again. If the "abort= " sequence >>> failed, s->lock_fd is unlocked. >> >> OK, you're right, I somehow forgot about the cases where our prepare >> sequence was either not run at all or failed. But I was thinking about= >> the case where our preparation was successful, but some later >> preparation in the reopen transaction failed. Then, s->lock_fd should = be >> locked exclusively, no? >=20 > Oh I missed to reason about that branch. Here we go: >=20 > If our preparation succeeded, raw_s->lock_fd is exclusively locked, s->= lock_fd > is unlocked. In abort we should try to return to the old state (raw_s->= lock_fd > is _not_ exclusively locked and s->lock_fd is shared locked), hence the= two > raw_lock_fd() calls at [4]. In this case, if the second raw_lock_fd() a= t [4] > doesn't work, s->lock_fd is unlocked, and raw_s->lock_fd will be closed= anyway, > which again matches what you suggested. Oh, right, it's raw_s->lock_fd that's exclusively locked, not s->lock_fd.= =2E. See? I really can't tell the difference between raw_s and s. :-/ So you're right, but that makes me just all the more skeptical about raw_s... Max --xpVOaKVUAfLouKe3LDDLMCE78fJUvC4hH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJYEMQaEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQFsS B/9GbjdJkkOBxnsWlVJ5OUJkVi/hx6TXqrJce5e6L64t6KJofFja1sK/pcvNf7IZ YAE4TCpKwYkuQFPqgmlcSVvr5ywqDqOJImRoPn/jzLDM/2fY/fVHHKByi099Oglb FJXb67uNUcq/2UkMq/IDedD8KvelU88N76H2WsUqfvwiPfSbO0deRf+8XByNc8Cc FTQ9bb6m8fS6hLyuOdldoDM19lYt9W9tSguNAth95NXGfrGnnzJdJulxxjHuhc6N 8xgm4GpSwFMDCLPtHzJibPlUnsDUYBPDi/AMBCHk/rnP2T6gvs7OAE1W2wbvG/U5 XXdJVpyc9Es+UooaAVHswzcx =LULr -----END PGP SIGNATURE----- --xpVOaKVUAfLouKe3LDDLMCE78fJUvC4hH--