[Qemu-devel] Op Blockers on child nodes (was Re: [PATCH v6 for 2.1 00/10] Modify block jobs to use) node-names

[Jeff Cody]

On Thu, Jun 19, 2014 at 10:49:52AM -0600, Eric Blake wrote:
> On 06/19/2014 10:26 AM, Jeff Cody wrote:

<snip> Breaking the reply into two parts, for easier discussion

> > 
> > Long term thoughts:
> > 
> > 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..)
> > 
> > Does this make sense, and did I miss any (likely so)?  It seems like
> > we should issue blockers not based on specific commands (e.g.
> > BLOCK_OP_TYPE_COMMIT), but rather based on what specific category of
> > interaction on a BDS we want to prohibit / allow.
> 
> Yes, I like that train of thought - it's not the operation that we are
> blocking, but the access category(ies) required by that operation.
> 
> > 
> > I don't think specific command blockers provide enough granularity,
> > and doesn't necessarily scale well as new commands are added. It
> > forces a new block job author to go through the specific
> > implementation of the other block job commands, and interpret what
> > operations to prohibit based on what other jobs do.  Whereas if each
> > command issues blockers based on the operation category, it takes care
> > of itself, and I just issue blockers based on my block job behavior.
> > 
> > Each command would then issue appropriate operational blockers to each
> > BDS affected by an operation.  For instance, at first blush, I think
> > block-commit would want (at the very least) to block (and check) the
> > following, in this example chain:
> > 
> > 
> >      [base] <-- [int1] <--  [int2] <-- [int3] <-- [top] <-- [overlay]
> > 
> >      becomes:
> > 
> >      [base] <-- [overlay]
> > 
> > 
> > Blocked operations per image:
> > 
> > * 'base' image
> > 
> > Operation       |  Blocked
> > -----------------------------
> > GUEST READ      |   Y
> > GUEST WRITE     |   Y
> > HOST READ       |    
> > HOST WRITE      |    
> > CHAIN           |   Y
> > I/O ATTRIBUTE   |
> 
> Makes sense:
> 
> blocking guest read because we are actively modifying the contents; if
> any other operation branches off of base, they will see data that is not
> consistent with what the guest sees through overlay.
> 
> blocking guest write because it is a backing chain, and overlay still
> depends on base image for sectors that have not yet been commited..
> 
> blocking chain manipulations because we are going to do a chain
> manipulation once our command completes.
> 
> Maybe you also want to block HOST_WRITE (our chain manipulation is done
> by writing host metadata; so if someone else writes that under our feet,
> we may have races on what gets written).
>

Hmm, you are right, we should block HOST_WRITE in addition to CHAIN.
The one host-level change we make to 'base' during a commit is
changing it from r/o to r/w, and we wouldn't want anyone else to undo
that.  Other than that, the other thing we might do is a
bdrv_truncate(), but I would consider that a GUEST_WRITE.

(I think I listed r/w flag changes under chain instead of host-write;
this is a mistake, that would be a host-write example)

> > 
> > 
> > * Intermediate images between 'base' up to and including 'top':
> > 
> > Operation       |  Blocked
> > -----------------------------
> > GUEST READ      |   
> > GUEST WRITE     |   Y
> > HOST READ       |
> > HOST WRITE      |   Y
> > CHAIN           |   Y
> > I/O ATTRIBUTE   |
> > 
> 
> This needs to also block GUEST_READ - as soon as the commit starts
> modifying base, ALL intermediate images are invalid (they no longer
> represent a state of memory as was seen by the guest).  Once you start a
> commit, you MUST NOT allow a fleecing operation on any of the
> intermediate operations.
>

Yes, you are right, I should have GUEST_READ in there as well.  

We could even get more granular; only apply the GUEST_READ blocker
once a layer above that BDS pushes data down into 'base' (e.g. 'int1'
data going into 'base' doesn't invalidate 'int2', but 'int3' or higher
data would).  Up until that point, that BDS is still OK.  That level
of granularity is probably not needed, and overly complex, however.
But it would be an interesting piece of framework to have in place.

> > 
> > * The overlay of 'top', if it exists:
> > 
> > Operation       |  Blocked
> > -----------------------------
> > GUEST READ      |   
> > GUEST WRITE     |    
> > HOST READ       |
> > HOST WRITE      |   Y
> > CHAIN           |   Y
> > I/O ATTRIBUTE   |
> > 
> > 
> > 
>