Re: [PATCH 7/7] XFS: don't use vnodes where unnecessary

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 14, 2008 at 04:10:22PM -0400, Christoph Hellwig wrote:
> >  	do {
> > +		struct inode	*inode;
> > +		boolean_t	vnode_refed;
> > +		xfs_inode_t	*ip = NULL;
> 
> This should not be inode_refed, no? :)

Yeah ;)

> > -		vp = VFS_I(ip);
> > -		if (!vp) {
> > +		if (xfs_iflags_test(ip, (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {
> 
> These changes really need to be folded into the previous patch for
> bisectability reasons..

Right - this was catching all this bits that I missed in the
previous patch ;)

> 
> > +		inode = VFS_I(ip);
> > +		if (VN_BAD(inode)) {
> 
> Just use is_bad_inode directly.  (Also in a few other places)

Will do.

> >  		} else {
> > -			/* safe to unlock here as we have a reference */
> > +			if (!xfs_ilock_nowait(ip, lock_flags)) {
> > +				/* leave it to reclaim */
> > +				read_unlock(&pag->pag_ici_lock);
> > +				continue;
> > +			}
> >  			read_unlock(&pag->pag_ici_lock);
> >  		}
> 
> Maybe not for this patch, but in general, why do we even bother about
> the inodes we can't get a reference to, shouldn't we just always leave
> this to reclaim?

Right now, we either get a reference or an ilock to prevent reclaim
from freeing it. The case ofthe vnode disappearing made it difficult
to make a clear delineation. With the combined inode, it is much
easier to make this distinction, but if we are doing ascending order
writeback of inodes, we really want to get all the inodes that are
dirty regardless of whether they are in reclaim or not.

> > -		if ((flags & SYNC_DELWRI) && VN_DIRTY(vp)) {
> > +		if ((flags & SYNC_DELWRI) && VN_DIRTY(VFS_I(ip))) {
> 
> Why not use the "inode" variable we have in scope anyway?

Yup, should do that.

> > +	if (!(inode->i_state & I_CLEAR))
> > +		return atomic_read(&inode->i_count);
> 
> Well, it's just zero when the inode is out of the vfs, so we could
> probably simply do it unconditional.

Ok, I'll have a look at that.

> 
> > +		vp = vn_grab(VFS_I(ip));
> > +		if (vp) {
> 
> Please switch this one to igrab, and the inode name, too.  As this
> is the last caller of vn_grab we can kill it now.

Ok, I'll fix that one up, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com