Re: [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2014-08-28 at 21:27 +0100, Wei Liu wrote:
> On Thu, Aug 28, 2014 at 08:31:39PM +0100, Ian Campbell wrote:
> > On Thu, 2014-08-28 at 20:04 +0100, Wei Liu wrote:
> > > Application locking is one thing, but we still need to serialise libxl
> > > access to those files, don't we? Any access to userdata store via libxl
> > > API should be serialised. The reason is stated in previous patch "libxl:
> > > properly lock user data store".
> > 
> > I may be confused here, so please correct me if I'm wrong...
> > 
> > Any individual userdata store/retrieve is atomic insofar as afterwards
> > there will be a consistent copy of the thing there, i.e. if there is a
> > race you will get one or the other copy of the data, but never a
> > mixture. Locking within the store/retrieve function neither helps nor
> > hinders  this (since to the loser of the race the result is
> > indistinguishable from someone coming along 1s later and updating).
> > 
> > The locking is there to protect against read-modify-write cycles (e.g.
> > updating the config), which necessarily implies taking the lock before
> > the read and releasing it after the write -- i.e. at the application
> > layer (the libxl-lock being a kind of special in-libxl "application"
> > layer). Without the lock two entities racing in the r-m-w cycle can
> > result in updates being lost.
> 
> You're right on the r-m-w analysis. But the lock does more than that. In
> this specific API family that manipulates userdata store, it also
> ensures files won't disappear until other thread that holds the lock
> finishes its job. Userdata vanishes under our feet is one abnormal state
> we would like to avoid, userdata reappears after we delete it is another
> abnormal state we would like to avoid.  If we don't hold this lock for
> this unlink API, we now have the chance to come into those two abnormal
> states. Does this make sense?

Yes, it makes sense to lock removal against any r-m-w in another thread.

But I don't think it follows the libxl_userdata_unlink should take any
particular lock, including the libxl lock, that would be the
responsibility of the caller.

For libxl-json userdata manipulation, you don't have this issue because
you aren't using unlink, I don't think. If libxl is using unlink
internally then it should arrange to hold the lock while calling unlink,
just like for r-m-w.

For xl cfg userdata the lock which you are making libxl_userdata_unlink
touch is not used by xl when doing r-m-w operations of the data (if it
even does such) so it doesn't protect you against anything.

If xl is doing r-m-w then it needs its own lock, and it should also
arrange to hold that lock over the unlink. This shouldn't be the
libxl-lock, it should be some lock which belongs to xl and it needs to
be taken at the application level.


> OK, TBH I don't quite like this API either. If we don't provide a way
> for xl to delete xl cfg userdata, what should we do with xl cfg? What do
> you suggest to achieve the said behavior of "xl config-update"?

I hope the above makes the clearer, but either xl needs a lock of its
own or it doesn't, but in no case is this libxl's business...

Ian.