On Wed, Apr 02, 2008 at 04:25:09PM +1000, Barry Naujok wrote:
quoted text > Adds two pieces of functionality for the basis of case-insensitive
> support in XFS:
>
> 1. A comparison result enumerated type: xfs_dacmp_t. It represents an
> exact match, case-insensitive match or no match at all. This patch
> only implements different and exact results.
>
> 2. xfs_nameops vector for specifying how to perform the hash generation
> of filenames and comparision methods. In this patch the hash vector
> points to the existing xfs_da_hashname function and the comparison
> method does a length compare, and if the same, does a memcmp and
> return the xfs_dacmp_t result.
>
> All filename functions that use the hash (create, lookup remove, rename,
> etc) now use the xfs_nameops.hashname function and all directory lookup
> functions also use the xfs_nameops.compname function.
Ok, so internally I see this is not the case. I'll comment on that
inline.
quoted text > The lookup functions also handle case-insensitive results even though
> the default comparison function cannot return that. And important
> aspect of the lookup functions is that an exact match always has
> precedence over a case-insensitive. So while a case-insensitive match
> is found, we have to keep looking just in case there is an exact
> match. In the meantime, the info for the first case-insensitive match
> is retained if no exact match is found.
>
> Signed-off-by: Barry Naujok <bnaujok@sgi.com>
......
quoted text > }
>
> +xfs_dacmp_t
> +xfs_da_compname(const uchar_t *name1, int len1, const uchar_t *name2, int len2)
> +{
> + return (len1 == len2 && memcmp(name1, name2, len1) == 0) ?
> + XFS_CMP_EXACT : XFS_CMP_DIFFERENT;
> +}
> +
> +struct xfs_nameops xfs_default_nameops = {
const.
quoted text > #ifdef __KERNEL__
> /*========================================================================
> @@ -248,7 +271,12 @@ xfs_daddr_t xfs_da_reada_buf(struct xfs_
> int xfs_da_shrink_inode(xfs_da_args_t *args, xfs_dablk_t dead_blkno,
> xfs_dabuf_t *dead_buf);
>
> +extern struct xfs_nameops xfs_default_nameops;
Does this need global visibility? It's only needed in xfs_dir_mount(),
right?
quoted text > Index: kern_ci/fs/xfs/xfs_dir2.h
> ===================================================================
> --- kern_ci.orig/fs/xfs/xfs_dir2.h
> +++ kern_ci/fs/xfs/xfs_dir2.h
> @@ -85,6 +85,12 @@ extern int xfs_dir_canenter(struct xfs_t
> char *name, int namelen);
> extern int xfs_dir_ino_validate(struct xfs_mount *mp, xfs_ino_t ino);
>
> +#define xfs_dir_hashname(dp, n, l) \
> + ((dp)->i_mount->m_dirnameops->hashname((n), (l)))
> +
> +#define xfs_dir_compname(dp, n1, l1, n2, l2) \
> + ((dp)->i_mount->m_dirnameops->compname((n1), (l1), (n2), (l2)))
> +
Static inline functions, please.
quoted text > /*
> * Utility routines for v2 directories.
> */
> 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
> @@ -643,6 +643,7 @@ xfs_dir2_block_lookup_int(
> int mid; /* binary search current idx */
> xfs_mount_t *mp; /* filesystem mount point */
> xfs_trans_t *tp; /* transaction pointer */
> + xfs_dacmp_t cmp; /* comparison result */
>
> dp = args->dp;
> tp = args->trans;
> @@ -698,19 +699,33 @@ xfs_dir2_block_lookup_int(
> ((char *)block + xfs_dir2_dataptr_to_off(mp, addr));
> /*
> * Compare, if it's right give back buffer & entry number.
> + *
> + * lookup case - use nameops;
> + *
> + * replace/remove case - as lookup has been already been
> + * performed, look for an exact match using the fast method
> */
> - if (dep->namelen == args->namelen &&
> - dep->name[0] == args->name[0] &&
> - memcmp(dep->name, args->name, args->namelen) == 0) {
> + cmp = args->oknoent ?
> + xfs_dir_compname(dp, dep->name, dep->namelen,
> + args->name, args->namelen) :
> + xfs_da_compname(dep->name, dep->namelen,
> + args->name, args->namelen);
Why add this "fast path"? All you're saving here is a few
instructions but making the code much harder to follow.
cmp = xfs_dir_compname(dp, dep->name, dep->namelen,
args->name, args->namelen);
Will do exactly the same thing and I'd much prefer readable code
over prematurely optimised code any day of the week....
quoted text > + if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> + args->cmpresult = cmp;
> *bpp = bp;
> *entno = mid;
> - return 0;
> + if (cmp == XFS_CMP_EXACT)
> + return 0;
> }
> - } while (++mid < be32_to_cpu(btp->count) && be32_to_cpu(blp[mid].hashval) == hash);
> + } while (++mid < be32_to_cpu(btp->count) &&
> + be32_to_cpu(blp[mid].hashval) == hash);
> +
> + ASSERT(args->oknoent);
> + if (args->cmpresult == XFS_CMP_CASE)
> + return 0;
So if we find multiple case matches, we'll take the last we find?
quoted text > /*
> * No match, release the buffer and return ENOENT.
> */
> - ASSERT(args->oknoent);
> xfs_da_brelse(tp, bp);
> return XFS_ERROR(ENOENT);
Should we really be promoting that assert to before we return a successful
case match?
quoted text > @@ -1391,19 +1394,49 @@ xfs_dir2_leaf_lookup_int(
> xfs_dir2_dataptr_to_off(mp, be32_to_cpu(lep->address)));
> /*
> * If it matches then return it.
> + *
> + * lookup case - use nameops;
> + *
> + * replace/remove case - as lookup has been already been
> + * performed, look for an exact match using the fast method
> */
> - if (dep->namelen == args->namelen &&
> - dep->name[0] == args->name[0] &&
> - memcmp(dep->name, args->name, args->namelen) == 0) {
> - *dbpp = dbp;
> + cmp = args->oknoent ?
> + xfs_dir_compname(dp, dep->name, dep->namelen,
> + args->name, args->namelen) :
> + xfs_da_compname(dep->name, dep->namelen,
> + args->name, args->namelen);
Same again.
quoted text > + if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> + args->cmpresult = cmp;
> *indexp = index;
> - return 0;
> + if (cmp == XFS_CMP_EXACT) {
> + /*
> + * case exact match: release the case-insens.
> + * match buffer if it exists and return the
> + * current data buffer.
> + */
> + if (cbp && cbp != dbp)
> + xfs_da_brelse(tp, cbp);
> + *dbpp = dbp;
> + return 0;
> + }
> + cbp = dbp;
> }
> }
> + ASSERT(args->oknoent);
> + if (args->cmpresult == XFS_CMP_CASE) {
> + /*
> + * case-insensitive match: release current buffer and
> + * return the buffer with the case-insensitive match.
> + */
> + if (cbp != dbp)
> + xfs_da_brelse(tp, dbp);
> + *dbpp = cbp;
> + return 0;
> + }
> /*
> * No match found, return ENOENT.
> */
> - ASSERT(args->oknoent);
Same question about promoting the assert....
quoted text > @@ -578,19 +579,27 @@ xfs_dir2_leafn_lookup_int(
> /*
> * Compare the entry, return it if it matches.
> */
> - if (dep->namelen == args->namelen &&
> - dep->name[0] == args->name[0] &&
> - memcmp(dep->name, args->name, args->namelen) == 0) {
> + cmp = args->oknoent ?
> + xfs_dir_compname(dp, dep->name, dep->namelen,
> + args->name, args->namelen):
> + xfs_da_compname(dep->name, dep->namelen,
> + args->name, args->namelen);
Same again.
quoted text > @@ -907,9 +914,8 @@ xfs_dir2_sf_removename(
> for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp);
> i < sfp->hdr.count;
> i++, sfep = xfs_dir2_sf_nextentry(sfp, sfep)) {
> - if (sfep->namelen == args->namelen &&
> - sfep->name[0] == args->name[0] &&
> - memcmp(sfep->name, args->name, args->namelen) == 0) {
> + if (xfs_da_compname(sfep->name, sfep->namelen,
> + args->name, args->namelen) == XFS_CMP_EXACT) {
> ASSERT(xfs_dir2_sf_get_inumber(sfp,
> xfs_dir2_sf_inumberp(sfep)) ==
> args->inumber);
This only checks for an exact match - what is supposed to happen
with a XFS_CMP_CASE return?
quoted text > @@ -1044,9 +1050,9 @@ xfs_dir2_sf_replace(
> for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp);
> i < sfp->hdr.count;
> i++, sfep = xfs_dir2_sf_nextentry(sfp, sfep)) {
> - if (sfep->namelen == args->namelen &&
> - sfep->name[0] == args->name[0] &&
> - memcmp(args->name, sfep->name, args->namelen) == 0) {
> + if (xfs_da_compname(sfep->name, sfep->namelen,
> + args->name, args->namelen) ==
> + XFS_CMP_EXACT) {
ditto.
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to
majordomo@vger.kernel.org
More majordomo info at
http://vger.kernel.org/majordomo-info.html
