Re: [PATCH 43/58] switch ocfs2 to ->evict_inode()

[Joel Becker <Joel.Becker@oracle.com>]

On Tue, Jun 08, 2010 at 11:22:03PM +0100, Al Viro wrote:
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

	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@oracle.com
Phone: (650) 506-8127