atomic v_usecount and v_holdcnt

[Konstantin Belousov]

On Tue, Mar 17, 2015 at 02:44:12AM +0100, Mateusz Guzik wrote:
> On Mon, Mar 16, 2015 at 11:46:43AM +0200, Konstantin Belousov wrote:
> > On Sat, Mar 14, 2015 at 11:52:26PM +0100, Mateusz Guzik wrote:
> > > On Mon, Nov 24, 2014 at 11:52:52AM +0200, Konstantin Belousov wrote:
> > > > On Sat, Nov 22, 2014 at 10:11:47PM +0100, Mateusz Guzik wrote:
> > > > > On Sat, Nov 22, 2014 at 11:25:27AM +0200, Konstantin Belousov wrote:
> > > > > > On Sat, Nov 22, 2014 at 01:28:12AM +0100, Mateusz Guzik wrote:
> > > > > > > The idea is that we don't need an interlock as long as we don't
> > > > > > > transition either counter 1->0 or 0->1.
> > > > > > I already said that something along the lines of the patch should work.
> > > > > > In fact, you need vnode lock when hold count changes between 0 and 1,
> > > > > > and probably the same for use count.
> > > > > > 
> > > > > 
> > > > > I don't see why this would be required (not that I'm an VFS expert).
> > > > > vnode recycling seems to be protected with the interlock.
> > > > > 
> > > > > In fact I would argue that if this is really needed, current code is
> > > > > buggy.
> > > > Yes, it is already (somewhat) buggy.
> > > > 
> > > > Most need of the lock is for the case of counts coming from 1 to 0.
> > > > The reason is the handling of the active vnode list, which is used
> > > > for limiting the amount of vnode list walking in syncer.  When hold
> > > > count is decremented to 0, vnode is removed from the active list.
> > > > When use count is decremented to 0, vnode is supposedly inactivated,
> > > > and vinactive() cleans the cached pages belonging to vnode.  In other
> > > > words, VI_OWEINACT for dirty vnode is sort of bug.
> > > > 
> > > 
> > > Modified the patch to no longer have the usecount + interlock dropped +
> > > VI_OWEINACT set window.
> > > 
> > > Extended 0->1 hold count + vnode not locked window remains. I can fix
> > > that if it is really necessary by having _vhold return with interlock
> > > held if it did such transition.
> > 
> > In v_upgrade_usecount(), you call v_incr_devcount() without without interlock
> > held.  What prevents the devfs vnode from being recycled, in particular,
> > from invalidation of v_rdev pointer ?
> > 
> 
> Right, that was buggy. Fixed in the patch below.
Why non-atomicity of updates to several counters is safe ?  This at least
requires an explanation in the comment, I mean holdcnt/usecnt pair.

Assume the thread increased the v_usecount, but did not managed to
acquire dev_mtx. Another thread performs vrele() and progressed to
v_decr_devcount(). It decreases the si_usecount, which might allow yet
another thread to see the si_usecount as too low and start unwanted
action. I think that the tests for VCHR must be done at the very
start of the functions, and devfs vnodes must hold vnode interlock
unconditionally.

> 
> > I think that refcount_acquire_if_greater() KPI is excessive.  You always
> > calls acquire with val == 0, and release with val == 1.
> > 
> 
> Yea i noted in my prevoius e-mail it should be changed (see below).
> 
> I replaced them with refcount_acquire_if_not_zero and
> refcount_release_if_not_last.
I dislike the length of the names.  Can you propose something shorter ?

The type for the local variable old in both functions should be u_int.

> 
> > WRT to _refcount_release_lock, why is lock_object->lc_lock/lc_unlock KPI
> > cannot be used ? This allows to make refcount_release_lock() a function
> > instead of gcc extension macros.  Not to mention that the macro is unused.
> 
> These were supposed to be used by other code, forgot to remove it from
> the patch I sent here.
> 
> We can discuss this in another thread.
> 
> Striclty speaking we could use it here for vnode interlock, but I did
> not want to get around VI_LOCK macro (which right now is just a
> mtx_lock, but this may change).
> 
> Updated patch is below:
Do not introduce ASSERT_VI_LOCK, the name difference between
ASSERT_VI_LOCKED and ASSERT_VI_LOCK is only in the broken grammar.
I do not see anything wrong with explicit if() statements where needed,
in all four places.

In vputx(), wrap the long line (if (refcount_release() || VI_DOINGINACT)).