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

[Mateusz Guzik]

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.

-- 
Mateusz Guzik <mjguzik gmail.com>