[Top] [All Lists]

[PATCH] xfs: only take the ILOCK in xfs_reclaim_inode()

To: xfs@xxxxxxxxxxx
Subject: [PATCH] xfs: only take the ILOCK in xfs_reclaim_inode()
From: Alex Elder <elder@xxxxxxxxxxxxx>
Date: Thu, 16 Feb 2012 16:01:00 -0600
Dkim-signature: v=1; a=rsa-sha1; c=relaxed; d=dreamhost.com; h= message-id:subject:from:reply-to:to:date:content-type :content-transfer-encoding:mime-version; s=dreamhost.com; bh=fgQ 0Mf1iXFMNMLITo5JaNIbz81k=; b=ktx/HaFFj8wgQMF4HXQG7jBAjeJgNPRa+iG hcCHZZXoq8plTpt7JPUwj9yMQjRuQmNOy82yM7K6zL5cXyr9SEp6nyfWOxBbufWR 7FcrcugrBUCyBQDj7cj2Uv6jdsx0p1o2Xp1KuZk9j10nwDol2WYoFPzZaKMZ2LTn ZcYgfebM=
Domainkey-signature: a=rsa-sha1; c=nofws; d=dreamhost.com; h=message-id :subject:from:reply-to:to:date:content-type :content-transfer-encoding:mime-version; q=dns; s=dreamhost.com; b=KQyOnkoK3VVYGIvqODg2lMKHiPrOkzKrol5ote12CLr8qqpR/Ml+uRXbnW3PS jCKJUx1MLz1K8mZDGX8ANVWtLdMQ6+qKjXk6vaJHcEViHcUYJmGdw9rzQ5snOtRD dOq87ffk2gLjoaeS5RGSRHGOzBa46gf9NaQ7hJ1gSsv1ew=
Reply-to: elder@xxxxxxxxxxxxx
At the end of xfs_reclaim_inode(), the inode is locked in order to
we wait for a possible concurrent lookup to complete before the
inode is freed.  This synchronization step was taking both the ILOCK
and the IOLOCK, but the latter was causing lockdep to produce
reports of the possibility of deadlock.

It turns out that there's no need to acquire the IOLOCK at this
point anyway.  It may have been required in some earlier version of
the code, but there should be no need to take the IOLOCK in
xfs_iget(), so there's no (longer) any need to get it here for
synchronization.  Add an assertion in xfs_iget() as a reminder
of this assumption.

Dave Chinner diagnosed this on IRC, and Christoph Hellwig suggested
no longer including the IOLOCK.  I just put together the patch.

Signed-off-by: Alex Elder <elder@xxxxxxxxxxxxx>
 fs/xfs/xfs_iget.c |    9 +++++++++
 fs/xfs/xfs_sync.c |   10 ++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
index 0fa98b1..39d51d9 100644
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -421,6 +421,15 @@ xfs_iget(
        xfs_perag_t     *pag;
        xfs_agino_t     agino;
+       /*
+        * xfs_reclaim_inode() uses the ILOCK to ensure an inode
+        * doesn't get freed while it's being referenced during a
+        * radix tree traversal here.  It assumes this function
+        * aqcuires only the ILOCK (and therefore it has no need to
+        * involve the IOLOCK in this synchronization).
+        */
+       ASSERT((lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) == 0);
        /* reject inode numbers outside existing AGs */
        if (!ino || XFS_INO_TO_AGNO(mp, ino) >= mp->m_sb.sb_agcount)
                return EINVAL;
diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
index f0994aedc..61c6986 100644
--- a/fs/xfs/xfs_sync.c
+++ b/fs/xfs/xfs_sync.c
@@ -918,17 +918,15 @@ reclaim:
         * can reference the inodes in the cache without taking references.
         * We make that OK here by ensuring that we wait until the inode is
-        * unlocked after the lookup before we go ahead and free it.  We get
-        * both the ilock and the iolock because the code may need to drop the
-        * ilock one but will still hold the iolock.
+        * unlocked after the lookup before we go ahead and free it.
-       xfs_ilock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
+       xfs_ilock(ip, XFS_ILOCK_EXCL);
-       xfs_iunlock(ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
+       xfs_iunlock(ip, XFS_ILOCK_EXCL);
-       return error;
+       return error;

<Prev in Thread] Current Thread [Next in Thread>