From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45734) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBV2f-0004Fj-2G for qemu-devel@nongnu.org; Tue, 22 Dec 2015 17:04:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBV2d-0005cx-Du for qemu-devel@nongnu.org; Tue, 22 Dec 2015 17:04:24 -0500 References: <1450802786-20893-1-git-send-email-kwolf@redhat.com> <1450802786-20893-11-git-send-email-kwolf@redhat.com> From: Eric Blake Message-ID: <5679C8DC.6000400@redhat.com> Date: Tue, 22 Dec 2015 15:04:12 -0700 MIME-Version: 1.0 In-Reply-To: <1450802786-20893-11-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WL4FuRv8cRs2DnkwcBt82197w4rDEGPEp" Subject: Re: [Qemu-devel] [PATCH 10/10] qcow2: Add image locking List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, mreitz@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --WL4FuRv8cRs2DnkwcBt82197w4rDEGPEp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 12/22/2015 09:46 AM, Kevin Wolf wrote: > People have been keeping destroying their qcow2 images by using Any of these sound better: People have kept on destroying People have been destroying People keep on destroying > 'qemu-img snapshot' on images that were in use by a VM. This patch adds= > some image locking that protects them against this mistake. >=20 > In order to achieve this, a new compatible header flag is introduced > that tells that the image is currently in use. It is (almost) always se= t > when qcow2 considers the image to be active and in a read-write mode. > During live migration, the source considers the image active until the > VM stops on migration completion. The destination considers it active a= s > soon as it starts running the VM. >=20 > In cases where qemu wasn't shut down cleanly, images may incorrectly > refuse to open. An option override-lock=3Don is provided to force openi= ng > the image (this is the option that qemu-img uses for 'force-unlock' and= > 'check --force'). >=20 > A few test cases have to be adjusted, either to update error messages, > to use read-only mode to avoid the check, or to override the lock where= > necessary. >=20 > Signed-off-by: Kevin Wolf > --- > diff --git a/block/qcow2.c b/block/qcow2.c > index 544c124..c07a078 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -307,6 +307,8 @@ int qcow2_mark_corrupt(BlockDriverState *bs) > BDRVQcow2State *s =3D bs->opaque; > =20 > s->incompatible_features |=3D QCOW2_INCOMPAT_CORRUPT; > + s->compatible_features &=3D ~QCOW2_COMPAT_IN_USE; > + So the moment we detect something is wrong, we (attempt to) write the corrupt bit but promise to do no further writes, so it makes sense that we can claim we are no longer using the image. > =20 > @@ -472,6 +474,11 @@ static QemuOptsList qcow2_runtime_opts =3D { > .type =3D QEMU_OPT_NUMBER, > .help =3D "Clean unused cache entries after this time (in = seconds)", > }, > + { > + .name =3D BDRV_OPT_OVERRIDE_LOCK, > + .type =3D QEMU_OPT_BOOL, > + .help =3D "Open the image read-write even if it is locked"= , > + }, Missing counterpart documentation in qapi/block-core.json BlockdevOptionsQcow2. > + /* Protect against opening the image r/w twice at the same time */= > + if (!bs->read_only && (s->compatible_features & QCOW2_COMPAT_IN_US= E)) { > + /* Shared storage is expected during migration */ > + bool migrating =3D (flags & BDRV_O_INCOMING); > + > + if (!migrating && !s->override_lock) { > + error_set(errp, ERROR_CLASS_IMAGE_FILE_LOCKED, > + "Image is already in use"); > + error_append_hint(errp, "This check can be disabled " > + "with override-lock=3Don. Caution: Openi= ng an " > + "image twice can cause corruption!"); Here's where I wondered in 6/10 if it is worth providing additional information about the current lock owner; and that information would come from [1]... > @@ -1164,6 +1193,17 @@ static int qcow2_open(BlockDriverState *bs, QDic= t *options, int flags, > } > } > =20 > + /* Set advisory lock in the header (do this as the final step so t= hat > + * failure doesn't leave a locked image around) */ > + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->qcow_versio= n >=3D 3) { > + s->compatible_features |=3D QCOW2_COMPAT_IN_USE; > + ret =3D qcow2_update_header(bs); This says that we set the advisory bit even when override-lock; the only purpose of override lock is to allow us to write to the image even if the bit was already set. I suppose the other choice would be that override-lock on means that we don't bother to set the bit at all, leaving us with the qemu 2.5 behavior of not claiming the lock and making it easier to stomp on the image - perhaps useful for regression testing, but probably not as safe as a default. So I can agree with how you implemented the override. > @@ -1272,12 +1321,32 @@ fail: > =20 > static void qcow2_reopen_commit(BDRVReopenState *state) > { > + /* We can't fail the commit, so if the header update fails, we may= end up > + * not protecting the image even though it is writable now. This i= s okay, > + * the lock is a best-effort service to protect the user from shoo= ting > + * themselves into the foot. */ s/into/in/ > + if (state->bs->read_only && (state->flags & BDRV_O_RDWR)) { > + BDRVQcow2State *s =3D state->bs->opaque; > + s->compatible_features |=3D QCOW2_COMPAT_IN_USE; > + (void) qcow2_update_header(state->bs); > + } > + > qcow2_update_options_commit(state->bs, state->opaque); > g_free(state->opaque); > } > =20 > static void qcow2_reopen_abort(BDRVReopenState *state) > { > + /* We can't fail the abort, so if the header update fails, we may = end up > + * not protecting the image any more. This is okay, the lock is a > + * best-effort service to protect the user from shooting themselve= s into s/into/in/ > @@ -1708,6 +1777,16 @@ static int qcow2_inactivate(BlockDriverState *bs= ) > qcow2_mark_clean(bs); > } > =20 > + if (!bs->read_only) { > + s->flags |=3D BDRV_O_INCOMING; > + s->compatible_features &=3D ~QCOW2_COMPAT_IN_USE; > + ret =3D qcow2_update_header(bs); > + if (ret < 0) { > + result =3D ret; > + error_report("Could not update qcow2 header: %s", strerror= (-ret)); > + } > + } > + > return result; > } > =20 > +++ b/docs/specs/qcow2.txt > @@ -96,7 +96,12 @@ in the description of a field. > marking the image file dirty and postp= oning > refcount metadata updates. > =20 > - Bits 1-63: Reserved (set to 0) > + Bit 1: Locking bit. If this bit is set, then = the > + image is supposedly in use by some pro= cess and > + shouldn't be opened read-write by anot= her > + process. =2E..[1] I'm wondering if we should add a new optional extension header here, which records the hostname+pid (and maybe also the argv[0]) of the process that set this bit. If this bit is clear, the extension header can be ignored/deleted as useless (or more likely, rewritten the moment we open the file read-write because we'll be setting the bit again); if this bit is set but the extension header is not present, we have no further information to report beyond "file is locked". But if this bit is set and the extension header is present, then we can attempt to tell the user more details about who last claimed to be writing to the file (which may help the user decide if it really still is in use, or if the lock is left over in error due to an abrupt exit). That also implies that the act of setting this bit should also default to adding the new extension header, populated with useful information. Overall, I like the direction this series is headed in! --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --WL4FuRv8cRs2DnkwcBt82197w4rDEGPEp Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWecjcAAoJEKeha0olJ0NqOWEH/3sydt3FsWHkNPvhnA3FfSF1 OVSHOx1bjVpkFvqqupZm0gOiS6lNSkTp6/25YCTvhy4EAdC4XzeT7J4SwbVDB0Nk 6uVMv5/M0LEY7jO/REORi65smzrX6AmGKvSrYri6sZphrL3D09s0pg2CoUMhJ+rP y4LqTwyJdW9WNltGMfqieyV+RJeyJgMEDx6zkbDiuOj6G9I4h0f/WMSY/BD/XTRH Eqp15KfIYw+wqx9GP8qSIF2U4KYf+i3LIDMRfDDcDq/hN6hi/J1v3Z2Ovn9E2pmw 3HxhvqR7+1QzVGT8FmWc48OJO7EwvH9zs2X10MG1lY8Y8axfk8BfpBQHwHgVSqs= =8Vj4 -----END PGP SIGNATURE----- --WL4FuRv8cRs2DnkwcBt82197w4rDEGPEp--