From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4zZH-0004lL-SM for qemu-devel@nongnu.org; Fri, 04 Dec 2015 18:15:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a4zZG-00087e-Mz for qemu-devel@nongnu.org; Fri, 04 Dec 2015 18:15:11 -0500 References: <1447126069-6185-1-git-send-email-mreitz@redhat.com> <1447126069-6185-8-git-send-email-mreitz@redhat.com> <20151201160105.GF6527@noname.str.redhat.com> From: Max Reitz Message-ID: <56621E74.4020505@redhat.com> Date: Sat, 5 Dec 2015 00:15:00 +0100 MIME-Version: 1.0 In-Reply-To: <20151201160105.GF6527@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="F17Suic94CuUROXukdw9HHf2StmXUpi5W" Subject: Re: [Qemu-devel] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --F17Suic94CuUROXukdw9HHf2StmXUpi5W Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 01.12.2015 17:01, Kevin Wolf wrote: > Am 10.11.2015 um 04:27 hat Max Reitz geschrieben: >> Move bdrv_drain_all(), bdrv_commit_all(), bdrv_flush_all() and >> bdrv_invalidate_cache_all() to BB. >> >> The only operation left is bdrv_close_all(), which cannot be moved to >> the BB because it should not only close all BBs, but also all >> monitor-owned BDSs. >> >> Signed-off-by: Max Reitz >=20 > bdrv_commit_all() and bdrv_flush_all() are relatively obvious. They are= > meant to commit/flush changes made by a guest, so moving to the BB leve= l > seems right. OK, I wanted to just follow this comment, but since monitor_bdrv_states is local to blockdev.c (and I consider that correct) that would mean either making it global (which I wouldn't like) or keeping bdrv_states (which I wouldn't like either, not least because dropping bdrv_states allows us to drop bdrv_new_root(), which may or may not be crucial to the follow-up series to this one). So, I'll be trying to defend what this patch does for now. > I'm not so sure about bdrv_invalidate_cache_all(). Even if a BB isn't > attached to a BDS, we still need to invalidate the caches of any images= > that have been opened before migration has completed, including those > images that are only owned by the monitor. I consider BB-less BDSs to be in a temporary state between blockdev-add and binding them to a device, or between unbinding them from a device and blockdev-del. So I don't even know if we should allow them to be migrated. Anyway, in my opinion, a BB-less BDS should be totally static. Invalidating its cache shouldn't do anything. Instead, its cache should be invalidated when it is detached from a BB (just like this patch adds a bdrv_drain() call to blk_remove_bs()). > Similarly I'm concerned about bdrv_drain_all(). Anything outside the > block layer should certainly call blk_drain_all() because it can only b= e > interested in those images that it can see. The calls in block.c, > however, look like they should consider BDSes without a BB as well. (Th= e > monitor, which can hold direct BDS references, may be another valid use= r > of a bdrv_drain_all that also covers BDSes without a BB.) Similarly, in my opinion, bdrv_drain() shouldn't do anything on a BB-less BDS. That is why this patch adds a bdrv_drain() call to blk_remove_bs() (a bdrv_invalidate_cache() call is missing yet, though). But I just remembered that block jobs don't use BBs... Did I mention before how wrong I think that is? Now I'm torn between trying to make block jobs use BBs once more (because I really think BB-less BDSs should not have any active I/O) and doing some ugly things with bdrv_states and/or bdrv_monitor_states. I'll have to think about it. > And block.c calling blk_*() functions smells a bit fishy anyway. I don't find it any more fishy than single-BDS functions calling bdrv_*_all() functions. And that is, not fishy at all. If some function wants to drain all I/O and we define I/O to always originate from a BB, then I find the only reasonable approach to call blk_drain_all(). Max --F17Suic94CuUROXukdw9HHf2StmXUpi5W 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 iQEcBAEBCAAGBQJWYh50AAoJEDuxQgLoOKytrdwIAJN2XeyqaG7mCCwfMMDWAAxS Q89Vavzs15T4/rDxWV17Nrin/UJ6IIDdYk0opf+AMSiDrJpf8VkUUAfmncsXBpoY 6Z2HfK5vsIhUmaRuErWeaF//VEtjf3tsW17PtNEwDIR3p305A6jp8XZhaSjJKKei PMbsNUKCS58Bo4UPtaQdrTyftfdPh/U47HMvX+4lCFmuvd6nSHSdbBd7pXP8hSsw 1zk+hSXGndTvCOvZMukAUu3B9UAvcbi2vcZUHD/n2Nj2uuUFSb1z3cAQXVj+8Vcd o7ZzOQb2UcV8H4bKFcNnlfkiv6fctsCJ0+EdpC4RYPKCivoG6q1q21y2eLJHzqA= =9YDE -----END PGP SIGNATURE----- --F17Suic94CuUROXukdw9HHf2StmXUpi5W--