Am 02.03.2015 um 16:15 hat Max Reitz geschrieben:
On 2015-03-02 at 04:25, Kevin Wolf wrote:
Am 27.02.2015 um 17:43 hat Max Reitz geschrieben:
Currently, bdrv_close_all() force-closes all BDSs with a BlockBackend,
which can lead to data corruption (see the iotest added in the final
patch of this series) and is most certainly very ugly.
This series reworks bdrv_close_all() to instead eject the BDS trees from
all BlockBackends and then close the monitor-owned BDS trees, which are
the only BDSs without a BB. In effect, all BDSs are closed just by
getting closed automatically due to their reference count becoming 0.
The benefit over the approach taken in v1 and v2 is that in device
models we often cannot simply drop the reference to a BB because there
may be some user which we forgot about. By ejecting the BDS trees from
the BB, the BB itself becomes unusable, but in a clean way (it will
return errors when accessed, but nothing will crash). Also, it is much
simpler (no reference tracking necessary).
The only disadvantage (I can see) is that the BBs are leaked; but this
does not matter because the qemu process is about to exit anyway.
I haven't looked at the actual patches yet, but just from this
description and the diffstat: We need to make sure that the refcount
really drops to 0.
If you mean the BBs: Tried that in v2, doesn't seem to work. I had
my fun tracing around where the handles to the BBs for device models
stay and who might have access to them, but when I say "fun" I mean
that ironically.
If you mean the BDSs: See below (below below).
Yes, I mean BDSes.
That is, if there are NBD servers, block jobs, etc.
that hold an additional reference, we must make sure to stop them. It
doesn't look like this series takes care of this, does it?
Everyone using BBs is fine; for instance, NBD servers are stopped
(they register a notifier for when the BDS tree is ejected from a
BB).
Good point. Even if the NBD server were not stopped, it still would
hold a reference only to the BB and not to the BDS, so the BDS would
correctly be closed.
So block jobs are not handled, once because I still didn't get
around to make them work on BBs instead of BDSs (and I probably
never will, although they really should), and because I missed the
bdrv_ref() in block_job_create()
I guess that means manually cancelling all block jobs in bdrv_close_all()...
Even in the far future, will block jobs really always work on a BB? Even
if they modify some node in the middle of the chain?
Hm... Perhaps we could even install an atexit handler that asserts that
all BDSes are really gone in the end?
I had that for BBs in v1 and v2, it was just a function that was
called at the end of bdrv_close_all(). For that to work for BDSs,
however, we'd need a list of all BDSs and assert that it's empty. I
found that a bit too much.
The list of all relevant BDSes should be the union of bdrv_states and
bdrv_graph_states, and I think it doesn't have to be empty, but all of
its entries have to be closed (bs->drv == NULL).
It's not 100% complete because we have these stupid QMP commands that
address BDSes using filenames instead of node or BB names, but it's
probably a useful subset to check.
A simpler but uglier way would be a counter that's incremented every
time a BDS is created and decremented every time one is deleted (or
incremented once per refcount increment and drecrement once per
refcount decrement). We'd then assert that this counter is 0.
Ugh. Well, it does the job, so holding my nose while applying should
do...