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

[Conrad Meyer]

Hi Konstantin,

On Fri, Nov 13, 2020 at 1:32 AM Konstantin Belousov <kib at freebsd.org> wrote:
>
> Author: kib
> Date: Fri Nov 13 09:31:57 2020
> New Revision: 367631
> URL: https://svnweb.freebsd.org/changeset/base/367631
>
> Log:
>   Implement vn_lock_pair().
>
> Modified: head/sys/kern/vfs_vnops.c
> ==============================================================================
> --- head/sys/kern/vfs_vnops.c   Fri Nov 13 02:05:45 2020        (r367630)
> +++ head/sys/kern/vfs_vnops.c   Fri Nov 13 09:31:57 2020        (r367631)
> @@ -3317,4 +3325,92 @@ vn_fallocate(struct file *fp, off_t offset, off_t len,
> ...
> +
> +static void
> +vn_lock_pair_pause(const char *wmesg)
> +{
> +       atomic_add_long(&vn_lock_pair_pause_cnt, 1);
> +       pause(wmesg, prng32_bounded(hz / 10));
> +}

This function is called when the try-lock of the second lock in the
pair (either order) fails.  The back-off period is up to 100ms,
expected average 50ms.  That seems really high?

Separately: prng32_bounded() may return 0, which is transparently
converted to the equivalent of 1 by pause_sbt(9).  This means a 1 tick
pause is marginally more likely than any other possible duration.  It
probably doesn't matter.

Thanks,
Conrad

> +
> +/*
> + * 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");

(Pause called here and in similar elided case for vp2 -> vp1 below.)

> +                       }
> +                       vn_lock(vp2, LK_EXCLUSIVE | LK_RETRY);
> +                       vp2_locked = true;
> +               }