From patchwork Fri Feb 16 15:09:33 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Ogness X-Patchwork-Id: 10224893 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6BF32603EE for ; Fri, 16 Feb 2018 15:11:15 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5D9A9294DA for ; Fri, 16 Feb 2018 15:11:15 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 525F2294EA; Fri, 16 Feb 2018 15:11:15 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E7FAE294DA for ; Fri, 16 Feb 2018 15:11:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966541AbeBPPKJ (ORCPT ); Fri, 16 Feb 2018 10:10:09 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:56540 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966462AbeBPPKI (ORCPT ); Fri, 16 Feb 2018 10:10:08 -0500 Received: from hsi-kbw-5-158-153-53.hsi19.kabel-badenwuerttemberg.de ([5.158.153.53] helo=linux.lab.linutronix.de.) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.80) (envelope-from ) id 1emhau-0003qW-PP; Fri, 16 Feb 2018 16:06:37 +0100 From: John Ogness To: linux-fsdevel@vger.kernel.org Cc: Al Viro , Linus Torvalds , Christoph Hellwig , Thomas Gleixner , Peter Zijlstra , Sebastian Andrzej Siewior , linux-kernel@vger.kernel.org Subject: [PATCH 4/4] fs/dcache: Avoid the try_lock loops in dentry_kill() Date: Fri, 16 Feb 2018 16:09:33 +0100 Message-Id: <20180216150933.971-5-john.ogness@linutronix.de> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20180216150933.971-1-john.ogness@linutronix.de> References: <20180216150933.971-1-john.ogness@linutronix.de> X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1, SHORTCIRCUIT=-0.0001 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP dentry_kill() holds dentry->d_lock and needs to acquire both dentry->d_inode->i_lock and dentry->d_parent->d_lock. This cannot be done with spin_lock() operations because it's the reverse of the regular lock order. To avoid ABBA deadlocks it is done with two trylock loops. Trylock loops are problematic in two scenarios: 1) PREEMPT_RT converts spinlocks to 'sleeping' spinlocks, which are preemptible. As a consequence the i_lock holder can be preempted by a higher priority task. If that task executes the trylock loop it will do so forever and live lock. 2) In virtual machines trylock loops are problematic as well. The VCPU on which the i_lock holder runs can be scheduled out and a task on a different VCPU can loop for a whole time slice. In the worst case this can lead to starvation. Commits 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()") and 046b961b45f9 ("shrink_dentry_list(): take parent's d_lock earlier") are addressing exactly those symptoms. Avoid the trylock loops by using dentry_lock_inode() and lock_parent() which take the locks in the appropriate order. As both functions drop dentry->lock briefly, this requires rechecking of the dentry content as it might have changed after dropping the lock. Signed-off-by: John Ogness --- fs/dcache.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 60 insertions(+), 16 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index 2cd252f88c5d..b557679c14c9 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -673,27 +673,73 @@ static bool dentry_lock_inode(struct dentry *dentry) static struct dentry *dentry_kill(struct dentry *dentry) __releases(dentry->d_lock) { - struct inode *inode = dentry->d_inode; - struct dentry *parent = NULL; + struct dentry *parent; + struct inode *inode; + +again: + parent = NULL; + inode = dentry->d_inode; + if (inode) { + /* + * Lock the inode. Might drop dentry->d_lock temporarily + * which allows inode to change. Start over if that happens. + */ + if (!dentry_lock_inode(dentry)) + goto again; - if (inode && unlikely(!spin_trylock(&inode->i_lock))) - goto failed; + /* + * Recheck refcount as it might have been incremented while + * d_lock was dropped. + */ + if (unlikely(dentry->d_lockref.count != 1)) + goto drop_ref; + } - if (!IS_ROOT(dentry)) { - parent = dentry->d_parent; - if (unlikely(!spin_trylock(&parent->d_lock))) { - if (inode) - spin_unlock(&inode->i_lock); - goto failed; - } + parent = lock_parent(dentry); + + /* + * Check refcount because it might have changed + * while d_lock was dropped. + */ + if (unlikely(dentry->d_lockref.count != 1)) + goto drop_ref; + + /* + * The inode may have changed in the window where the + * dentry was unlocked. If that happens it is necessary + * to start over because the wrong inode lock is held. + */ + if (unlikely(inode != dentry->d_inode)) { + if (parent) + spin_unlock(&parent->d_lock); + if (inode) + spin_unlock(&inode->i_lock); + goto again; } __dentry_kill(dentry); return parent; -failed: +drop_ref: + /* + * If refcount is > 1 it was incremented while dentry->d_lock was + * dropped. Just decrement the refcount, unlock and tell the caller + * to stop the directory walk. + * + * For paranoia reasons check whether the refcount is < 1. If so, + * report the detection and avoid the decrement which would just + * cause a problem in some other place. The warning might be + * helpful to decode the root cause of the refcounting bug. + */ + if (!WARN_ON(dentry->d_lockref.count < 1)) + dentry->d_lockref.count--; + spin_unlock(&dentry->d_lock); - return dentry; /* try again with same dentry */ + if (parent) + spin_unlock(&parent->d_lock); + if (inode) + spin_unlock(&inode->i_lock); + return NULL; } /* @@ -865,10 +911,8 @@ void dput(struct dentry *dentry) kill_it: dentry = dentry_kill(dentry); - if (dentry) { - cond_resched(); + if (dentry) goto repeat; - } } EXPORT_SYMBOL(dput);