Re: [PATCH 01/10] xfs: Make inode reclaim states explicit

From: Dave Chinner <david@fromorbit.com>
To: Alex Elder <aelder@sgi.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 01/10] xfs: Make inode reclaim states explicit
Date: Sat, 6 Feb 2010 11:07:52 +1100	[thread overview]
Message-ID: <20100206000752.GF11483@discord.disaster> (raw)
In-Reply-To: <1265396780.2714.23.camel@doink1>

On Fri, Feb 05, 2010 at 01:06:20PM -0600, Alex Elder wrote:
> On Wed, 2010-02-03 at 10:24 +1100, Dave Chinner wrote:
> > A.K.A.: don't rely on xfs_iflush() return value in reclaim
> > 
> > We have gradually been moving checks out of the reclaim code because
> > they are duplicated in xfs_iflush(). We've had a history of problems
> > in this area, and many of them stem from the overloading of the
> > return values from xfs_iflush() and interaction with inode flush
> > locking to determine if the inode is safe to reclaim.
> > 
> > With the desire to move to delayed write flushing of inodes and
> > non-blocking inode tree reclaim walks, the overloading of the
> > return value of xfs_iflush makes it very difficult to determine
> > the correct thing to do next.
> > 
> > This patch explicitly re-adds the checks to the inode reclaim code,
> > removing the reliance on the return value of xfs_iflush() to
> > determine what to do next. It also means that we can clearly
> > document all the inode states that reclaim must handle and hence
> > we can easily see that we handled all the necessary cases.
> > 
> > This also removes the need for the xfs_inode_clean() check in
> > xfs_iflush() as all callers now check this first (safely).
> 
> I have a few comments, below.  One is a real bug.  At this
> point I'm trusting that your enumeration of all the possible
> inode states is correct.  Other than what I indicate below,
> this change looks good.

...

> It looks to me like your code adds a call to xfs_ifunlock(ip)
> in the bad inode case that was not there before.  My guess is
> that your way is correct, but I'd like to know whether you
> agree this differs.  Is this intentional?  Regardless, is
> the new xfs_ifunlock() call correct?

Yes, it is correct - the code before was broken.

> > +	if (xfs_iflags_test(ip, XFS_ISTALE))
> > +		goto reclaim;
> > +	if (xfs_inode_clean(ip))
> > +		goto reclaim;
> > +
> > +	/* Now we have an inode that needs flushing */
> > +	error = xfs_iflush(ip, sync_mode);
> > +	if (!error) {
> > +		switch(sync_mode) {
> > +		case XFS_IFLUSH_DELWRI_ELSE_ASYNC:
> > +		case XFS_IFLUSH_DELWRI:
> > +		case XFS_IFLUSH_ASYNC:
> > +		case XFS_IFLUSH_DELWRI_ELSE_SYNC:
> > +		case XFS_IFLUSH_SYNC:
> > +			/* IO issued, synchronise with IO completion */
> > +			xfs_iflock(ip);
> 
> You must not have run this with DEBUG enabled.
> You need a "break;" here.

I always run with DEBUG enabled, but I probably didn't
test this patch by iteself in the last iteration because
i don't have unlimited test cycles. This code gets removed
by the next patch, so the fact that it was broken would
not have been picked up by testing. I'll update it
and re-push it to the for-2.6.34 branch.

Cheers,
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs