Re: [Qemu-block] [Qemu-devel] [PATCH 06/10] qemu-img: Prepare for locked images

[Kevin Wolf]

Am 14.01.2016 um 14:07 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 13.01.2016 um 09:44 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <address@hidden> writes:
> >> > Yes, these exist in theory. The question is what scenarios you want to
> >> > protect against and whether improving the mechanism to cover these cases
> >> > is worth the effort.
> >> >
> >> > The answer for what I wanted to protect is a manual action on an image
> >> > that is already in use. The user isn't quick enough to manually let two
> >> > processes open the same image at the same time, so I didn't consider
> >> > that scenario relevant.
> >> >
> >> > But assuming that everyone (including the human user) follows the above
> >> > protocol (force-unlock only what was yours before the crash), at least
> >> > cases 1 and 2 don't happen anyway.
> >> 
> >> "Force-unlock only what you locked yourself" is easier to stipulate than
> >> to adhere to when the tools can't give you a hint on who did the
> >> locking.  This is particularly true when "you" is a human, with human
> >> imperfect memory.
> >> 
> >> I understand that this locking can't provide complete protection, and
> >> merely aims to catch certain common accidents.
> >> 
> >> However, to avoid a false sense of security, its limitations need to be
> >> clearly documented.  This very much includes the rule "force-unlock only
> >> what you locked yourself".  In my opinion, it should also include the
> >> raciness.
> >> 
> >> Sometimes, solving a problem is easier than documenting it.
> >
> > Maybe I'll just merge the migration fixes and shelve the rest of the
> > series until I'm bored enough to implement the "real thing" with an
> > incompatible feature flag, lock IDs with an autogenerated part and
> > another part from the user, saved host name and PID, and a qcow2 driver
> > that refuses to write anything to an image it doesn't hold the lock for
> > even in corner cases. For now, I've already used more time for this than
> > I intended (didn't expect all that live migration fun initially...).
> >
> > And after all, it's not my VMs that get corrupted.
> >
> > If you think that solving the problem "for real" is too easy to take
> > shortcuts, I'll happily create a BZ and assign it to you if you'd like
> > me to.
> 
> I'm not sure what to do with this message.  Did I tick you off?
> 
> I took the trouble to understand what you're trying to do, in particular
> the limitations.  I also explored a possible alternative that might be
> less limited.  I didn't ask you to adopt this alternative.  I didn't
> call your patch a shortcut.  I did point out that its limitations need
> to be documented, and encouraged you to weigh documentation effort
> against implementation effort.  Additionally, I wondered how this
> affects libvirt, and whether we need to cooperate more closely on
> locking.
> 
> If that's review gone too far, then I fail to see why we bother with it.

No, it's fine. I just don't like the outcome of the review and was a bit
disappointed about what that means (see below), but that doesn't mean
that you did anything wrong.

I had a few spare minutes and wanted to hack up a quick solution just to
prevent the snapshot case. But now it seems that people (not just you)
seem to expect something more complete instead. And you might be right,
if we do the incomplete thing now and something better later, we might
end up with two mechanisms in the image format. So perhaps we should do
the "real thing".

But I've used up the spare minutes I had before Christmas to hack on
locking and now I need to handle more important projects and all the
other patch series on the list first. At the same time, something like
this needs to be merged early in a release cycle, so that libvirt can
implement it in time.

Combine everything and you get that if we want the real thing, and we
want myself to do it, it might not be for 2.6.

Kevin