Re: [Qemu-devel] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3146 bytes --]

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 <mreitz@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]