From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz1me-0000r5-Mr for qemu-devel@nongnu.org; Tue, 25 Oct 2016 09:28:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz1md-0007zR-Hy for qemu-devel@nongnu.org; Tue, 25 Oct 2016 09:28:52 -0400 References: <1475237406-26917-1-git-send-email-famz@redhat.com> <1475237406-26917-6-git-send-email-famz@redhat.com> <20161025063147.GE5427@lemon> From: Max Reitz Message-ID: Date: Tue, 25 Oct 2016 15:28:33 +0200 MIME-Version: 1.0 In-Reply-To: <20161025063147.GE5427@lemon> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="n2XgDJ0rPiG1RiPITKA6fWje0j4BgGHuK" 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) --n2XgDJ0rPiG1RiPITKA6fWje0j4BgGHuK 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: 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> In-Reply-To: <20161025063147.GE5427@lemon> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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);= >=20 > [1] >=20 >>> + if (ret) { >>> + error_setg_errno(errp, -ret, "Failed to lock new fd (sha= red)"); >>> + break; >>> + } >>> + ret =3D raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK); >=20 > [2] >=20 >>> + 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_EXCLUSIV= E); >=20 > [3] >=20 >>> + if (ret) { >>> + error_setg_errno(errp, -ret, "Failed to lock new fd (exc= lusive)"); >>> + 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); >=20 > [4] >=20 >>> + if (ret) { >>> + error_report("Failed to restore lock on old fd"); >> >> If we get here, s->lock_fd is still locked exclusively. The following = is >> a very personal opinion, but anyway: I think it would be be better for= >> it to be unlocked. If it's locked too strictly, this can really break >> 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. >=20 > There are four lock states when we land on this "abort" branch: >=20 > a) Lock "prepare" was not run, some other error happened earlier, so = the lock > aren't changed compared to before the transaction starts: raw_s->l= ock_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. >=20 > b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is i= ntact, so > we are good. >=20 > c) The raw_lock_fd [2] failed. Again, similar to above. >=20 > 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 downgrade > 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? [...] >>> +static int raw_reopen_handle_lock(BDRVReopenState *state, >>> + RawReopenOperation op, >>> + Error **errp) >>> +{ >>> + BDRVRawReopenState *raw_s =3D state->opaque; >> >> Please choose another name, it's hard not to confuse this with the >> BDRVRawState all the time. (e.g. raw_rs or just rs would be enough.) >=20 > Sorry I can't change the name in this patch, it will cause more inconsi= stency > because raw_reopen_* already use the name broadly. If you really insist= it's a > bad name, I can append or prepend a patch in the next version. Well, I don't insist, but it really confused me a couple of times, even after I had realized my mistake once. Max --n2XgDJ0rPiG1RiPITKA6fWje0j4BgGHuK Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJYD14BEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQKaH B/99/NJdPkVEhDAIbM+GQFZw1E2SsV9C3vWQZjl3ZgS6NFAdP5QDcdmebLdU7h2E o0KpbfNJzGHQ7iSMcfd+cM4jYuqVt5HQaRNbxLWfbHcodLb9cUF/eBjy3z587of+ DwVNtKTTwa57PqRHAXsectQcJ35nHt4j+xUGaSNQ56GEwpqIIc9SpEnXy2z6C++C aZHXLaJrKzfNStiw7TRGHSoRR0HGBr+ublShYaTruq1570WTyWscwN7nSV7VYP1V UpfTXgy7kJfk/z56P7L6FfW+Ao+JCsE5toU/Y5A3hOY4utSxIRC5VRwi3CKJojJ1 hEVN9uQoQv/lfqS7f+ML+FZ+ =81nc -----END PGP SIGNATURE----- --n2XgDJ0rPiG1RiPITKA6fWje0j4BgGHuK--