Date: Fri, 13 Nov 2020 21:14:45 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r367631 - in head/sys: kern sys Message-ID: <X67bJaC1u33kcPEf@kib.kiev.ua> In-Reply-To: <CAGudoHE8K-gOp9X9kH9o7u2BOCk1NPPvb41i4tpHegx6=zKRJQ@mail.gmail.com> References: <202011130931.0AD9VwBL082843@repo.freebsd.org> <CAGudoHE8K-gOp9X9kH9o7u2BOCk1NPPvb41i4tpHegx6=zKRJQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Nov 13, 2020 at 07:30:47PM +0100, Mateusz Guzik wrote:
> On 11/13/20, Konstantin Belousov <kib@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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?X67bJaC1u33kcPEf>