Re: [Qemu-block] bug in reopen arch

[Kevin Wolf]

Am 22.06.2018 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.06.2018 11:56, Kevin Wolf wrote:
> > (Berto, I'm CCing you just because this is about reopen, so you might
> > have thoughts about it. But it's not really related to what you're
> > currently working on.)
> > 
> > Am 21.06.2018 um 19:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 21.06.2018 20:17, Kevin Wolf wrote:
> > > > Am 21.06.2018 um 17:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > 21.06.2018 17:25, Kevin Wolf wrote:
> > > > > > Am 15.06.2018 um 20:42 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > > > Now, I've found the following workaround, what do you think about 
> > > > > > > something
> > > > > > > like this as a temporary fix:
> > > > > > I honestly don't understand why this workaround makes any 
> > > > > > difference.
> > > > > with this patch, commit for children will be called earlier than for 
> > > > > parent,
> > > > > so, when reopening bitmaps rw (which is done in commit) bs->file will 
> > > > > be
> > > > > already completely reopened rw, and all works.
> > > > .bdrv_reopen_commit() can't do any I/O because it must not fail.
> > > > Therefore the order in which nodes are committed should not matter.
> > > > 
> > > > Any I/O that needs to be done has to be in .bdrv_reopen_prepare() (and
> > > > possibly be kept in a temporary buffer) and .bdrv_reopen_commit() can
> > > > only apply what is already in memory.
> > > > 
> > > > I don't see the code for reopening bitmaps in master. Is this a pending
> > > > patch?
> > > it is in block.c, in bdrv_reopen_commit()
> > > 
> > > ...
> > > if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
> > >          Error *local_err = NULL;
> > >          if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) {
> > This is already an ugly hack, we shouldn't have a separate callback for
> > reopening bitmaps. :-(
> > 
> > If done properly, this code would only exist internally in qcow2 as part
> > of the .bdrv_reopen/commit implementation.
> > 
> > I'm also not convinced of the error handling. According to commit
> > cb9ff6c25, this is mostly about the IN_USE flag. The qcow2 spec says
> > "The bitmap was not saved correctly and may be inconsistent." for this
> > flag. So the fail-safe state is IN_USE being set.
> > 
> > 
> > What I think qcow2 should be doing is setting IN_USE in .prepare (for ro
> > -> rw) and clearing it in .commit (for rw -> ro).
> 
> why this in prepare and this in commit? Not both in commit or both in
> prepare?

We cannot guarantee full transactional semantics because we need to
write to the image to commit, and this may fail. So no atomicity for us.

For the case that the write fails, we have the choice between:

1. Clear IN_USE in .prepare, even though it should be set for writable
   images. Setting it in .commit fails. This is unsafe if we make the
   bitmaps writable as promised. Currently code leaves them read-only,
   but doesn't return an error from the reopen operation. The management
   tool can't know about the situation, so it will fail later.

2. Set IN_USE in .prepare, even though it can be cleared for read-only
   files. Clearing it in .commit fails. This is always safe and leaves
   the read-only image in a state as if QEMU had crashed.

I think 2. is preferable.

Kevin

> > For this to be possible, .prepare needs write access to an image that
> > was read-only before and is becoming writable; and .commit needs write
> > access to an image that was writable and is becoming read-only.
> > 
> > This is the problem to solve, and implementing a proper design that can
> > provide this will solve the bug for you, too.
> > 
> > 
> > I'm not completely sure how this can be achieved best. Allowing parents
> > to choose whether they want to access the old or the new state is
> > probably not going to work in the general case because that's
> > essentially an image opened twice.
> > 
> > Even if it feels like a hack, too, maybe we need to make file-posix
> > already switch to the "better" file descriptor in .prepare and store the
> > old one in the BDRVRawReopenState so it can be restored in .abort.
> > 
> > The "better" file descriptor is the one that allows more operation, i.e.
> > writable is better than read-only in this sense. Of course, if we have
> > two options like read-only that can prevent certain operations, it may
> > be unclear, which of the two file descriptors is the one we want.
> > 
> > And obviously, this still needs child-to-parent .prepare order. I
> > believe when I tried reversing the current order a while ago, I ran into
> > problems, so whether this is possible or how it needs to be done in
> > detail needs to be checked carefully.
> > 
> > Kevin
> 
> 
> -- 
> Best regards,
> Vladimir
>