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@xxxxxxxxxxxxx
|