On Tue, Jun 24, 2014 at 10:48:52AM +0800, Fam Zheng wrote: > On Mon, 06/23 21:08, Stefan Hajnoczi wrote: > > On Thu, Jun 19, 2014 at 12:26:00PM -0400, Jeff Cody wrote: > > > On Thu, Jun 19, 2014 at 05:17:16PM +0800, Stefan Hajnoczi wrote: > > > > On Tue, Jun 17, 2014 at 05:53:48PM -0400, Jeff Cody wrote: > > > > Let's discuss this topic in a sub-thread and figure out what to do for > > > > QEMU 2.1. This is an important issue to solve before the release > > > > because we can't change QMP command semantics easily later. > > > > > > > > My questions are: > > > > a. How do we fix resize, snapshot-sync, etc? It seems like we need to > > > > propagate child op blockers. > > > > > > > > b. Is it a good idea to perform op blocker checks on the root node? > > > > It's inconsistent with resize, snapshot-sync, etc. Permissions in > > > > BDS graphs with multiple root nodes (e.g. guest device and NBD > > > > run-time server) will be different depending on which root you > > > > specify. > > > > > > I don't think (b) is the ultimate solution. It is used as a stop-gap > > > because op blockers in the current implementation is essentially > > > analogous to the in-use flag. But is it good enough for 2.1? If > > > *everything* checks the topmost node in 2.1, then I think we are OK in > > > all cases except where images files share a common BDS. > > > > Checking op blockers on the root node as a stop-gap is a good idea. > > Let's apply it across all commands (e.g. snapshot-sync, resize). > > > > Fam pointed out that this approach is vulnerable to blockdev-add, where > > blockers could be set/checked on an incomplete BDS graph (since you can > > add new nodes on top). Do we need to move the blockers up the graph if > > a new root node is inserted? > > My concern is if we allow adding new root on top, it's not easy to know the > real root then. > > To give an example: > > If we have > > [base id=""] <- [active id="drive0" blockers=...] > > When user does > > (QMP) block-commit device="drive0" ... > > We should check drive0, which is OK. > > Then, assume user adds a new root on top, we would take care of moving the > blockers: > > [base id=""] <- [active id="drive0"] <- [active id="drive1" blockers=] > > At this point, what if user does something on drive0 again? > > (QMP) block-commit device="drive0" ... > > The right thing to do is to check blockers on "drive1", since it's the real > root now. But how do we know? Do we need to add a back reference pointer > ->overlap_hd in BDS, or do we maintain a look up table, or do we search all > BDS > graphs to figure out? > > None is easier than if we put the blockers in the bottom BDS, in the first > place: > > [base id="" blockers=...] <- [active id="drive0"] > ^^^^^^^^^^^^ >
I think you are right. If we place the blocker at the bottom-most BDS, then that would be a more restrictive blocker. This may end up being more restrictive than needed, but more importantly it should make everything safe. Also, it is an easy change for 2.1 - just call bdrv_find_base(bs), and set/check/clear blockers on the returned BDS. > Even if user adds a new root, we don't need to worry about moving blockers, > because the bottom is not changed. > > [base id="" blockers=...] <- [active id="drive0"] <- [active id="drive1"] > > Checking the blockers are easy, either for drive0 or drive1: just follow the > backing chain until getting to the end. > > Fam