svn commit: r367631 - in head/sys: kern sys

[Konstantin Belousov]

On Fri, Nov 13, 2020 at 07:30:47PM +0100, Mateusz Guzik wrote:
> On 11/13/20, Konstantin Belousov <kib at freebsd.org> wrote:
> > +static u_long vn_lock_pair_pause_cnt;
> > +SYSCTL_ULONG(_debug, OID_AUTO, vn_lock_pair_pause, CTLFLAG_RD,
> > +    &vn_lock_pair_pause_cnt, 0,
> > +    "Count of vn_lock_pair deadlocks");
> > +
> > +static void
> > +vn_lock_pair_pause(const char *wmesg)
> > +{
> > +	atomic_add_long(&vn_lock_pair_pause_cnt, 1);
> > +	pause(wmesg, prng32_bounded(hz / 10));
> > +}
> > +
> > +/*
> > + * Lock pair of vnodes vp1, vp2, avoiding lock order reversal.
> > + * vp1_locked indicates whether vp1 is exclusively locked; if not, vp1
> > + * must be unlocked.  Same for vp2 and vp2_locked.  One of the vnodes
> > + * can be NULL.
> > + *
> > + * The function returns with both vnodes exclusively locked, and
> > + * guarantees that it does not create lock order reversal with other
> > + * threads during its execution.  Both vnodes could be unlocked
> > + * temporary (and reclaimed).
> > + */
> > +void
> > +vn_lock_pair(struct vnode *vp1, bool vp1_locked, struct vnode *vp2,
> > +    bool vp2_locked)
> > +{
> > +	int error;
> > +
> > +	if (vp1 == NULL && vp2 == NULL)
> > +		return;
> > +	if (vp1 != NULL) {
> > +		if (vp1_locked)
> > +			ASSERT_VOP_ELOCKED(vp1, "vp1");
> > +		else
> > +			ASSERT_VOP_UNLOCKED(vp1, "vp1");
> > +	} else {
> > +		vp1_locked = true;
> > +	}
> > +	if (vp2 != NULL) {
> > +		if (vp2_locked)
> > +			ASSERT_VOP_ELOCKED(vp2, "vp2");
> > +		else
> > +			ASSERT_VOP_UNLOCKED(vp2, "vp2");
> > +	} else {
> > +		vp2_locked = true;
> > +	}
> > +	if (!vp1_locked && !vp2_locked) {
> > +		vn_lock(vp1, LK_EXCLUSIVE | LK_RETRY);
> > +		vp1_locked = true;
> > +	}
> > +
> > +	for (;;) {
> > +		if (vp1_locked && vp2_locked)
> > +			break;
> > +		if (vp1_locked && vp2 != NULL) {
> > +			if (vp1 != NULL) {
> > +				error = VOP_LOCK1(vp2, LK_EXCLUSIVE | LK_NOWAIT,
> > +				    __FILE__, __LINE__);
> > +				if (error == 0)
> > +					break;
> > +				VOP_UNLOCK(vp1);
> > +				vp1_locked = false;
> > +				vn_lock_pair_pause("vlp1");
> > +			}
> > +			vn_lock(vp2, LK_EXCLUSIVE | LK_RETRY);
> > +			vp2_locked = true;
> > +		}
> > +		if (vp2_locked && vp1 != NULL) {
> > +			if (vp2 != NULL) {
> > +				error = VOP_LOCK1(vp1, LK_EXCLUSIVE | LK_NOWAIT,
> > +				    __FILE__, __LINE__);
> > +				if (error == 0)
> > +					break;
> > +				VOP_UNLOCK(vp2);
> > +				vp2_locked = false;
> > +				vn_lock_pair_pause("vlp2");
> > +			}
> > +			vn_lock(vp1, LK_EXCLUSIVE | LK_RETRY);
> > +			vp1_locked = true;
> > +		}
> > +	}
> > +	if (vp1 != NULL)
> > +		ASSERT_VOP_ELOCKED(vp1, "vp1 ret");
> > +	if (vp2 != NULL)
> > +		ASSERT_VOP_ELOCKED(vp2, "vp2 ret");
> >  }
> >
> 
> Multiple callers who get here with flipped addresses can end up
> failing against each other in an indefinite loop.
> 
> Instead, after initial trylocking fails, the routine should establish
> ordering it will follow for itself, for example by sorting by address.
> 
> Then pseudo-code would be:
> retry:
> vn_lock(lower, ...);
> if (!VOP_LOCK(higher, LK_NOWAIT ...)) {
>      vn_unlock(lower);
>      vn_lock(higher);
>      vn_unlock(higher);
>      goto retry;
> }
> 
> Note this also eliminates the pause.

I disagree.  It will conflict with normal locking order with 50% probability.
My code switches between two orders, eventually getting out of this situation.