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 <mre...@redhat.com> > > 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 level > 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 be > 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. (The > monitor, which can hold direct BDS references, may be another valid user > 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
signature.asc
Description: OpenPGP digital signature