Re: [PATCH 6/7] XFS: Combine the XFS and Linux inodes.

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 14, 2008 at 04:00:06PM -0400, Christoph Hellwig wrote:
> > +	if (!(inode->i_state & I_CLEAR)) {
> >  		ip->i_d.di_atime.t_sec = (__int32_t)inode->i_atime.tv_sec;
> >  		ip->i_d.di_atime.t_nsec = (__int32_t)inode->i_atime.tv_nsec;
> >  	}
> 
> Actually we can just do this unconditionally, as the atime values don't
> become invalid just because the VFS doesn't know about the inode
> anymore.  And two useless because already previously updated assignments
> are cheaper than a branch.

Ok, I'll make it unconditional.

> > + * We are always called with an uninitialised linux inode here.
> > + * We need to initialise the necessary fields and take a reference
> > + * on it.
> >   */
> >  void
> >  xfs_setup_inode(
> >  	struct xfs_inode	*ip)
> >  {
> > -	struct inode		*inode = ip->i_vnode;
> > +	struct inode		*inode = &ip->i_vnode;
> 
> VFS_I?

Yup, I thought I caught all of them :/

> 
> > +	inode->i_ino = ip->i_ino;
> > +	inode->i_state = I_NEW|I_LOCK;
> > +	inode_used(ip->i_mount->m_super, inode);
> > +	ASSERT(atomic_read(&inode->i_count) == 1);
> 
> Where does inode_used come from?  (It's also a rather ugly name..)

Ah, separate patch not sent. basically does:

void inode_used(struct super_block *sb, struct inode *inode)
{
       spin_lock(&inode_lock);
       inodes_stat.nr_inodes++;
       list_add(&inode->i_list, &inode_in_use);
       list_add(&inode->i_sb_list, &sb->s_inodes);
       spin_unlock(&inode_lock);
}

> > +/* XXX: development debug only */
> >  STATIC struct inode *
> >  xfs_fs_alloc_inode(
> >  	struct super_block	*sb)
> >  {
> > -	return kmem_zone_alloc(xfs_vnode_zone, KM_SLEEP);
> > +	BUG();
> >  }
> 
> Actually keeping this one is a good idea, even if it's just to catch
> really dumb things like the iget_locked OpenAFS's cache manager does on
> random filesystems..

Ok, I'll leave that in.

> > -void
> > -xfs_inode_init_once(
> > +STATIC void
> > +xfs_fs_inode_init_once(
> 
> might be a good idea to already use that name in the patch that
> introduces it :)

The problem was that we have two init functions at this point;
one for the vnode, one for the inode....

> >  /*
> > + * we need to provide an empty inode free function to prevent
> > + * the generic code from trying to free ouuur combined inode.
> > + */
> > +STATIC void
> > +xfs_fs_destroy_inode(
> > +	struct inode		*inode)
> > +{
> > +	return;
> > +}
> 
> Why it's this kept in the original place, close to alloc_inode?

Just the way things happened. I'll move it back.

> Also the return statement is superflous.

Yup.

> >  /* convert from xfs inode to vfs inode */
> >  static inline struct inode *VFS_I(struct xfs_inode *ip)
> >  {
> > -	return (struct inode *)ip->i_vnode;
> > +	return (struct inode *)&ip->i_vnode;
> >  }
> 
> No need for the cast in either version..

Ok, I'll kill it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com