Re: [PATCH 2/4] XFS: Return case-insensitive match for dentry cache

["Barry Naujok" <bnaujok@xxxxxxx>]

On Mon, 21 Apr 2008 18:59:47 +1000, Christoph Hellwig <hch@xxxxxxxxxxxxx>  
wrote:

> > +STATIC struct dentry *
> > +xfs_ci_dentry_update(
> > +       struct dentry   *dent,
> > +       struct inode    *dent_inode,
> > +       struct xfs_name *name)
> > +{
> > +       int             err;
> > +       struct dentry   *real_dent;
> > +       struct dentry   *new_dent;
> > +       struct qstr     nls_name;
>
> This helper should go into fs/dcache.c with a slightly move descritive
> name (d_add_ci?).
>
> Also the naming is rather odd, please replace every occurance of
> dent with dentry, and the variable dent_inode should be just inode.
>
> Also when moving this to dcache.c please provide a nice big kerneldoc
> comment describing it like Anton did for the ntfs lookup instance.
> of the
>
> > +       /*
> > +        * following code from ntfs_lookup() in fs/ntfs/namei.c
> > +        */
>
> Not a very helpful comment :)
>
> > +               new_dent = d_splice_alias(dent_inode, real_dent);
> > +               if (new_dent)
> > +                       dput(real_dent);
> > +               else
> > +                       new_dent = real_dent;
> > +               return new_dent;
>
> I think this would be more readable as
>
>                 if (new_dentry) {
>                         dput(real_dentry);
>                         return new_dentry;
>                 }
>                 return real_dentry;
>
> > +       /* Matching dentry exists, check if it is negative. */
> > +       if (real_dent->d_inode) {
> > +               if (unlikely(real_dent->d_inode != dent_inode)) {
> > +                       /* This can happen because bad inodes are unhashed. */
> > +                       BUG_ON(!is_bad_inode(dent_inode));
> > +                       BUG_ON(!is_bad_inode(real_dent->d_inode));
> > +               }
>
> Shouldn't this be a can't above?
>
> > +                * Already have the inode and the dentry attached, decrement
> > +                * the reference count to balance the xfs_lookup() we did
> > +                * earlier on.  We found the dentry using d_lookup() so it
> > +                * cannot be disconnected and thus we do not need to worry
> > +                * about any NFS/disconnectedness issues here.
>
> 		s/xfs_lookup/iget/

This whole function (other than the xfs_blah) is a direct copy of
Anton's code, I left the variable names etc the same.

> > +kmem_zone_t	*xfs_name_zone;
>
> What about just using kmalloc here?  We know the length of the name
> anyway, so there is no point of allocating the maximum possible size.

Not with the CI/NLS, the length could be different.

> >         error = xfs_iget(dp->i_mount, NULL, inum, 0, 0, ipp, 0);
> > -       if (error)
> > +       if (error) {
> > +               if (ci_match && *ci_match)
> > +                       xfs_name_free(name->name);
> >                 goto out;
> > +       }
>
> All the allocation and freeing for ci_match looks odd and error prone
> to me.  I think the low-level directory code should never allocate
> args->value unless it's explicitly asked for a CI match.  That way
> there's only one place in xfs_ci_lookup to free it either.

Yes, I could do that, it is a neater, cleaner implementation. I did it
this way to only alloc as required rather than an alloc for every lookup.

Barry.