switch ocfs2 to ->evict_inode()
PATCH 43/58 switch ocfs2 to ->evict_inode()
--- 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
@@ -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
@@ -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
@@ -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
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@@ -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
-0sigset_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
Al Viro
maintainerQuoted 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.
Al Viro
maintainerQuoted 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_nlinklike 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...
Al Viro
maintainerPATCH v2 43/58 switch ocfs2 to ->evict_inode()
In reply to Al Virofs/ocfs2/inode.c +14 -7
@@ -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
@@ -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
@@ -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,
Quoted 1 line · Al Viro
Signed-off-by Al Viro <[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@@ -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@@ -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@@ -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