On Tue, Jun 08, 2010 at 11:22:03PM +0100, Al Viro wrote:
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
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.
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index abb0a95..29343c9 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -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;
> + }
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.
> @@ -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);
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
--
"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: joel.becker@xxxxxxxxxx
Phone: (650) 506-8127
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
