From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48693) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aIeUp-0006hl-7w for qemu-devel@nongnu.org; Mon, 11 Jan 2016 10:35:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aIeUo-0006IG-3i for qemu-devel@nongnu.org; Mon, 11 Jan 2016 10:35:03 -0500 Date: Mon, 11 Jan 2016 16:34:51 +0100 From: Kevin Wolf Message-ID: <20160111153451.GE9454@noname.redhat.com> References: <1450802786-20893-1-git-send-email-kwolf@redhat.com> <1450802786-20893-8-git-send-email-kwolf@redhat.com> <5679BDEC.4040100@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="/e2eDi0V/xtL+Mc8" Content-Disposition: inline In-Reply-To: <5679BDEC.4040100@redhat.com> Subject: Re: [Qemu-devel] [PATCH 07/10] qcow2: Implement .bdrv_inactivate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com --/e2eDi0V/xtL+Mc8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 22.12.2015 um 22:17 hat Eric Blake geschrieben: > On 12/22/2015 09:46 AM, Kevin Wolf wrote: > > The callback has to ensure that closing or flushing the image afterwards > > wouldn't cause a write access to the image files. This means that just > > the caches have to be written out, which is part of the existing > > .bdrv_close implementation. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block/qcow2.c | 45 ++++++++++++++++++++++++++++----------------- > > 1 file changed, 28 insertions(+), 17 deletions(-) > >=20 >=20 > Mostly code motion, but I still ended up with questions. >=20 > > =20 > > +static int qcow2_inactivate(BlockDriverState *bs) > > +{ > > + BDRVQcow2State *s =3D bs->opaque; > > + int ret, result =3D 0; > > + > > + ret =3D qcow2_cache_flush(bs, s->l2_table_cache); > > + if (ret) { > > + result =3D ret; > > + error_report("Failed to flush the L2 table cache: %s", > > + strerror(-ret)); >=20 > I asked Markus if we want error_report_errno() - and ever since then, I > keep finding more and more uses that would benefit from it :) At least they should be easy to find if we ever do introduce it. > > + } > > + > > + ret =3D qcow2_cache_flush(bs, s->refcount_block_cache); > > + if (ret) { > > + result =3D ret; > > + error_report("Failed to flush the refcount block cache: %s", > > + strerror(-ret)); > > + } > > + >=20 > If the media fails in between these two statements,... >=20 > > + if (result =3D=3D 0) { > > + qcow2_mark_clean(bs); >=20 > ...can't qcow2_mark_clean() fail due to an EIO or other write error? Do > we care? I guess the worst is that we didn't mark the image clean after > all, which is no worse than if qemu[-img] had been SIGKILL'd at the same > point where I hypothesized that the media could fail. Exactly. We can't avoid that disks fail, but we can make sure that we do things in the right order so that the failure can't result in corruption. Marking the image clean as the last thing should be the right order to ensure this. > > + } > > + > > + return result; >=20 > If both flushes failed, you return the result set to the return value of > the second flush. Is it ever possible that the return value of the > first flush might be more useful? I can imagine that there are such cases. But if both fail, we don't have a way to tell which error code is more useful, so we can just pick any. We have an error_report() for both, at least, in case the admin needs to check what happened. > > @@ -1693,23 +1719,7 @@ static void qcow2_close(BlockDriverState *bs) > > s->l1_table =3D NULL; > > =20 > > if (!(bs->open_flags & BDRV_O_INCOMING)) { >=20 > > - if (!ret1 && !ret2) { > > - qcow2_mark_clean(bs); > > - } > > + qcow2_inactivate(bs); > > } >=20 > Then again, the lone existing caller in this addition doesn't even care > about the return value. The only other caller was your new code added > earlier in the series; in 5/10 migration_completion(), which uses the > value as a conditional but doesn't try to call strerror(-ret). >=20 > Since this is mostly code motion, any semantic changes you would want to > make based on my questions above probably belong in their own patches. > Therefore, for this patch as written, >=20 > Reviewed-by: Eric Blake Thanks. Kevin --/e2eDi0V/xtL+Mc8 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWk8ubAAoJEH8JsnLIjy/W9NwP/1yjATO6kvmABQs5CoaIB5zZ Uvk+UEGWsfvBuztyAPWoxLHsNzzbv0Ra2b/uvOXgxBkWvjc7U/SoS4niqKjAuW9f Zi8KoqAVPo2K349a04Vpsxv41hOTz3shKI5m6tS22/Nk2pysULiCQYMaxaEuYx3f yKfM/dF0ClzIcdr3z9OUxcBFttAsRlSS3shStQ+0A1gwTHmH7BFnc9Om4aUzFGhQ Ca6cVWaGWM/sr+VoaYXJH2pN5KlvprEkl80SmVb0V3jgo/LVxZs+vL3PjvcSP2Fr u5o/zCB+tOBmTrFBSccMF7/HUzZuxwYUmLxkAfuCk3tFl8pMaHxRfyDnNDixLj8I NWaxlY+JS2RCnuDxKXETO7W8q4uqEWSA4gnGZiyGjThDHBDov26GTgt0wjkWnl/C KYCDGESoFkIKP4wsTPpqnI0tRtKV4vdFGaqnrJb5b5w9l9YYK+9IoP6hZ78B+Aqr 9dF9clW1nAdOfjb97+UyqLBbkd6PC4QViFfoveRTVMIcZMwzoXc1jSuGuGiMy/Fr 0k0c4iyeh/RfsDvA7fn5VQ2fLS0+uEaM7N7yU+6qUQfiaWdD+C1pwjDN2OhoceTk tVgvO5mQk8L96Ixw5YTv4Z78KipOEhauIDz+sR68kqK0YYt+9E6NIbJsAVF9ytwF zQ4YCtv1JTQFNyImMVht =QgZk -----END PGP SIGNATURE----- --/e2eDi0V/xtL+Mc8--