Skip to contentSkip to conversation

switch ocfs2 to ->evict_inode()

PATCH In review Al Viro 3 participants 6 messages Jun 8, 2010 – Jun 9, 2010

Subsystems (2)
VFS OCFS2
lore
Share & tools
lore

Al Viro

author
#0 lore raw

PATCH 43/58 switch ocfs2 to ->evict_inode()

Signed-off-by Al Viro <[email protected]>
---
 fs/ocfs2/inode.c |   23 ++++++++---------------
 fs/ocfs2/inode.h |    3 +--
 fs/ocfs2/super.c |    3 +--
 3 files changed, 10 insertions(+), 19 deletions(-)
fs/ocfs2/inode.c +8 -15
index abb0a95..29343c9 100644
@@ -969,14 +969,20 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
 	truncate_inode_pages(&inode->i_data, 0);
 }
 
-void ocfs2_delete_inode(struct inode *inode)
+void ocfs2_evict_inode(struct inode *inode)
 {
 	int wipe, status;
 	sigset_t oldset;
 	struct buffer_head *di_bh = NULL;
+	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 
 	mlog_entry("(inode->i_ino = %lu)\n", inode->i_ino);
 
+	if (inode->i_nlink) {
+		truncate_inode_pages(&inode->i_data, 0);
+		goto bail;
+	}
+
 	/* When we fail in read_inode() we mark inode as bad. The second test
 	 * catches the case when inode allocation fails before allocating
 	 * a block for inode. */

@@ -1075,19 +1081,7 @@ bail_unlock_nfs_sync:
 bail_unblock:
 	ocfs2_unblock_signals(&oldset);
 bail:
-	clear_inode(inode);
-	mlog_exit_void();
-}
-
-void ocfs2_clear_inode(struct inode *inode)
-{
-	int status;
-	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-
-	mlog_entry_void();
-
-	if (!inode)
-		goto bail;
+	end_writeback(inode);
 
 	mlog(0, "Clearing inode: %llu, nlink = %u\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno, inode->i_nlink);

@@ -1180,7 +1174,6 @@ void ocfs2_clear_inode(struct inode *inode)
 	jbd2_journal_release_jbd_inode(OCFS2_SB(inode->i_sb)->journal->j_journal,
 				       &oi->ip_jinode);
 
-bail:
 	mlog_exit_void();
 }
fs/ocfs2/inode.h +1 -2
index 9f5f5fc..975eedd 100644
@@ -123,8 +123,7 @@ static inline struct ocfs2_caching_info *INODE_CACHE(struct inode *inode)
 	return &OCFS2_I(inode)->ip_metadata_cache;
 }
 
-void ocfs2_clear_inode(struct inode *inode);
-void ocfs2_delete_inode(struct inode *inode);
+void ocfs2_evict_inode(struct inode *inode);
 void ocfs2_drop_inode(struct inode *inode);
 
 /* Flags for ocfs2_iget() */
fs/ocfs2/super.c +1 -2
index 0eaa929..ae1a443 100644
@@ -145,8 +145,7 @@ static const struct super_operations ocfs2_sops = {
 	.alloc_inode	= ocfs2_alloc_inode,
 	.destroy_inode	= ocfs2_destroy_inode,
 	.drop_inode	= ocfs2_drop_inode,
-	.clear_inode	= ocfs2_clear_inode,
-	.delete_inode	= ocfs2_delete_inode,
+	.evict_inode	= ocfs2_evict_inode,
 	.sync_fs	= ocfs2_sync_fs,
 	.put_super	= ocfs2_put_super,
 	.remount_fs	= ocfs2_remount,
Signature · 1 line
1.5.6.5
#1 lore raw
Quoted 2 lines · Al Viro
Signed-off-by Al Viro <[email protected]>
	First and foremost, I like the ->evict_inode() idea.  That's

really the state we're talking about; what to do when an inode is leaving icache. The evict scheme is much less confusing about icache lifetimes.

Quoted 12 lines
fs/ocfs2/inode.c +1 -1
index abb0a95..29343c9 100644
@@ -969,14 +969,20 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
 	truncate_inode_pages(&inode->i_data, 0);
 }
 
-void ocfs2_delete_inode(struct inode *inode)
+void ocfs2_evict_inode(struct inode *inode)
 {
 	int wipe, status;

fs/ocfs2/inode.c static void ocfs2_cleanup_delete_inode(struct inode *inode, · line 984

Quoted 10 lines
fs/ocfs2/inode.c +5 -0
 	sigset_t oldset;
 	struct buffer_head *di_bh = NULL;
+	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 
 	mlog_entry("(inode->i_ino = %lu)\n", inode->i_ino);
 
+	if (inode->i_nlink) {
+		truncate_inode_pages(&inode->i_data, 0);
+		goto bail;
+	}

Gonna have to NAK this as is, unfortunately. This i_nlink check
is not safe. In the old code, we get into ->delete_inode() either when the local i_nlink==0 (which your code handles just fine) or when ocfs2_drop_inode() notices the MAYBE_ORPHANED flag and explicitly calls generic_delete_inode(). In ocfs2_delete_inode(), we don't check our i_nlink until ocfs2_query_wipe_inode(), which is safely under the cluster lock.
So the old code, with its distinct delete_inode path, lets us
know deterministically when an inode must still be alive. Our old ocfs2_clear_inode() can safely avoid taking any cluster locks. It only has to free up and drop locks we're already holding.
Your new code is trivially functional by removing the i_nlink
check. The ocfs2_delete_inode() code is smart enough to realize the inode is actually still alive after locking it down. That is, after all, what we do in the MAYBE_ORPHANED case. However, that means every single inode eviction has to take cluster locks we may not be holding. That's not acceptable from a performance perspective.

	I do think it works, though, if you check for MAYBE_ORPHANED.

	if (inode->i_nlink && !(oi->ip_falgs & OCFS2_INODE_MAYBE_ORPHANED)) {
		ocfs2_cleanup_delete_inode(inode, 0);
		goto bail;
	}

Yeah, that makes me happy.

Quoted 8 lines
fs/ocfs2/inode.c +0 -4
@@ -1075,19 +1081,7 @@ bail_unlock_nfs_sync:
 bail_unblock:
 	ocfs2_unblock_signals(&oldset);
 bail:
-	clear_inode(inode);
-	mlog_exit_void();
-}
-

fs/ocfs2/inode.c bail_unlock_nfs_sync: · line 1084

Quoted 10 lines
fs/ocfs2/inode.c +1 -9
-void ocfs2_clear_inode(struct inode *inode)
-{
-	int status;
-	struct ocfs2_inode_info *oi = OCFS2_I(inode);
-
-	mlog_entry_void();
-
-	if (!inode)
-		goto bail;
+	end_writeback(inode);

The other thing that bothers me about your change is the
smushing of two long functions into a longer one. It just doesn't read nicely to me.
What about making ocfs2_delete_inode() and ocfs2_clear_inode()
static and then adding:

void ocfs2_evict_inode(struct inode *inode)

{
	if (!inode->i_nlink ||
	    (OCFS2_I(inode)->ip_flags & OCFS2_INODE_MAYBE_ORPHANED)) {
		ocfs2_delete_inode(inode);
	} else
		truncate_inode_pages(&inode->i_data, 0);
	ocfs2_clear_inode(inode);
}

end_writeback() gets added to ocfs2_clear_inode() as expected.

Joel

Signature · 12 lines
"There is shadow under this red rock.
 (Come in under the shadow of this red rock)
 And I will show you something different from either
 Your shadow at morning striding behind you
 Or your shadow at evening rising to meet you.
 I will show you fear in a handful of dust."

Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
maintainer
#2 lore raw
Quoted 6 lines · Joel Becker
	I do think it works, though, if you check for MAYBE_ORPHANED.

	if (inode->i_nlink && !(oi->ip_falgs & OCFS2_INODE_MAYBE_ORPHANED)) {
		ocfs2_cleanup_delete_inode(inode, 0);
		goto bail;
	}

Damn, that probably means we have other problems like that. Other filesystems that have extra "delete" cases (and do not play with i_nlink like gfs2 does) and have non-trivial ->delete_inode().

Ho-hum... OK, not too many. btrfs seems to be the only one besides ocfs2. Will fix...

I'll post replacement patches in a few.

maintainer
#3 lore raw
Quoted 8 lines · Al Viro
Quoted 6 lines · Joel Becker
	I do think it works, though, if you check for MAYBE_ORPHANED.

	if (inode->i_nlink && !(oi->ip_falgs & OCFS2_INODE_MAYBE_ORPHANED)) {
		ocfs2_cleanup_delete_inode(inode, 0);
		goto bail;
	}
Quoted 8 lines

Damn, that probably means we have other problems like that. Other filesystems that have extra "delete" cases (and do not play with i_nlink like gfs2 does) and have non-trivial ->delete_inode().

Ho-hum... OK, not too many. btrfs seems to be the only one besides ocfs2. Will fix...

I'll post replacement patches in a few.

Posted and pushed to vfs-2.6.git...

maintainer
#4 lore raw

PATCH v2 43/58 switch ocfs2 to ->evict_inode()

In reply to Al Viro
Signed-off-by Al Viro <[email protected]>
fs/ocfs2/inode.c +14 -7
index abb0a95..eb7fd07 100644
@@ -969,7 +969,7 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
 	truncate_inode_pages(&inode->i_data, 0);
 }
 
-void ocfs2_delete_inode(struct inode *inode)
+static void ocfs2_delete_inode(struct inode *inode)
 {
 	int wipe, status;
 	sigset_t oldset;

@@ -1075,20 +1075,17 @@ bail_unlock_nfs_sync:
 bail_unblock:
 	ocfs2_unblock_signals(&oldset);
 bail:
-	clear_inode(inode);
 	mlog_exit_void();
 }
 
-void ocfs2_clear_inode(struct inode *inode)
+static void ocfs2_clear_inode(struct inode *inode)
 {
 	int status;
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 
 	mlog_entry_void();
 
-	if (!inode)
-		goto bail;
-
+	end_writeback(inode);
 	mlog(0, "Clearing inode: %llu, nlink = %u\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno, inode->i_nlink);
 

@@ -1180,10 +1177,20 @@ void ocfs2_clear_inode(struct inode *inode)
 	jbd2_journal_release_jbd_inode(OCFS2_SB(inode->i_sb)->journal->j_journal,
 				       &oi->ip_jinode);
 
-bail:
 	mlog_exit_void();
 }
 
+void ocfs2_evict_inode(struct inode *inode)
+{
+	if (!inode->i_nlink ||
+	    (OCFS2_I(inode)->ip_flags & OCFS2_INODE_MAYBE_ORPHANED)) {
+		ocfs2_delete_inode(inode);
+	} else {
+		truncate_inode_pages(&inode->i_data, 0);
+	}
+	ocfs2_clear_inode(inode);
+}
+
 /* Called under inode_lock, with no more references on the
  * struct inode, so it's safe here to check the flags field
  * and to manipulate i_nlink without any other locks. */
fs/ocfs2/inode.h +1 -2
index 9f5f5fc..975eedd 100644
@@ -123,8 +123,7 @@ static inline struct ocfs2_caching_info *INODE_CACHE(struct inode *inode)
 	return &OCFS2_I(inode)->ip_metadata_cache;
 }
 
-void ocfs2_clear_inode(struct inode *inode);
-void ocfs2_delete_inode(struct inode *inode);
+void ocfs2_evict_inode(struct inode *inode);
 void ocfs2_drop_inode(struct inode *inode);
 
 /* Flags for ocfs2_iget() */
fs/ocfs2/super.c +1 -2
index 0eaa929..ae1a443 100644
@@ -145,8 +145,7 @@ static const struct super_operations ocfs2_sops = {
 	.alloc_inode	= ocfs2_alloc_inode,
 	.destroy_inode	= ocfs2_destroy_inode,
 	.drop_inode	= ocfs2_drop_inode,
-	.clear_inode	= ocfs2_clear_inode,
-	.delete_inode	= ocfs2_delete_inode,
+	.evict_inode	= ocfs2_evict_inode,
 	.sync_fs	= ocfs2_sync_fs,
 	.put_super	= ocfs2_put_super,
 	.remount_fs	= ocfs2_remount,
Acked-by on v2 43/58
#5 lore raw
Quoted 1 line · Al Viro
Signed-off-by Al Viro <[email protected]>
Acked-by Joel Becker <[email protected]>

Let me know when you push this to your next branch. I want to
try and schedule some testing.

Joel

Quoted 91 lines
fs/ocfs2/inode.c +14 -7
index abb0a95..eb7fd07 100644
@@ -969,7 +969,7 @@ static void ocfs2_cleanup_delete_inode(struct inode *inode,
 	truncate_inode_pages(&inode->i_data, 0);
 }
 
-void ocfs2_delete_inode(struct inode *inode)
+static void ocfs2_delete_inode(struct inode *inode)
 {
 	int wipe, status;
 	sigset_t oldset;

@@ -1075,20 +1075,17 @@ bail_unlock_nfs_sync:
 bail_unblock:
 	ocfs2_unblock_signals(&oldset);
 bail:
-	clear_inode(inode);
 	mlog_exit_void();
 }
 
-void ocfs2_clear_inode(struct inode *inode)
+static void ocfs2_clear_inode(struct inode *inode)
 {
 	int status;
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 
 	mlog_entry_void();
 
-	if (!inode)
-		goto bail;
-
+	end_writeback(inode);
 	mlog(0, "Clearing inode: %llu, nlink = %u\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno, inode->i_nlink);
 

@@ -1180,10 +1177,20 @@ void ocfs2_clear_inode(struct inode *inode)
 	jbd2_journal_release_jbd_inode(OCFS2_SB(inode->i_sb)->journal->j_journal,
 				       &oi->ip_jinode);
 
-bail:
 	mlog_exit_void();
 }
 
+void ocfs2_evict_inode(struct inode *inode)
+{
+	if (!inode->i_nlink ||
+	    (OCFS2_I(inode)->ip_flags & OCFS2_INODE_MAYBE_ORPHANED)) {
+		ocfs2_delete_inode(inode);
+	} else {
+		truncate_inode_pages(&inode->i_data, 0);
+	}
+	ocfs2_clear_inode(inode);
+}
+
 /* Called under inode_lock, with no more references on the
  * struct inode, so it's safe here to check the flags field
  * and to manipulate i_nlink without any other locks. */
fs/ocfs2/inode.h +1 -2
index 9f5f5fc..975eedd 100644
@@ -123,8 +123,7 @@ static inline struct ocfs2_caching_info *INODE_CACHE(struct inode *inode)
 	return &OCFS2_I(inode)->ip_metadata_cache;
 }
 
-void ocfs2_clear_inode(struct inode *inode);
-void ocfs2_delete_inode(struct inode *inode);
+void ocfs2_evict_inode(struct inode *inode);
 void ocfs2_drop_inode(struct inode *inode);
 
 /* Flags for ocfs2_iget() */
fs/ocfs2/super.c +1 -3
index 0eaa929..ae1a443 100644
@@ -145,8 +145,7 @@ static const struct super_operations ocfs2_sops = {
 	.alloc_inode	= ocfs2_alloc_inode,
 	.destroy_inode	= ocfs2_destroy_inode,
 	.drop_inode	= ocfs2_drop_inode,
-	.clear_inode	= ocfs2_clear_inode,
-	.delete_inode	= ocfs2_delete_inode,
+	.evict_inode	= ocfs2_evict_inode,
 	.sync_fs	= ocfs2_sync_fs,
 	.put_super	= ocfs2_put_super,
 	.remount_fs	= ocfs2_remount,
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 9 lines
Life's Little Instruction Book #337

	"Reread your favorite book."

Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127