From owner-svn-src-head@freebsd.org Fri Nov 13 18:30:51 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 886D72EA427; Fri, 13 Nov 2020 18:30:51 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com [IPv6:2a00:1450:4864:20::32e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CXn8q3Km8z4jYS; Fri, 13 Nov 2020 18:30:51 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm1-x32e.google.com with SMTP id p22so9366080wmg.3; Fri, 13 Nov 2020 10:30:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=JgE1FTKBZcA2LepvqEZxb2reZVhRjgBv6ZO925sFyYM=; b=rLdB7bznx1/IbclTnBL4Brq7c4bQA+SuWYWvKsBKY6H5xcuX+5WshdOeUigh6uq07H xS8m5aWpSBapRi4oEI0xgD1ltFQhHL0iJ5hjM/zasnq0Mt18c0/wLP1WiQt5sFcRsUUN 8HSgQxuRDe62LrDFLurFTjm7T3VTrriKW2dVbT+Bp5ewyAeTu/lyZLk5/J7BDTy/FiCt Lbx100Li9IHcg2tp9UhVGJA0ZFMRjm2MYgt7cJOOEsMUH6VhFbfqclZKy+WYh5SQ5FrL nRZr8PFVX1Ccu2M2mqerU9J+ThhY3nNSABweB+gYJwA9RlE50ivPf4ZDNaYFvzVJJFRH mVYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=JgE1FTKBZcA2LepvqEZxb2reZVhRjgBv6ZO925sFyYM=; b=Y2a59ALIA9xbC2ZhnChI6RF9dIvowBqQUyMYNwTxuo8YwkyV9r3LTz85efaUNDyDOv /2KHQ59xyo/qDU8CTgsfNW4LY7rsu1czmQYn5PeUlb9Dm1t0pKZRk8NoBsWVJjbWA9wN cpGsIpaxmvtlajmtbpqeoCCmZsyxK+zOUtalt809FQqqyyYJWlMbLaXDjE4b9NVeB/in 56qRI+hlmJagEsz/wq+8yPnSQiJqHMRBoLf8tMnoTaEVvn7ivLx8xZZUxGU1uHzqZslQ qIa5cLVVzXQRTGAcByCcAYUAIQmF+8SsHZwbV7uOUNnOFSl4HpJbk+KFFEA2/v00wRuE WXdw== X-Gm-Message-State: AOAM5321+Sg3XbUIiobC/e8X5OTI0TJG3Vf5wCK5DOVJ/+ykzEJQQmF3 GXLDggxDVt3Hoh/lz5xBXw2NxtYmst9B7vATaYhHsH0KgMY= X-Google-Smtp-Source: ABdhPJx9ACTGMmx2csSJN1EGEYh8VxKdoVv9m69oQyJ9j5KRahbvCXHmv518TAOIGXxs1Q7lMA4x6vfsGefs84zn1tI= X-Received: by 2002:a1c:e442:: with SMTP id b63mr3969705wmh.10.1605292248731; Fri, 13 Nov 2020 10:30:48 -0800 (PST) MIME-Version: 1.0 Received: by 2002:adf:dec7:0:0:0:0:0 with HTTP; Fri, 13 Nov 2020 10:30:47 -0800 (PST) In-Reply-To: <202011130931.0AD9VwBL082843@repo.freebsd.org> References: <202011130931.0AD9VwBL082843@repo.freebsd.org> From: Mateusz Guzik Date: Fri, 13 Nov 2020 19:30:47 +0100 Message-ID: Subject: Re: svn commit: r367631 - in head/sys: kern sys To: Konstantin Belousov Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4CXn8q3Km8z4jYS X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Nov 2020 18:30:51 -0000 On 11/13/20, Konstantin Belousov 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