xfs
[Top] [All Lists]

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

To: Barry Naujok <bnaujok@xxxxxxx>
Subject: Re: [PATCH 4/7] XFS: Return case-insensitive match for dentry cache
From: David Chinner <dgc@xxxxxxx>
Date: Thu, 3 Apr 2008 15:22:09 +1000
Cc: xfs@xxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx
In-reply-to: <20080402062708.654277049@xxxxxxxxxxxxxxxxxxxxxxx>
References: <20080402062508.017738664@xxxxxxxxxxxxxxxxxxxxxxx> <20080402062708.654277049@xxxxxxxxxxxxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
On Wed, Apr 02, 2008 at 04:25:12PM +1000, Barry Naujok wrote:
> This implements the code to store the actual filename found
> during a lookup in the dentry cache and to avoid multiple entries
> in the dcache pointing to the same inode.
> 
> It also introduces a new type, xfs_name, which is similar to the
> dentry cache's qstr type. It contains a pointer to a zone allocated
> string (MAXNAMELEN sized) and the length of the actual name. This
> string does not need to be NULL terminated (a counted string).
> 
> xfs_name_t is only used in the lookup path for this patch, but may
> be used in other locations too if desired. It maybe desirable not
> to use xfs_name_t at all in the lookup functions but stick to
> separate parameters (which will mean 7 instead of 5 arguments).
> 
> To avoid polluting the dcache, we implement a new directory inode
> operations for lookup. xfs_vn_ci_lookup() interacts directly with
> the dcache and the code was derived from ntfs_lookup() in
> fs/ntfs/namei.c. The dentry hash and compare overrides introduced
> in the ASCII-CI patch has been removed.
> 
> The "actual name" is only allocated and returned for a case-
> insensitive match and not an actual match.

> +STATIC struct dentry *
> +xfs_vn_ci_lookup(
> +     struct inode    *dir,
> +     struct dentry   *dentry,
> +     struct nameidata *nd)
> +{
> +     struct xfs_inode *cip;
> +     int             error;
>       struct dentry   *result;
> +     struct qstr     ci_name = {0, 0, NULL};
> +     struct inode    *inode;
>  
>       if (dentry->d_name.len >= MAXNAMELEN)
>               return ERR_PTR(-ENAMETOOLONG);
>  
> -     if (xfs_sb_version_hasoldci(&mp->m_sb))
> -             dentry->d_op = &xfs_ci_dentry_operations;
> +     error = xfs_lookup(XFS_I(dir), &dentry->d_name, &cip, &ci_name);

Bit confusing with cip = "child inode" and ci_name = "case insensitive".
i.e. same prefix, different meanings...

>  
> -     error = xfs_lookup(XFS_I(dir), dentry, &cip);
>       if (unlikely(error)) {
>               if (unlikely(error != ENOENT))
>                       return ERR_PTR(-error);
>               d_add(dentry, NULL);
>               return NULL;
>       }
> +     inode = cip->i_vnode;
> +
> +     /* if exact match, just splice and exit */
> +     if (!ci_name.name) {
> +             result = d_splice_alias(inode, dentry);
> +             return result;
> +     }

        if (!ci_name.name)
                return d_splice_alias(inode, dentry);

>  
> -     result = d_splice_alias(cip->i_vnode, dentry);
> -     if (result)
> -             result->d_op = dentry->d_op;
> -     return result;
> +     /*
> +      * case-insensitive match, create a dentry to return and fill it
> +      * in with the correctly cased name. Parameter "dentry" is not
> +      * used anymore and the caller will free it.
> +      * Derived from fs/ntfs/namei.c
> +      */
> +
> +     ci_name.hash = full_name_hash(ci_name.name, ci_name.len);
> +
> +     /* Does an existing dentry match? */
> +     result = d_lookup(dentry->d_parent, &ci_name);
> +     if (!result) {
> +             /* if not, create one */
> +             result = d_alloc(dentry->d_parent, &ci_name);
> +             xfs_da_name_free((char *)ci_name.name);
> +             if (!result)
> +                     return ERR_PTR(-ENOMEM);
> +             dentry = d_splice_alias(inode, result);
> +             if (dentry) {
> +                     dput(result);
> +                     return dentry;
> +             }
> +             return result;
> +     }

This looks like it came from the ntfs code - i find that much easier
to follow with "real_dent" and "new_dent" instead of "result" and "dentry"
respectively.

> +     xfs_da_name_free((char *)ci_name.name);
> +
> +     /* an existing dentry matches, use it */

Ah, I see the rest of this is basically a copy and paste of the ntfs code
(without some of the useful comments). I think a generic helper function
is in order here that contains all the coments from the ntfs code....

> +
> +     if (result->d_inode) {
> +             /*
> +              * already an inode attached, deref the inode that was
> +              * refcounted with xfs_lookup and return the dentry.
> +              */
> +             if (unlikely(result->d_inode != inode)) {
> +                     /* This can happen because bad inodes are unhashed. */
> +                     BUG_ON(!is_bad_inode(inode));
> +                     BUG_ON(!is_bad_inode(result->d_inode));

Bit drastic - how about failing the lookup and returning EIO in this case?

> +             }
> +             iput(inode);
> +             return result;
> +     }

.....
> Index: kern_ci/fs/xfs/linux-2.6/xfs_super.c
> ===================================================================
> --- kern_ci.orig/fs/xfs/linux-2.6/xfs_super.c
> +++ kern_ci/fs/xfs/linux-2.6/xfs_super.c
> @@ -566,7 +566,10 @@ xfs_set_inodeops(
>               inode->i_mapping->a_ops = &xfs_address_space_operations;
>               break;
>       case S_IFDIR:
> -             inode->i_op = &xfs_dir_inode_operations;
> +             inode->i_op =
> +                     xfs_sb_version_hasoldci(&XFS_I(inode)->i_mount->m_sb) ?

+                       xfs_sb_version_hasoldci(&XFS_M(inode->i_sb)->m_sb) ?
        
> +     xfs_ino_t       *inum,          /* out: inode number */
> +     xfs_name_t      *ci_name)       /* out: actual name if different */
>  {
>       xfs_da_args_t   args;
>       int             rval;
> @@ -259,9 +260,9 @@ xfs_dir_lookup(
>       ASSERT((dp->i_d.di_mode & S_IFMT) == S_IFDIR);
>       XFS_STATS_INC(xs_dir_lookup);
>  
> -     args.name = name;
> -     args.namelen = namelen;
> -     args.hashval = xfs_dir_hashname(dp, name, namelen);
> +     args.name = name->name;
> +     args.namelen = name->len;
> +     args.hashval = xfs_dir_hashname(dp, name->name, name->len);
>       args.inumber = 0;
>       args.dp = dp;
>       args.firstblock = NULL;
> @@ -272,6 +273,8 @@ xfs_dir_lookup(
>       args.justcheck = args.addname = 0;
>       args.oknoent = 1;
>       args.cmpresult = XFS_CMP_DIFFERENT;
> +     args.value = NULL;
> +     args.valuelen = 0;

Rather than initialising more of the args to zero (already 7 members
explicitly initialised to zero or NULL), change it to:

        memset(&args, 0, sizeof(xfs_da_args_t));
        args.name = name->name;
        args.namelen = name->len;
        args.hashval = xfs_dir_hashname(dp, name->name, name->len);
        args.dp = dp;
        args.whichfork = XFS_DATA_FORK;
        args.trans = tp;
        args.oknoent = 1;
        args.cmpresult = XFS_CMP_DIFFERENT;

>  
>       if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
>               rval = xfs_dir2_sf_lookup(&args);
> @@ -287,8 +290,17 @@ xfs_dir_lookup(
>               rval = xfs_dir2_node_lookup(&args);
>       if (rval == EEXIST)
>               rval = 0;
> -     if (rval == 0)
> +     if (rval == 0) {

        if (!rval) {
> Index: kern_ci/fs/xfs/xfs_dir2_block.c
> ===================================================================
> --- kern_ci.orig/fs/xfs/xfs_dir2_block.c
> +++ kern_ci/fs/xfs/xfs_dir2_block.c
> @@ -616,6 +616,15 @@ xfs_dir2_block_lookup(
>        * Fill in inode number, release the block.
>        */
>       args->inumber = be64_to_cpu(dep->inumber);
> +     /*
> +      * If a case-insensitive match, allocate a buffer and copy the actual
> +      * name into the buffer. Return it via args->value.
> +      */
> +     if (args->cmpresult == XFS_CMP_CASE) {
> +             args->value = xfs_da_name_alloc();
> +             memcpy(args->value, dep->name, dep->namelen);
> +             args->valuelen = dep->namelen;

                xfs_da_ci_name_dup();
> +     }
>       xfs_da_brelse(args->trans, bp);
>       return XFS_ERROR(EEXIST);
>  }
> Index: kern_ci/fs/xfs/xfs_dir2_leaf.c
> ===================================================================
> --- kern_ci.orig/fs/xfs/xfs_dir2_leaf.c
> +++ kern_ci/fs/xfs/xfs_dir2_leaf.c
> @@ -1301,6 +1301,15 @@ xfs_dir2_leaf_lookup(
>        * Return the found inode number.
>        */
>       args->inumber = be64_to_cpu(dep->inumber);
> +     /*
> +      * If a case-insensitive match, allocate a buffer and copy the actual
> +      * name into the buffer. Return it via args->value.
> +      */
> +     if (args->cmpresult == XFS_CMP_CASE) {
> +             args->value = xfs_da_name_alloc();
> +             memcpy(args->value, dep->name, dep->namelen);
> +             args->valuelen = dep->namelen;

                xfs_da_ci_name_dup();
> +     }
>       xfs_da_brelse(tp, dbp);
>       xfs_da_brelse(tp, lbp);
>       return XFS_ERROR(EEXIST);
> Index: kern_ci/fs/xfs/xfs_dir2_node.c
> ===================================================================
> --- kern_ci.orig/fs/xfs/xfs_dir2_node.c
> +++ kern_ci/fs/xfs/xfs_dir2_node.c
> @@ -643,6 +643,8 @@ xfs_dir2_leafn_lookup_for_entry(
>                       xfs_dir2_dataptr_to_off(mp, be32_to_cpu(lep->address)));
>               /*
>                * Compare the entry, return it if it matches.
> +              * "oknoent" is set for lookup and clear for
> +              * remove and replace.
>                */

That should have been in an earlier patch....

>               cmp = args->oknoent ?
>                       xfs_dir_compname(dp, dep->name, dep->namelen,
> @@ -1857,10 +1859,22 @@ xfs_dir2_node_lookup(
>       if (error)
>               rval = error;
>       /*
> -      * If case-insensitive match was found in a leaf, return EEXIST.
> -      */
> -     else if (rval == ENOENT && args->cmpresult == XFS_CMP_CASE)
> +      * If case-insensitive match was found (xfs_dir2_leafn_lookup_int
> +      * returns ENOENT for a case-insensitive match, but sets
> +      * args->cmpresult to XFS_CMP_CASE):
> +      *   - Allocate a buffer and copy the actual name into the buffer and
> +      *       return it via args->value.
> +      *   - set rval to EEXIST
> +      */
> +     else if (rval == ENOENT && args->cmpresult == XFS_CMP_CASE) {
> +             xfs_dir2_data_entry_t   *dep = (xfs_dir2_data_entry_t *)
> +                                     ((char *)state->extrablk.bp->data +
> +                                             state->extrablk.index);
> +             args->value = xfs_da_name_alloc();
> +             memcpy(args->value, dep->name, dep->namelen);
> +             args->valuelen = dep->namelen;
>               rval = EEXIST;
> +     }

Yeah, more reason to move the comment inside the if block....

oh, and xfs_da_ci_name_dup()....

> -     if (args->cmpresult == XFS_CMP_CASE)
> +     if (args->cmpresult == XFS_CMP_CASE) {
> +             /*
> +              * If a case-insensitive match, allocate a buffer and copy the
> +              * actual name into the buffer and return it via args->value.
> +              */
> +             args->value = xfs_da_name_alloc();
> +             memcpy(args->value, ci_sfep->name, ci_sfep->namelen);
> +             args->valuelen = ci_sfep->namelen;

                xfs_da_ci_name_dup()

> --- kern_ci.orig/fs/xfs/xfs_utils.c
> +++ kern_ci/fs/xfs/xfs_utils.c
> @@ -24,6 +24,7 @@
>  #include "xfs_trans.h"
>  #include "xfs_sb.h"
>  #include "xfs_ag.h"
> +#include "xfs_da_btree.h"

What's that needed for? What ever it is, i think you've put it
in the wrong header file....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group


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