Re: [REVIEW 1/2] Case insensitive support for XFS - kernel patch

[Eric Sandeen <sandeen@sandeen.net>]

Barry Naujok wrote:
> This patch should apply to 2.6.24-rc6.

actually doesn't seem to; at least doesn't apply to rc8... mount
handling has moved, etc - well, easy enough to fix up.

The samba guys are excited about this, I bet :)

Lots of comments below; keep an open mind and a sense of humor, most
aren't demands but ideas, musings etc...


> ===========================================================================
> fs/xfs/Makefile
> ===========================================================================
> 
> --- a/fs/xfs/Makefile	2008-01-18 15:31:23.000000000 +1100
> +++ b/fs/xfs/Makefile	2007-10-23 16:17:22.173903950 +1000
> @@ -74,6 +74,7 @@ xfs-y				+= xfs_alloc.o \
>  				   xfs_trans_extfree.o \
>  				   xfs_trans_inode.o \
>  				   xfs_trans_item.o \
> +				   xfs_unicode.o \

obj-$(CONFIG_XFS_UNICODE)  perhaps?  It's a lot of new code that not
everyone needs?

>  				   xfs_utils.o \
>  				   xfs_vfsops.o \
>  				   xfs_vnodeops.o \
> 
...

> ===========================================================================
> fs/xfs/linux-2.6/xfs_iops.c
> ===========================================================================
> 
> --- a/fs/xfs/linux-2.6/xfs_iops.c	2008-01-18 15:31:23.000000000 +1100
> +++ b/fs/xfs/linux-2.6/xfs_iops.c	2008-01-17 12:26:26.905427167 +1100
> @@ -47,6 +47,8 @@
>  #include "xfs_buf_item.h"
>  #include "xfs_utils.h"
>  #include "xfs_vnodeops.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_unicode.h"
>  
>  #include <linux/capability.h>
>  #include <linux/xattr.h>
> @@ -388,7 +390,7 @@ xfs_vn_lookup(
>  	if (dentry->d_name.len >= MAXNAMELEN)
>  		return ERR_PTR(-ENAMETOOLONG);
>  
> -	error = xfs_lookup(XFS_I(dir), dentry, &cvp);
> +	error = xfs_lookup(XFS_I(dir), dentry, &cvp, NULL, NULL);
>  	if (unlikely(error)) {
>  		if (unlikely(error != ENOENT))
>  			return ERR_PTR(-error);
> @@ -399,6 +401,113 @@ xfs_vn_lookup(
>  	return d_splice_alias(vn_to_inode(cvp), dentry);
>  }
>  
> +STATIC struct dentry *
> +xfs_vn_ci_lookup(
> +	struct inode	*dir,
> +	struct dentry	*dentry,
> +	struct nameidata *nd)
> +{
> +	bhv_vnode_t	*cvp;
> +	int		error;
> +	struct dentry	*result;
> +	struct qstr	actual_name;
> +	struct inode	*inode;
> +
> +	if (dentry->d_name.len >= MAXNAMELEN)
> +		return ERR_PTR(-ENAMETOOLONG);
> +
> +	error = xfs_lookup(XFS_I(dir), dentry, &cvp, (char **)&actual_name.name,
> +			&actual_name.len);
> +	if (unlikely(error)) {
> +		if (unlikely(error != ENOENT))
> +			return ERR_PTR(-error);
> +		d_add(dentry, NULL);
> +		return NULL;
> +	}
> +	inode = vn_to_inode(cvp);
> +
> +	/* if exact match, just splice and exit */
> +	if (!actual_name.name) {
> +		result = d_splice_alias(inode, dentry);
> +		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
> +	 */
> +
> +	actual_name.hash = full_name_hash(actual_name.name, actual_name.len);
> +
> +	/* Does an existing dentry match? */
> +	result = d_lookup(dentry->d_parent, &actual_name);
> +	if (!result) {
> +		/* if not, create one */
> +		result = d_alloc(dentry->d_parent, &actual_name);
> +		xfs_free_unicode_nls_name((char *)actual_name.name);
> +		if (!result)
> +			return ERR_PTR(-ENOMEM);
> +		dentry = d_splice_alias(inode, result);
> +		if (dentry) {
> +			dput(result);
> +			return dentry;
> +		}
> +		return result;
> +	}
> +	xfs_free_unicode_nls_name((char *)actual_name.name);
> +
> +	/* an existing dentry matches, use it */
> +
> +	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));
> +		}
> +		iput(inode);
> +		return result;
> +	}
> +
> +	if (!S_ISDIR(inode->i_mode)) {
> +		/* not a directory, easy to handle */
> +		d_instantiate(result, inode);
> +		return result;
> +	}
> +
> +	spin_lock(&dcache_lock);
> +	if (list_empty(&inode->i_dentry)) {
> +		/*
> +		 * Directory without a 'disconnected' dentry; we need to do
> +		 * d_instantiate() by hand because it takes dcache_lock which
> +		 * we already hold.
> +		 */
> +		list_add(&result->d_alias, &inode->i_dentry);
> +		result->d_inode = inode;
> +		spin_unlock(&dcache_lock);
> +		security_d_instantiate(result, inode);
> +		return result;
> +	}
> +	/*
> +	 * Directory with a 'disconnected' dentry; get a reference to the
> +	 * 'disconnected' dentry.
> +	 */
> +	dentry = list_entry(inode->i_dentry.next, struct dentry, d_alias);
> +	dget_locked(dentry);
> +	spin_unlock(&dcache_lock);
> +	security_d_instantiate(result, inode);
> +	d_move(dentry, result);
> +	iput(inode);
> +	dput(result);
> +	return dentry;
> +}
> +
> +

It seems like it might be nice to make the CI code a compile-time
option?  Fairly big new chunks... if it could be done cleanly, might be
nice...

> ===========================================================================
> fs/xfs/linux-2.6/xfs_linux.h
> ===========================================================================
> 
> --- a/fs/xfs/linux-2.6/xfs_linux.h	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/linux-2.6/xfs_linux.h	2008-01-11 14:49:16.537591564 +1100
> @@ -75,6 +75,8 @@
>  #include <linux/delay.h>
>  #include <linux/log2.h>
>  #include <linux/spinlock.h>
> +#include <linux/ctype.h>
> +#include <linux/nls.h>
>  
>  #include <asm/page.h>
>  #include <asm/div64.h>
> @@ -180,6 +182,12 @@
>  #define howmany(x, y)	(((x)+((y)-1))/(y))
>  
>  /*
> + * NLS UTF-8 character set
> + */
> +
> +#define XFS_NLS_UTF8	"utf8"

I guess that had to go somewhere? :)

> +
> +/*
>   * Various platform dependent calls that don't fit anywhere else
>   */
>  #define xfs_sort(a,n,s,fn)	sort(a,n,s,fn,NULL)
> 
> ===========================================================================
> fs/xfs/linux-2.6/xfs_super.c
> ===========================================================================
> 
> --- a/fs/xfs/linux-2.6/xfs_super.c	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/linux-2.6/xfs_super.c	2008-01-11 14:46:25.067566854 +1100
> @@ -50,6 +50,7 @@
>  #include "xfs_vnodeops.h"
>  #include "xfs_vfsops.h"
>  #include "xfs_version.h"
> +#include "xfs_unicode.h"
>  #include "xfs_log_priv.h"
>  
>  #include <linux/namei.h>
> @@ -121,6 +122,9 @@ xfs_args_allocate(
>  #define MNTOPT_ATTR2	"attr2"		/* do use attr2 attribute format */
>  #define MNTOPT_NOATTR2	"noattr2"	/* do not use attr2 attribute format */
>  #define MNTOPT_FILESTREAM  "filestreams" /* use filestreams allocator */
> +#define MNTOPT_NLS	"nls"		/* NLS code page to use */
> +#define MNTOPT_CILOOKUP	"ci"		/* case-insensitive dir names */
> +#define MNTOPT_CIATTR	"ciattr"	/* case-insensitive attr names */
>  #define MNTOPT_QUOTA	"quota"		/* disk quotas (user) */
>  #define MNTOPT_NOQUOTA	"noquota"	/* no quotas */
>  #define MNTOPT_USRQUOTA	"usrquota"	/* user quota enabled */


Please document in Documentation/filesystems/xfs.txt too...

> @@ -315,6 +319,18 @@ xfs_parseargs(
>  			args->flags &= ~XFSMNT_ATTR2;
>  		} else if (!strcmp(this_char, MNTOPT_FILESTREAM)) {
>  			args->flags2 |= XFSMNT2_FILESTREAMS;
> +		} else if (!strcmp(this_char, MNTOPT_NLS)) {
> +			if (!value || !*value) {
> +				cmn_err(CE_WARN,
> +					"XFS: %s option requires an argument",
> +					this_char);
> +				return EINVAL;
> +			}
> +			strncpy(args->nls, value, MAXNAMELEN);
> +		} else if (!strcmp(this_char, MNTOPT_CILOOKUP)) {
> +			args->flags2 |= XFSMNT2_CILOOKUP;
> +		} else if (!strcmp(this_char, MNTOPT_CIATTR)) {
> +			args->flags2 |= XFSMNT2_CIATTR;
>  		} else if (!strcmp(this_char, MNTOPT_NOQUOTA)) {
>  			args->flags &= ~(XFSMNT_UQUOTAENF|XFSMNT_UQUOTA);
>  			args->flags &= ~(XFSMNT_GQUOTAENF|XFSMNT_GQUOTA);
> @@ -454,6 +470,8 @@ xfs_showargs(
>  		{ XFS_MOUNT_OSYNCISOSYNC,	"," MNTOPT_OSYNCISOSYNC },
>  		{ XFS_MOUNT_ATTR2,		"," MNTOPT_ATTR2 },
>  		{ XFS_MOUNT_FILESTREAMS,	"," MNTOPT_FILESTREAM },
> +		{ XFS_MOUNT_CI_LOOKUP,		"," MNTOPT_CILOOKUP },
> +		{ XFS_MOUNT_CI_ATTR,		"," MNTOPT_CIATTR },
>  		{ XFS_MOUNT_DMAPI,		"," MNTOPT_DMAPI },
>  		{ XFS_MOUNT_GRPID,		"," MNTOPT_GRPID },
>  		{ 0, NULL }
> @@ -516,6 +534,13 @@ xfs_showargs(
>  	if (!(mp->m_qflags & XFS_ALL_QUOTA_ACCT))
>  		seq_puts(m, "," MNTOPT_NOQUOTA);
>  
> +	if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +		if (mp->m_nls)
> +			seq_printf(m, "," MNTOPT_NLS "=%s", mp->m_nls->charset);
> +		else
> +			seq_puts(m, "," MNTOPT_NLS "=" XFS_NLS_UTF8);
> +	}
> +
>  	return 0;
>  }
>  __uint64_t
> @@ -563,7 +588,11 @@ 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_I(inode)->i_mount->m_flags & XFS_MOUNT_CI_LOOKUP) ?
> +				&xfs_dir_ci_inode_operations :
> +				&xfs_dir_inode_operations;

Do any linux filesystems in existence actually have
XFS_SB_VERSION_OLDCIBIT?  I'd think not - this patch is *adding* that
macro - what's this for?

>  		inode->i_fop = &xfs_dir_file_operations;
>  		break;
>  	case S_IFLNK:
> 
> ===========================================================================
> fs/xfs/xfs_attr.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_attr.c	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_attr.c	2008-01-18 13:25:20.068339942 +1100
> @@ -106,6 +106,17 @@ ktrace_t *xfs_attr_trace_buf;
>   * Overall external interface routines.
>   *========================================================================*/
>  
> +void
> +xfs_attr_mount(struct xfs_mount *mp)
> +{
> +	mp->m_attr_magicpct = (mp->m_sb.sb_blocksize * 37) / 100;
> +	if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +		mp->m_attrnameops = (mp->m_flags & XFS_MOUNT_CI_ATTR) ?
> +			&xfs_unicode_ci_nameops : &xfs_unicode_nameops;
> +	} else
> +		mp->m_attrnameops = &xfs_default_nameops;
> +}

Hm, I thought most little mount-helper subroutines went right in next to
xfs_mountfs() in xfs_mount.c; just a thought.  Then you wouldn't need
the prototype in the header either...

> +
>  int
>  xfs_attr_fetch(xfs_inode_t *ip, const char *name, int namelen,
>  	       char *value, int *valuelenp, int flags, struct cred *cred)
> @@ -122,14 +133,14 @@ xfs_attr_fetch(xfs_inode_t *ip, const ch
>  	 * Fill in the arg structure for this request.
>  	 */
>  	memset((char *)&args, 0, sizeof(args));
> -	args.name = name;
> -	args.namelen = namelen;
>  	args.value = value;
>  	args.valuelen = *valuelenp;
>  	args.flags = flags;
> -	args.hashval = xfs_da_hashname(args.name, args.namelen);
>  	args.dp = ip;
>  	args.whichfork = XFS_ATTR_FORK;
> +	error = xfs_da_setup_name_and_hash(&args, name, namelen);
> +	if (error)
> +		return error;

In the spirit of incremental-but-functional patches, which helps to
break down & review big patchsets like this, I think this addition of
xfs_da_setup_name_and_hash() could stand on its own, and add the nls
stuff in a subsequent patch?

>  	/*
>  	 * Decide on what work routines to call based on the inode size.
> @@ -153,6 +164,7 @@ xfs_attr_fetch(xfs_inode_t *ip, const ch
>  
>  	if (error == EEXIST)
>  		error = 0;
> +	xfs_da_cleanup_name(&args, name);

I'm being a little lazy here; under which circumstances would args->name
!= name, and need to be freed?

...

> ===========================================================================
> fs/xfs/xfs_attr_leaf.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_attr_leaf.c	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_attr_leaf.c	2008-01-18 13:25:11.873394723 +1100
> @@ -42,6 +42,7 @@
>  #include "xfs_attr.h"
>  #include "xfs_attr_leaf.h"
>  #include "xfs_error.h"
> +#include "xfs_unicode.h"
>  
>  /*
>   * xfs_attr_leaf.c
> @@ -90,6 +91,10 @@ STATIC void xfs_attr_leaf_moveents(xfs_a
>  					 xfs_mount_t *mp);
>  STATIC int xfs_attr_leaf_entsize(xfs_attr_leafblock_t *leaf, int index);
>  
> +STATIC int xfs_attr_put_listent(xfs_attr_list_context_t *context,
> +					attrnames_t *namesp, char *name,
> +					int namelen, int valuelen, char *value);
> +
>  /*========================================================================
>   * Namespace helper routines
>   *========================================================================*/
> @@ -135,6 +140,38 @@ xfs_attr_namesp_match_overrides(int arg_
>   * External routines when attribute fork size < XFS_LITINO(mp).
>   *========================================================================*/
>  
> +STATIC xfs_attr_sf_entry_t *
> +xfs_attr_shortform_find_ent(xfs_da_args_t *args)
> +{
> +	xfs_attr_shortform_t *sf;
> +	xfs_attr_sf_entry_t *sfe;
> +	int i;
> +	xfs_attr_sf_entry_t *ci_sfe = NULL;
> +
> +	ASSERT(args->dp->i_afp->if_flags & XFS_IFINLINE);
> +	sf = (xfs_attr_shortform_t *)args->dp->i_afp->if_u1.if_data;
> +	sfe = &sf->list[0];
> +
> +	args->cmpresult = XFS_CMP_DIFFERENT;
> +	for (i = 0; i < sf->hdr.count; sfe = XFS_ATTR_SF_NEXTENTRY(sfe), i++) {
> +		if (!xfs_attr_namesp_match(args->flags, sfe->flags))
> +			continue;
> +		switch (xfs_attr_compname(args->dp, sfe->nameval, sfe->namelen,
> +				args->name, args->namelen)) {
> +		case XFS_CMP_EXACT:
> +			args->cmpresult = XFS_CMP_EXACT;
> +			return sfe;
> +		case XFS_CMP_CASE:
> +			if (!ci_sfe) {
> +				args->cmpresult = XFS_CMP_CASE;
> +				ci_sfe = sfe;
> +			}
> +		default:;
> +		}
> +	}
> +	return ci_sfe;
> +}

Perhaps this helper could be a sub-patch too?  Just a thought.

>  /*
>   * Query whether the requested number of additional bytes of extended
>   * attribute space will be able to fit inline.
> @@ -295,13 +332,10 @@ xfs_attr_shortform_add(xfs_da_args_t *ar
>  	sfe = &sf->list[0];
>  	for (i = 0; i < sf->hdr.count; sfe = XFS_ATTR_SF_NEXTENTRY(sfe), i++) {
>  #ifdef DEBUG
> -		if (sfe->namelen != args->namelen)
> -			continue;
> -		if (memcmp(args->name, sfe->nameval, args->namelen) != 0)
> -			continue;
>  		if (!xfs_attr_namesp_match(args->flags, sfe->flags))
>  			continue;
> -		ASSERT(0);
> +		ASSERT(xfs_attr_compname(args->dp, args->name, args->namelen,
> +			sfe->nameval, sfe->namelen) == XFS_CMP_DIFFERENT);

Perhaps this helper too could come as a subpatch.

...


> +	if (!sfe)
> +		return XFS_ERROR(ENOATTR);
> +

Indentation from here on looks busted.  Are you missing braces?

>  		if (args->flags & ATTR_KERNOVAL) {
>  			args->valuelen = sfe->valuelen;
> -			return(XFS_ERROR(EEXIST));
> +		return XFS_ERROR(EEXIST);

need another tab here?

>  		}
>  		if (args->valuelen < sfe->valuelen) {
>  			args->valuelen = sfe->valuelen;
> -			return(XFS_ERROR(ERANGE));
> +		return XFS_ERROR(ERANGE);

and here?

>  		}
>  		args->valuelen = sfe->valuelen;
> -		memcpy(args->value, &sfe->nameval[args->namelen],
> -						    args->valuelen);
> -		return(XFS_ERROR(EEXIST));
> -	}
> -	return(XFS_ERROR(ENOATTR));
> +	memcpy(args->value, &sfe->nameval[args->namelen], args->valuelen);
> +
> +	return XFS_ERROR(EEXIST);
>  }
>  
>  /*
...


> @@ -631,7 +626,7 @@ xfs_attr_shortform_list(xfs_attr_list_co
>  				continue;
>  			}
>  			namesp = xfs_attr_flags_namesp(sfe->flags);
> -			error = context->put_listent(context,
> +			error = xfs_attr_put_listent(context,
>  					   namesp,
>  					   (char *)sfe->nameval,
>  					   (int)sfe->namelen,
> @@ -734,7 +729,7 @@ xfs_attr_shortform_list(xfs_attr_list_co
>  			cursor->hashval = sbp->hash;
>  			cursor->offset = 0;
>  		}
> -		error = context->put_listent(context,
> +		error = xfs_attr_put_listent(context,
>  					namesp,
>  					sbp->name,
>  					sbp->namelen,

maybe the introduction of xfs_attr_put_listent could be a small
prep-work patch too.  Or is this getting old ;)

> @@ -1960,6 +1955,7 @@ xfs_attr_leaf_lookup_int(xfs_dabuf_t *bp
>  	xfs_attr_leaf_name_remote_t *name_rmt;
>  	int probe, span;
>  	xfs_dahash_t hashval;
> +	xfs_dacmp_t cmp;
>  
>  	leaf = bp->data;
>  	ASSERT(be16_to_cpu(leaf->hdr.info.magic) == XFS_ATTR_LEAF_MAGIC);
> @@ -2008,6 +2004,7 @@ xfs_attr_leaf_lookup_int(xfs_dabuf_t *bp
>  	/*
>  	 * Duplicate keys may be present, so search all of them for a match.
>  	 */
> +	args->cmpresult = XFS_CMP_DIFFERENT;
>  	for (  ; (probe < be16_to_cpu(leaf->hdr.count)) &&
>  			(be32_to_cpu(entry->hashval) == hashval);
>  			entry++, probe++) {
> @@ -2019,35 +2016,40 @@ xfs_attr_leaf_lookup_int(xfs_dabuf_t *bp
>  		 * If we are looking for complete entries, show only those.
>  		 */
>  		if ((args->flags & XFS_ATTR_INCOMPLETE) !=
> -		    (entry->flags & XFS_ATTR_INCOMPLETE)) {
> -			continue;
> -		}
> -		if (entry->flags & XFS_ATTR_LOCAL) {
> -			name_loc = XFS_ATTR_LEAF_NAME_LOCAL(leaf, probe);
> -			if (name_loc->namelen != args->namelen)
> -				continue;
> -			if (memcmp(args->name, (char *)name_loc->nameval, args->namelen) != 0)
> +				(entry->flags & XFS_ATTR_INCOMPLETE))
>  				continue;
>  			if (!xfs_attr_namesp_match(args->flags, entry->flags))
>  				continue;
> +		if (entry->flags & XFS_ATTR_LOCAL) {
> +			name_loc = XFS_ATTR_LEAF_NAME_LOCAL(leaf, probe);
> +			cmp = xfs_attr_compname(args->dp, args->name, args->namelen,
> +					name_loc->nameval, name_loc->namelen);
> +			if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> +				args->cmpresult = cmp;
>  			args->index = probe;

weird indentation here too...

> -			return(XFS_ERROR(EEXIST));
> +				args->rmtblkno = 0;
> +				args->rmtblkcnt = 0;
> +				if (cmp == XFS_CMP_EXACT)
> +					return XFS_ERROR(EEXIST);
> +			}
>  		} else {
>  			name_rmt = XFS_ATTR_LEAF_NAME_REMOTE(leaf, probe);
> -			if (name_rmt->namelen != args->namelen)
> -				continue;
> -			if (memcmp(args->name, (char *)name_rmt->name,
> -					     args->namelen) != 0)
> -				continue;
> -			if (!xfs_attr_namesp_match(args->flags, entry->flags))
> -				continue;
> +			cmp = xfs_attr_compname(args->dp, args->name, args->namelen,
> +					name_rmt->name, name_rmt->namelen);
> +			if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> +				args->cmpresult = cmp;
>  			args->index = probe;
>  			args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
>  			args->rmtblkcnt = XFS_B_TO_FSB(args->dp->i_mount,
>  						   be32_to_cpu(name_rmt->valuelen));
> -			return(XFS_ERROR(EEXIST));
> +				if (cmp == XFS_CMP_EXACT)
> +					return XFS_ERROR(EEXIST);

and here

> +			}
>  		}
>  	}
> +	if (args->cmpresult == XFS_CMP_CASE)
> +		return XFS_ERROR(EEXIST);
> +
>  	args->index = probe;
>  	return(XFS_ERROR(ENOATTR));
>  }
> @@ -2418,7 +2420,7 @@ xfs_attr_leaf_list_int(xfs_dabuf_t *bp, 
>  			xfs_attr_leaf_name_local_t *name_loc =
>  				XFS_ATTR_LEAF_NAME_LOCAL(leaf, i);
>  
> -			retval = context->put_listent(context,
> +			retval = xfs_attr_put_listent(context,

sub-patch? :)

...				(int)name_rmt->namelen,
> @@ -2472,6 +2474,31 @@ xfs_attr_leaf_list_int(xfs_dabuf_t *bp, 
>  	return(retval);
>  }
>  
> +/*
> + * Do NLS name conversion if required for attribute name and call
> + * context's put_listent routine
> + */
> +
> +STATIC int
> +xfs_attr_put_listent(xfs_attr_list_context_t *context, attrnames_t *namesp,
> +	char *name, int namelen, int valuelen, char *value)
> +{
> +	xfs_mount_t *mp = context->dp->i_mount;
> +	char *nls_name = NULL;
> +	int rval;
> +
> +	if (!mp->m_nls)
> +		return context->put_listent(context, namesp, name, namelen,
> +				valuelen, value);
> +
> +	rval = xfs_unicode_to_nls(mp->m_nls, name, namelen, &nls_name);
> +	if (rval < 0)
> +		return -rval;
> +	rval = context->put_listent(context, namesp, nls_name, rval,
> +			valuelen, value);
> +	xfs_free_unicode_nls_name(nls_name);
> +	return rval;
> +}
>  
>  /*========================================================================
>   * Manage the INCOMPLETE flag in a leaf entry
> ===========================================================================
> fs/xfs/xfs_attr_leaf.h
> ===========================================================================
> 
> --- a/fs/xfs/xfs_attr_leaf.h	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_attr_leaf.h	2008-01-11 14:16:44.268796245 +1100
> @@ -36,6 +36,7 @@ struct xfs_da_args;
>  struct xfs_da_state;
>  struct xfs_da_state_blk;
>  struct xfs_inode;
> +struct xfs_mount;
>  struct xfs_trans;
>  
>  /*========================================================================
> @@ -204,6 +205,16 @@ static inline int xfs_attr_leaf_entsize_
>  	return (((bsize) >> 1) + ((bsize) >> 2));
>  }
>  
> +/*
> + * Do hash and name compare based on nameops
> + */
> +#define xfs_attr_hashname(dp, n, l) \
> +		((dp)->i_mount->m_attrnameops->hashname((dp), (n), (l)))
> +
> +#define xfs_attr_compname(dp, n1, l1, n2, l2) \
> +		((dp)->i_mount->m_attrnameops->compname((dp), (n1), (l1), \
> +							(n2), (l2)))
> +
>  
>  /*========================================================================
>   * Structure used to pass context around among the routines.
> @@ -296,6 +307,7 @@ int	xfs_attr_root_inactive(struct xfs_tr
>  /*
>   * Utility routines.
>   */
> +void	xfs_attr_mount(struct xfs_mount *mp);

this could just go in xfs_mount.c and be static.

>  xfs_dahash_t	xfs_attr_leaf_lasthash(struct xfs_dabuf *bp, int *count);
>  int	xfs_attr_leaf_order(struct xfs_dabuf *leaf1_bp,
>  				   struct xfs_dabuf *leaf2_bp);
> 

> ===========================================================================
> fs/xfs/xfs_da_btree.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_da_btree.c	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_da_btree.c	2007-10-31 16:04:16.463309546 +1100
> @@ -46,6 +46,7 @@
>  #include "xfs_dir2_block.h"
>  #include "xfs_dir2_node.h"
>  #include "xfs_error.h"
> +#include "xfs_unicode.h"
>  
>  /*
>   * xfs_da_btree.c
> @@ -1530,6 +1531,100 @@ xfs_da_hashname(const uchar_t *name, int
>  	}
>  }
>  
> +
> +static xfs_dahash_t
> +xfs_default_hashname(xfs_inode_t *inode, const uchar_t *name, int namelen)
> +{
> +	return xfs_da_hashname(name, namelen);
> +}

Could these be rolled into one instead of the wrapper?  I see 3 other
direct callers of xfs_da_hashname... and does xfs_attr_shortform_list
need to use xfs_attr_hashname instead of xfs_da_hashname?  Do "." and
".." ever have different answers in different codepages?  Does that matter?

> +xfs_dacmp_t
> +xfs_default_compname(xfs_inode_t *inode, 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;
> +}
> +
> +static xfs_dahash_t
> +xfs_unicode_ci_hashname(
> +	xfs_inode_t	*inode,
> +	const uchar_t	*name,
> +	int		namelen)
> +{
> +	return xfs_unicode_hash(inode->i_mount->m_cft, name, namelen);
> +}

this wrapper is the only caller of xfs_unicode_hash?  Is it just to
conveniently get from inode to m_cft?

strikes me as a little interesting that while an inode is passed into a
xfs_hashname_t, it's never actually used directly - same for compname -
would it make any sense to just pass the xfs_cft in from the start?

...

> ===========================================================================
> fs/xfs/xfs_dir2.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2.c	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_dir2.c	2008-01-11 14:24:51.701973714 +1100
> @@ -42,8 +42,56 @@
>  #include "xfs_dir2_node.h"
>  #include "xfs_dir2_trace.h"
>  #include "xfs_error.h"
> +#include "xfs_unicode.h"
>  #include "xfs_vnodeops.h"
>  
> +/*
> + * V1 case-insensitive support for directories
> + */

remind me, what's "V1?"

> +static xfs_dahash_t
> +xfs_ascii_ci_hashname(
> +	xfs_inode_t	*inode,
> +	const uchar_t	*name,
> +	int		namelen)
> +{
> +	xfs_dahash_t	hash;
> +	int		i;
> +
> +	for (i = 0, hash = 0; i < namelen; i++)
> +		hash = tolower(name[i]) ^ rol32(hash, 7);
> +
> +	return hash;
> +}
> +
> +static xfs_dacmp_t
> +xfs_ascii_ci_compname(
> +	xfs_inode_t	*inode,
> +	const uchar_t	*name1,
> +	int		len1,
> +	const uchar_t	*name2,
> +	int 		len2)
> +{
> +	xfs_dacmp_t	result = XFS_CMP_EXACT;
> +	int		i;
> +
> +	if (len1 != len2)
> +		return XFS_CMP_DIFFERENT;
> +
> +	for (i = 0; i < len1; i++) {
> +		if (name1[i] == name2[i])
> +			continue;
> +		if (tolower(name1[i]) != tolower(name2[i]))
> +			return XFS_CMP_DIFFERENT;
> +		result = XFS_CMP_CASE;
> +	}
> +
> +	return result;
> +}
> +
> +static const struct xfs_nameops xfs_ascii_ci_nameops = {
> +	.hashname	= xfs_ascii_ci_hashname,
> +	.compname	= xfs_ascii_ci_compname,
> +};
>  
>  void
>  xfs_dir_mount(
> @@ -64,6 +112,13 @@ xfs_dir_mount(
>  		(mp->m_dirblksize - (uint)sizeof(xfs_da_node_hdr_t)) /
>  		(uint)sizeof(xfs_da_node_entry_t);
>  	mp->m_dir_magicpct = (mp->m_dirblksize * 37) / 100;
> +
> +	if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +		mp->m_dirnameops = (mp->m_flags & XFS_MOUNT_CI_LOOKUP) ?
> +			&xfs_unicode_ci_nameops : &xfs_unicode_nameops;
> +	} else
> +		mp->m_dirnameops = (xfs_sb_version_hasoldci(&mp->m_sb)) ?
> +			&xfs_ascii_ci_nameops : &xfs_default_nameops;

Oh, "V1" is oldci - can you document it that way in the comment?  And if
that hasn't been seen in the wild on linux can this go away?

...

> ===========================================================================
> fs/xfs/xfs_dir2_block.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2_block.c	2008-01-18 15:31:24.000000000 +1100
> +++ b/fs/xfs/xfs_dir2_block.c	2008-01-11 14:28:44.763934272 +1100
...

> @@ -697,20 +720,34 @@ xfs_dir2_block_lookup_int(
>  		dep = (xfs_dir2_data_entry_t *)
>  			((char *)block + xfs_dir2_dataptr_to_off(mp, addr));
>  		/*
> -		 * Compare, if it's right give back buffer & entry number.
> -		 */
> -		if (dep->namelen == args->namelen &&
> -		    dep->name[0] == args->name[0] &&
> -		    memcmp(dep->name, args->name, args->namelen) == 0) {
> +		 * 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
> +		 */
> +		cmp = args->oknoent ?
> +			xfs_dir_compname(dp, dep->name, dep->namelen,
> +						args->name, args->namelen) :
> +			xfs_default_compname(dp, dep->name, dep->namelen,
> +						args->name, args->namelen);
> +		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> +			args->cmpresult = cmp;
>  			*bpp = bp;
>  			*entno = mid;
> +			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);

thanks... can you trim down the other > 80 char lines you've added, too? :)

> @@ -1300,10 +1313,17 @@ xfs_dir2_leaf_lookup(
>  	/*
>  	 * Return the found inode number.
>  	 */
> +	error = EEXIST;
>  	args->inumber = be64_to_cpu(dep->inumber);
> +	if (args->cmpresult == XFS_CMP_CASE) {
> +		args->valuelen = xfs_unicode_to_nls(args->dp->i_mount->m_nls,
> +				dep->name, dep->namelen, (char **)&args->value);
> +		if (args->valuelen < 0)
> +			error = -args->valuelen;

hm, error signs... new functions return negative, whereas old xfs code
returns positive errors?

...

> @@ -1391,19 +1413,31 @@ xfs_dir2_leaf_lookup_int(
>  		       xfs_dir2_dataptr_to_off(mp, be32_to_cpu(lep->address)));
>  		/*
>  		 * If it matches then return it.
> -		 */
> -		if (dep->namelen == args->namelen &&
> -		    dep->name[0] == args->name[0] &&
> -		    memcmp(dep->name, args->name, args->namelen) == 0) {
> +		 *
> +		 * lookup case - use nameops;
> +		 *
> +		 * replace/remove case - as lookup has been already been
> +		 * performed, look for an exact match using the fast method
> +		 */
> +		cmp = args->oknoent ?
> +			xfs_dir_compname(dp, dep->name, dep->namelen,
> +						args->name, args->namelen) :
> +			xfs_default_compname(dp, dep->name, dep->namelen,
> +						args->name, args->namelen);
> +		if (cmp != XFS_CMP_DIFFERENT && cmp != args->cmpresult) {
> +			args->cmpresult = cmp;
>  			*dbpp = dbp;
>  			*indexp = index;
> +			if (cmp == XFS_CMP_EXACT)
>  			return 0;

tab missing?


> ===========================================================================
> fs/xfs/xfs_dir2_node.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2_node.c	2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_dir2_node.c	2007-10-31 12:32:04.060201390 +1100
...

> @@ -572,28 +575,34 @@ xfs_dir2_leafn_lookup_int(
>  			/*
>  			 * Point to the data entry.
>  			 */
> -			dep = (xfs_dir2_data_entry_t *)
> -			      ((char *)curbp->data +
> +			dep = (xfs_dir2_data_entry_t *)((char *)curbp->data +
>  			       xfs_dir2_dataptr_to_off(mp, be32_to_cpu(lep->address)));
>  			/*
>  			 * 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_default_compname(dp, dep->name, dep->namelen,
> +						args->name, args->namelen);
> +			if (cmp != XFS_CMP_DIFFERENT &&
> +					cmp != args->cmpresult) {
> +				args->cmpresult = cmp;
>  				args->inumber = be64_to_cpu(dep->inumber);
>  				*indexp = index;
>  				state->extravalid = 1;
>  				state->extrablk.bp = curbp;
>  				state->extrablk.blkno = curdb;
> -				state->extrablk.index =
> -					(int)((char *)dep -
> +				state->extrablk.index = (int)((char *)dep -
>  					      (char *)curbp->data);
>  				state->extrablk.magic = XFS_DIR2_DATA_MAGIC;
> +				if (cmp == XFS_CMP_EXACT)
>  				return XFS_ERROR(EEXIST);

tab...  Hmm wonder if I caught all of these...

> ===========================================================================
> fs/xfs/xfs_dir2_sf.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_dir2_sf.c	2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_dir2_sf.c	2008-01-17 12:25:01.552398622 +1100
> @@ -38,6 +38,7 @@
>  #include "xfs_dir2_leaf.h"
>  #include "xfs_dir2_block.h"
>  #include "xfs_dir2_trace.h"
> +#include "xfs_unicode.h"
>  
>  /*
>   * Prototypes for internal functions.
> @@ -708,6 +709,8 @@ xfs_dir2_sf_getdents(
>  	xfs_dir2_dataptr_t	dot_offset;
>  	xfs_dir2_dataptr_t	dotdot_offset;
>  	xfs_ino_t		ino;
> +	char			*nls_name = NULL; /* NLS name buffer */
> +	int			nls_namelen = 0;
>  
>  	mp = dp->i_mount;
>  
> @@ -772,6 +775,9 @@ xfs_dir2_sf_getdents(
>  		}
>  	}
>  
> +	if (mp->m_nls)
> +		nls_name = xfs_alloc_unicode_nls_name();
> +
>  	/*
>  	 * Loop while there are more entries and put'ing works.
>  	 */
> @@ -789,16 +795,22 @@ xfs_dir2_sf_getdents(
>  #if XFS_BIG_INUMS
>  		ino += mp->m_inoadd;
>  #endif
> -
> -		if (filldir(dirent, sfep->name, sfep->namelen,
> +		if (mp->m_nls)
> +			nls_namelen = xfs_unicode_to_nls(mp->m_nls, sfep->name,
> +					sfep->namelen, &nls_name);
> +		if (filldir(dirent,
> +				nls_namelen > 0 ? nls_name : (char *)sfep->name,
> +				nls_namelen > 0 ? nls_namelen : sfep->namelen,
>  					    off, ino, DT_UNKNOWN)) {

Hmm we've seen the foo > 0 ? bar : baz stuff a few times, should this
get a helper?

...

> @@ -844,23 +857,43 @@ xfs_dir2_sf_lookup(
>  	if (args->namelen == 2 &&
>  	    args->name[0] == '.' && args->name[1] == '.') {
>  		args->inumber = xfs_dir2_sf_get_inumber(sfp, &sfp->hdr.parent);
> +		args->cmpresult = XFS_CMP_EXACT;
>  		return XFS_ERROR(EEXIST);
>  	}
>  	/*
>  	 * Loop over all the entries trying to match ours.
>  	 */
> -	for (i = 0, sfep = xfs_dir2_sf_firstentry(sfp);
> -	     i < sfp->hdr.count;
> +	args->cmpresult = XFS_CMP_DIFFERENT;
> +	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) {
> -			args->inumber =
> -				xfs_dir2_sf_get_inumber(sfp,
> +		switch (xfs_dir_compname(dp, sfep->name, sfep->namelen,
> +				args->name, args->namelen)) {
> +		case XFS_CMP_EXACT:
> +			args->cmpresult = XFS_CMP_EXACT;
> +			args->inumber = xfs_dir2_sf_get_inumber(sfp,
>  					xfs_dir2_sf_inumberp(sfep));
> +			if (args->value) {
> +				xfs_free_unicode_nls_name(args->value);
> +				args->value = NULL;
> +			}
>  			return XFS_ERROR(EEXIST);
> +
> +		case XFS_CMP_CASE:
> +			if (!args->value) {
> +				args->valuelen = xfs_unicode_to_nls(
> +					args->dp->i_mount->m_nls, sfep->name,
> +					sfep->namelen, (char **)&args->value);
> +				if (args->valuelen < 0)
> +					return XFS_ERROR(-args->valuelen);
> +				args->cmpresult = XFS_CMP_CASE;
> +				args->inumber = xfs_dir2_sf_get_inumber(sfp,
> +						xfs_dir2_sf_inumberp(sfep));
> +			}
> +		default: ;

Hmm that's a little funky, to my eyes anyway.

...

> ===========================================================================
> fs/xfs/xfs_mount.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_mount.c	2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_mount.c	2008-01-17 17:10:29.777728874 +1100
> @@ -25,6 +25,7 @@
>  #include "xfs_sb.h"
>  #include "xfs_ag.h"
>  #include "xfs_dir2.h"
> +#include "xfs_attr.h"
>  #include "xfs_dmapi.h"
>  #include "xfs_mount.h"
>  #include "xfs_bmap_btree.h"
> @@ -43,6 +44,9 @@
>  #include "xfs_rw.h"
>  #include "xfs_quota.h"
>  #include "xfs_fsops.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_attr_leaf.h"
> +#include "xfs_unicode.h"
>  
>  STATIC void	xfs_mount_log_sbunit(xfs_mount_t *, __int64_t);
>  STATIC int	xfs_uuid_mount(xfs_mount_t *);
> @@ -119,6 +123,8 @@ static const struct {
>      { offsetof(xfs_sb_t, sb_logsectsize),0 },
>      { offsetof(xfs_sb_t, sb_logsunit),	 0 },
>      { offsetof(xfs_sb_t, sb_features2),	 0 },
> +    { offsetof(xfs_sb_t, sb_bad_features2), 0 },

bad_features2?  what's this and what does it have to do w/ CI, and why
is it set but never used in xfs_sb_from_disk?

if features2 "could be here" shouldn't we be doing something with that?
 This could do with comments at least, somewhere.

...

> @@ -1165,6 +1176,17 @@ xfs_mountfs(
>  	}
>  
>  	/*
> +	 * Load in unicode case folding table from disk
> +	 */
> +	if (xfs_sb_version_hasunicode(&mp->m_sb)) {
> +		error = xfs_unicode_read_cft(mp);
> +		if (error) {
> +			cmn_err(CE_WARN, "XFS: failed to read case folding table");

80+ chars...

..

> ===========================================================================
> fs/xfs/xfs_sb.h
> ===========================================================================
> 
> --- a/fs/xfs/xfs_sb.h	2008-01-18 15:31:25.000000000 +1100
> +++ b/fs/xfs/xfs_sb.h	2007-10-23 16:55:47.440178601 +1000
> @@ -46,10 +46,12 @@ struct xfs_mount;
>  #define XFS_SB_VERSION_SECTORBIT	0x0800
>  #define	XFS_SB_VERSION_EXTFLGBIT	0x1000
>  #define	XFS_SB_VERSION_DIRV2BIT		0x2000
> +#define XFS_SB_VERSION_OLDCIBIT		0x4000
>  #define	XFS_SB_VERSION_MOREBITSBIT	0x8000
>  #define	XFS_SB_VERSION_OKSASHFBITS	\
>  	(XFS_SB_VERSION_EXTFLGBIT | \
> -	 XFS_SB_VERSION_DIRV2BIT)
> +	 XFS_SB_VERSION_DIRV2BIT | \
> +	 XFS_SB_VERSION_OLDCIBIT)

there's that oldcibit again...

>  #define	XFS_SB_VERSION_OKREALFBITS	\
>  	(XFS_SB_VERSION_ATTRBIT | \
>  	 XFS_SB_VERSION_NLINKBIT | \
> @@ -77,10 +79,12 @@ struct xfs_mount;
>  #define XFS_SB_VERSION2_LAZYSBCOUNTBIT	0x00000002	/* Superblk counters */
>  #define XFS_SB_VERSION2_RESERVED4BIT	0x00000004
>  #define XFS_SB_VERSION2_ATTR2BIT	0x00000008	/* Inline attr rework */
> +#define XFS_SB_VERSION2_UNICODEBIT	0x00000020	/* Unicode names */
>  
>  #define	XFS_SB_VERSION2_OKREALFBITS	\
>  	(XFS_SB_VERSION2_LAZYSBCOUNTBIT	| \
> -	 XFS_SB_VERSION2_ATTR2BIT)
> +	 XFS_SB_VERSION2_ATTR2BIT | \
> +	 XFS_SB_VERSION2_UNICODEBIT)
>  #define	XFS_SB_VERSION2_OKSASHFBITS	\
>  	(0)
>  #define XFS_SB_VERSION2_OKREALBITS	\
> @@ -145,6 +149,9 @@ typedef struct xfs_sb {
>  	__uint16_t	sb_logsectsize;	/* sector size for the log, bytes */
>  	__uint32_t	sb_logsunit;	/* stripe unit size for the log */
>  	__uint32_t	sb_features2;	/* additional feature bits */
> +	__uint32_t	sb_bad_features2; /* features2 could be here */

Ok, so...?

...

> @@ -463,6 +475,12 @@ static inline int xfs_sb_version_hassect
>  		((sbp)->sb_versionnum & XFS_SB_VERSION_SECTORBIT);
>  }
>  
> +static inline int xfs_sb_version_hasoldci(xfs_sb_t *sbp)
> +{
> +	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4) && \
> +		((sbp)->sb_versionnum & XFS_SB_VERSION_OLDCIBIT);
> +}

You forgot the uppercase indirection macro!   ;)

...


> ===========================================================================
> fs/xfs/xfs_unicode.c
> ===========================================================================
...

> +
> +char *
> +xfs_alloc_unicode_nls_name(void)
> +{
> +	return kmem_zone_alloc(xfs_nls_uni_zone, KM_SLEEP);
> +}

Why wrap/hide this?

> +
> +
> +void
> +xfs_free_unicode_nls_name(
> +	char		*name)
> +{
> +	kmem_zone_free(xfs_nls_uni_zone, name);
> +}

Or this?  Eh, maybe it's handy.

> +int
> +xfs_nls_to_unicode(
> +	struct nls_table *nls,
> +	const char	*nls_name,
> +	int		nls_namelen,
> +	char		**uni_name)
> +{
> +	char		*n;
> +	int		i, o;
> +	wchar_t		uc;
> +	int		nlen;
> +	int		u8len;
> +	int		rval;
> +
> +	n = *uni_name ? *uni_name : xfs_alloc_unicode_nls_name();
> +
> +	if (!nls) {
> +		if (nls_namelen > MAXNAMELEN) {
> +			rval = -ENAMETOOLONG;

The rest of core xfs code returns positive errors; why the shift in this
file?  Well, I guess because you want to return a length, but this
strikes me as a bit inconsistent... we've been burned by getting error
signs wrong in the past, this looks like an exception to the existing
sign conventions

...


> +int
> +xfs_unicode_validate(
> +	const uchar_t	*name,
> +	int		namelen)
> +{
> +	wchar_t		uc;
> +	int		i, nlen;
> +
> +	for (i = 0; i < namelen; i += nlen) {
> +		if (*name >= 0xf0) {
> +			cmn_err(CE_WARN, "xfs_unicode_validate: "
> +					"UTF-8 char beyond U+FFFF\n");
> +			return -EINVAL;
> +		}
> +		/* utf8_mbtowc must fail on overlong sequences too */
> +		nlen = utf8_mbtowc(&uc, name + i, namelen - i);
> +		if (nlen < 0) {
> +			cmn_err(CE_WARN, "xfs_unicode_validate: "
> +					"invalid UTF-8 sequence\n");
> +			return -EILSEQ;
> +		}
> +		/* check for invalid/surrogate/private unicode chars */
> +		if (uc >= 0xfffe || (uc >= 0xd800 && uc <= 0xf8ff)) {
> +			cmn_err(CE_WARN, "xfs_unicode_validate: "
> +					"unsupported UTF-8 char\n");
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}

and now this leads to stuff like:

                        rval = xfs_unicode_validate(name, namelen);
                        if (rval < 0)
                                return -rval;

... this looks odd to me.

...

> +xfs_unicode_read_cft(
> +	xfs_mount_t	*mp)
> +{
> +	int		error;
> +	xfs_inode_t	*cftip;
> +	int		size;
> +	int		nfsb;
> +	int             nmap;
> +	xfs_bmbt_irec_t *mapp;
> +	int		n;
> +	int		byte_cnt;
> +	xfs_buf_t	*bp;
> +	char		*table;
> +	xfs_dcft_t	*dcft;
> +
> +	if (mp->m_sb.sb_cftino == NULLFSINO || mp->m_sb.sb_cftino == 0)
> +		return EINVAL;

oh, here it's positive? :)

...

> +void
> +xfs_unicode_free_cft(
> +	const xfs_cft_t	*cft)
> +{
> +	remove_cft(cft);
> +}

why the wrapper?

> +void
> +xfs_unicode_init(void)
> +{
> +	mutex_init(&cft_lock);
> +	xfs_nls_uni_zone = kmem_zone_init(MAXNAMELEN, "xfs_nls_uni");
> +}

Hm, no corresponding mutex_destroy

> +void
> +xfs_unicode_uninit(void)
> +{
> +	int		i;
> +
> +	mutex_lock(&cft_lock);
> +
> +	for (i = 0; i < cft_size; i++) {
> +		ASSERT(cft_list[i].refcount == 0);
> +		vfree(cft_list[i].table);
> +	}
> +	kmem_free(cft_list, cft_size * sizeof(struct cft_item));
> +	cft_size = 0;
> +	cft_list = NULL;
> +
> +	mutex_unlock(&cft_lock);

destroy mutex here too just for tidiness

> +	kmem_zone_destroy(xfs_nls_uni_zone);
> +}
> 

...