Re: [Qemu-devel] [PATCH] block: Make op blocker recursive

[Fam Zheng]

On Fri, 06/20 09:30, Eric Blake wrote:
> On 06/19/2014 11:01 PM, Fam Zheng wrote:
> > On Thu, 06/19 22:20, Benoît Canet wrote:
> >> The Thursday 19 Jun 2014 à 14:13:20 (-0600), Eric Blake wrote :
> >>> On 06/19/2014 02:01 PM, Benoît Canet wrote:
> >>>> As the code will start to operate on arbitratry nodes we need the op 
> >>>> blocker
> >>>
> >>> s/arbitratry/arbitrary/
> >>>
> >>>> to recursively block or unblock whole BDS subtrees.
> > 
> > I don't get the reason, can you elaborate?
> 
> Consider what happens if I have:
> 
> base <- snap1 <- active
> 
> then I start a fleecing NBD server on the state as it was at snap1:
> 
> base <- snap1 <- active
>               \- fleecing
> 
> then I do a blockpull into active:
> 
> base <- snap1 <- fleecing
> active
> 
> at this point, base and snap1 are no longer tied to active, but they
> STILL must be protected from operations that would modify their contents
> in a way that would break the fleecing operation.  The solution we are
> looking at is making BDS blockers recursive to every element of the
> chain, not just the top-level device.

This would already have been protected by backing blocker of fleecing target.

> 
> Another example: consider:
> 
> base <- snap1 <- active
> 
> then someone uses Jeff's proposed new change-backing-file QMP command to
> rewrite the snap1 metadata to point to base via a relative name instead
> of an absolute name.  It shouldn't matter whether active is blocked, but
> only whether snap1 is blocked.  But to know if snap1 is blocked, we have
> to propagate the blockers of active down recursively to its backing files.

Why do we need to block changging of metadata? I think this operation is safe
in most cases.

Correct me if I'm missing anything, but even if snap1 _is_ blocked, it would be
because snap1 is serving as backing of active. In this case, the actual blocker
should be active->backing_blocker.

> 
> >> What would be a cleaner solution ?
> > 
> > What is the question to solve?
> 
> I think Jeff's idea is on target - rather than blocking by operation, we
> should instead be blocking on access patterns (various operations
> trigger several access patterns):
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg04752.html
> 
> Jeff's initial list included:
> 
> > So if I think of operations that are done on block devices from a
> > block job, and chuck them into categories, I think we have:
> > 
> > 1) Read of guest-visible data
> > 2) Write of guest-visible data
> > 3) Read of host-visible data (e.g. image file metadata)
> > 4) Write of host-visible data (e.g. image file metadata, such as
> > the backing-file)
> > 5) Block chain manipulations (e.g. movement of a BDS, change to r/w
> > instead of r/o, etc..)
> > 6) I/O attribute changes (e.g. throttling, etc..)

Most operations looks safe to me, given the way how IOThreads and coroutine
work now. It's only the chain manpulations in long running block jobs that are
exclusive, and by nature it should be checked per chain.  Can we set some op
blockers on the bottom BDS and check it each time, to prevent user from
starting a second chain manipulator?

Fam