[OpenAFS-devel] Re: pthreading the bosserver

[chas williams - CONTRACTOR]

On Thu, 8 Aug 2013 19:54:26 -0400 (EDT)
Benjamin Kaduk <kaduk@mit.edu> wrote:

> > For the restart cases ('restart -all' and 'restart -bosserver') the user
> > (or bosserver itself) should probably be prevented from creating new
> > bnodes since the bnode list shouldn't grow while the various shutdown
> > procs are being applied.
> >
> > That would be easy enough to fix with a flag/lock.
> 
> Well, I think your tempBnodes_lock is performing this function, or nearly 
> this function.  I am not convinced that the extra tempBnodes queue is 
> necessary, though; I think we can just have a flag and skip the extra 
> queue and lock.

You need it.  You shouldn't hold the bnode list lock while running
the proc passed into bnode_ApplyInstance() the proc's might need to
lock the bnode list in order to change the reference counts on the
bnodes.

However, fixing Jeffery's issue with restarts is pretty easy though.  We
just need another lock (essentially a modification/write lock) to block
changes (add and deletes but not reference count changes) to the list of
current bnodes while restart's are happening. Things like
SBOZO_RestartAll() shold hold this lock to ensure that the multiple uses
of bnode_ApplyInstance() apply to the same list of bnodes. Any racing
bnode_CreateBnode() would eventually succeed (except in the cases were
the bosserver is shutting down completely).

> > It was important to get this right so I didn't need to hold the locks
> > as often.  The original code was mostly correct but benefited greatly
> > from LWP's cooperative nature.
> 
> Interesting, that's not quite as I would have expected.  The reference 
> count is serving only to keep the bnode from being deleted, I thought, not 
> as a write barrier.  So I would not have thought the locking would add a 
> substantial overhead.  And yes, "totally bogus" was a bit overblown, it 
> was mostly okay but have a few places that were quite wrong.

It does but you need to take the bnode list lock in order to change the
reference count on the bnodes.  I considered a read/write lock strategy
that would let you just have a single bnode list and avoid the temp
queues.  But that seemed overly complicated for bosserver.  Now
considering restartAll perhaps that would be a better choice.

> I'll keep looking at the windows stuff (maybe I just need to #include 
> <winsock2.h> and it will just work?) and per-bnode locks a bit more, but 
> at the moment, my leaning is that the easiest way forward would be to take 
> your code and replace the tempBnodes queue with the global flag mentioned 
> above.  Would you be okay if I went and did that?  This would leave us 
> with a tbozo, and we could get rid of the LWP copy after a bit of time.

I have no way to test windows.  I can only compile test. You can try to
eliminate the temp list of bnodes if you like but I think there are
easier ways to solve this problem.  Yes, it does look complicated but
it really isn't -- It is a temporary list of nodes.