xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 6/7] XFS: Combine the XFS and Linux inodes.
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 14 Aug 2008 16:00:06 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1218698083-11226-7-git-send-email-david@xxxxxxxxxxxxx>
References: <1218698083-11226-1-git-send-email-david@xxxxxxxxxxxxx> <1218698083-11226-7-git-send-email-david@xxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
> +     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.

> + * 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?

> +     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..)

> +/* 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..

> -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 :)

>  /*
> + * 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?
Also the return statement is superflous.

>  /* 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..


<Prev in Thread] Current Thread [Next in Thread>