[PATCH v5 5/8] fat: restructure export_operations

[PATCH v5 5/8] fat: restructure export_operations

[Namjae Jeon]

From: Namjae Jeon <namjae.jeon@samsung.com>

Define two nfs export_operation structures,one for 'stale_rw' mounts and
the other for 'nostale_ro'.The latter uses i_pos as a basis for encoding
and decoding file handles.

Also, assign i_pos to kstat->ino.The logic for rebuilding the inode is
added in the subsequent patches.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Ravishankar N <ravi.n1@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 fs/fat/fat.h             |    8 +--
 fs/fat/file.c            |    5 ++
 fs/fat/inode.c           |   11 ++--
 fs/fat/nfs.c             |  125 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/exportfs.h |   11 ++++
 5 files changed, 141 insertions(+), 19 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 1f1c9df..40ceb79 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -404,12 +404,8 @@ int fat_cache_init(void);
 void fat_cache_destroy(void);
 
 /* fat/nfs.c */
-struct fid;
-extern struct dentry *fat_fh_to_dentry(struct super_block *sb, struct fid *fid,
-				       int fh_len, int fh_type);
-extern struct dentry *fat_fh_to_parent(struct super_block *sb, struct fid *fid,
-				       int fh_len, int fh_type);
-extern struct dentry *fat_get_parent(struct dentry *child_dir);
+extern const struct export_operations fat_export_ops;
+extern const struct export_operations fat_export_ops_nostale;
 
 /* helper for printk */
 typedef unsigned long long	llu;
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 416fa5f..edac47d 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -306,6 +306,11 @@ int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
 	struct inode *inode = dentry->d_inode;
 	generic_fillattr(inode, stat);
 	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
+
+	if (MSDOS_SB(inode->i_sb)->options.nfs == FAT_NFS_NOSTALE_RO) {
+		/* Use i_pos for ino. This is used as fileid of nfs. */
+		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);
+	}
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fat_getattr);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 7a62be4..3127c0b 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -18,7 +18,6 @@
 #include <linux/pagemap.h>
 #include <linux/mpage.h>
 #include <linux/buffer_head.h>
-#include <linux/exportfs.h>
 #include <linux/mount.h>
 #include <linux/vfs.h>
 #include <linux/parser.h>
@@ -690,12 +689,6 @@ static const struct super_operations fat_sops = {
 	.show_options	= fat_show_options,
 };
 
-static const struct export_operations fat_export_ops = {
-	.fh_to_dentry	= fat_fh_to_dentry,
-	.fh_to_parent	= fat_fh_to_parent,
-	.get_parent	= fat_get_parent,
-};
-
 static int fat_show_options(struct seq_file *m, struct dentry *root)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(root->d_sb);
@@ -1118,8 +1111,10 @@ out:
 		opts->allow_utime = ~opts->fs_dmask & (S_IWGRP | S_IWOTH);
 	if (opts->unicode_xlate)
 		opts->utf8 = 0;
-	if (opts->nfs == FAT_NFS_NOSTALE_RO)
+	if (opts->nfs == FAT_NFS_NOSTALE_RO) {
 		sb->s_flags |= MS_RDONLY;
+		sb->s_export_op = &fat_export_ops_nostale;
+	}
 
 	return 0;
 }
diff --git a/fs/fat/nfs.c b/fs/fat/nfs.c
index ef4b5fa..6ea1d41 100644
--- a/fs/fat/nfs.c
+++ b/fs/fat/nfs.c
@@ -14,6 +14,19 @@
 #include <linux/exportfs.h>
 #include "fat.h"
 
+struct fat_fid {
+	u32 i_gen;
+	u32 i_pos_low;
+	u16 i_pos_hi;
+	u16 parent_i_pos_hi;
+	u32 parent_i_pos_low;
+	u32 parent_i_gen;
+} __packed;
+
+#define FAT_FID_SIZE_WITHOUT_PARENT (offsetof(struct fat_fid, \
+					      parent_i_pos_hi)/4)
+#define FAT_FID_SIZE_WITH_PARENT (sizeof(struct fat_fid)/4)
+
 /**
  * Look up a directory inode given its starting cluster.
  */
@@ -39,8 +52,8 @@ static struct inode *fat_dget(struct super_block *sb, int i_logstart)
 	return inode;
 }
 
-static struct inode *fat_nfs_get_inode(struct super_block *sb,
-				       u64 ino, u32 generation)
+static struct inode *__fat_nfs_get_inode(struct super_block *sb,
+				       u64 ino, u32 generation, loff_t i_pos)
 {
 	struct inode *inode;
 
@@ -56,35 +69,124 @@ static struct inode *fat_nfs_get_inode(struct super_block *sb,
 	return inode;
 }
 
+static struct inode *fat_nfs_get_inode(struct super_block *sb,
+				       u64 ino, u32 generation)
+{
+
+	return __fat_nfs_get_inode(sb, ino, generation, 0);
+}
+
+static int
+fat_encode_fh_nostale(struct inode *inode, __u32 *fh, int *lenp,
+		      struct inode *parent)
+{
+	int len = *lenp;
+	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
+	struct fat_fid *fid = (struct fat_fid *) fh;
+	loff_t i_pos;
+	int type = FILEID_FAT_WITHOUT_PARENT;
+
+	if (parent && (len < FAT_FID_SIZE_WITH_PARENT)) {
+		*lenp = FAT_FID_SIZE_WITH_PARENT;
+		return 255;
+	} else if (len < FAT_FID_SIZE_WITHOUT_PARENT) {
+		*lenp = FAT_FID_SIZE_WITHOUT_PARENT;
+		return 255;
+	}
+
+	i_pos = fat_i_pos_read(sbi, inode);
+	*lenp = FAT_FID_SIZE_WITHOUT_PARENT;
+	fid->i_gen = inode->i_generation;
+	fid->i_pos_low = i_pos & 0xFFFFFFFF;
+	fid->i_pos_hi = (i_pos >> 32) & 0xFFFF;
+	if (parent) {
+		i_pos = fat_i_pos_read(sbi, parent);
+		fid->parent_i_pos_hi = (i_pos >> 32) & 0xFFFF;
+		fid->parent_i_pos_low = i_pos & 0xFFFFFFFF;
+		fid->parent_i_gen = parent->i_generation;
+		type = FILEID_FAT_WITH_PARENT;
+		*lenp = FAT_FID_SIZE_WITH_PARENT;
+	}
+
+	return type;
+}
+
 /**
  * Map a NFS file handle to a corresponding dentry.
  * The dentry may or may not be connected to the filesystem root.
  */
-struct dentry *fat_fh_to_dentry(struct super_block *sb, struct fid *fid,
+static struct dentry *fat_fh_to_dentry(struct super_block *sb, struct fid *fid,
 				int fh_len, int fh_type)
 {
 	return generic_fh_to_dentry(sb, fid, fh_len, fh_type,
 				    fat_nfs_get_inode);
 }
 
+static struct dentry *fat_fh_to_dentry_nostale(struct super_block *sb,
+					       struct fid *fh, int fh_len,
+					       int fh_type)
+{
+	struct inode *inode = NULL;
+	struct fat_fid *fid = (struct fat_fid *)fh;
+	loff_t i_pos;
+
+	switch (fh_type) {
+	case FILEID_FAT_WITHOUT_PARENT:
+		if (fh_len < FAT_FID_SIZE_WITHOUT_PARENT)
+			return NULL;
+	case FILEID_FAT_WITH_PARENT:
+		if ((fh_len < FAT_FID_SIZE_WITH_PARENT) &&
+			(fh_type == FILEID_FAT_WITH_PARENT))
+			return NULL;
+		i_pos = fid->i_pos_hi;
+		i_pos = (i_pos << 32) | (fid->i_pos_low);
+		inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);
+		break;
+	}
+
+	return d_obtain_alias(inode);
+}
+
 /*
  * Find the parent for a file specified by NFS handle.
  * This requires that the handle contain the i_ino of the parent.
  */
-struct dentry *fat_fh_to_parent(struct super_block *sb, struct fid *fid,
+static struct dentry *fat_fh_to_parent(struct super_block *sb, struct fid *fid,
 				int fh_len, int fh_type)
 {
 	return generic_fh_to_parent(sb, fid, fh_len, fh_type,
 				    fat_nfs_get_inode);
 }
 
+static struct dentry *fat_fh_to_parent_nostale(struct super_block *sb,
+					       struct fid *fh, int fh_len,
+					       int fh_type)
+{
+	struct inode *inode = NULL;
+	struct fat_fid *fid = (struct fat_fid *)fh;
+	loff_t i_pos;
+
+	if (fh_len < FAT_FID_SIZE_WITH_PARENT)
+		return NULL;
+
+	switch (fh_type) {
+	case FILEID_FAT_WITH_PARENT:
+		i_pos = fid->parent_i_pos_hi;
+		i_pos = (i_pos << 32) | (fid->parent_i_pos_low);
+		inode = __fat_nfs_get_inode(sb, 0, fid->parent_i_gen, i_pos);
+		break;
+	}
+
+	return d_obtain_alias(inode);
+}
+
 /*
  * Find the parent for a directory that is not currently connected to
  * the filesystem root.
  *
  * On entry, the caller holds child_dir->d_inode->i_mutex.
  */
-struct dentry *fat_get_parent(struct dentry *child_dir)
+static struct dentry *fat_get_parent(struct dentry *child_dir)
 {
 	struct super_block *sb = child_dir->d_sb;
 	struct buffer_head *bh = NULL;
@@ -99,3 +201,16 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
 
 	return d_obtain_alias(parent_inode);
 }
+
+const struct export_operations fat_export_ops = {
+	.fh_to_dentry   = fat_fh_to_dentry,
+	.fh_to_parent   = fat_fh_to_parent,
+	.get_parent     = fat_get_parent,
+};
+
+const struct export_operations fat_export_ops_nostale = {
+	.encode_fh      = fat_encode_fh_nostale,
+	.fh_to_dentry   = fat_fh_to_dentry_nostale,
+	.fh_to_parent   = fat_fh_to_parent_nostale,
+	.get_parent     = fat_get_parent,
+};
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 0e14525..66bb662 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -85,6 +85,17 @@ enum fid_type {
 	FILEID_NILFS_WITH_PARENT = 0x62,
 
 	/*
+	 * 32 bit generation number, 40 bit i_pos.
+	 */
+	FILEID_FAT_WITHOUT_PARENT = 0x71,
+
+	/*
+	 * 32 bit generation number, 40 bit i_pos,
+	 * 32 bit parent generation number, 40 bit parent i_pos
+	 */
+	FILEID_FAT_WITH_PARENT = 0x72,
+
+	/*
 	 * Filesystems must not use 0xff file ID.
 	 */
 	FILEID_INVALID = 0xff,
-- 
1.7.9.5

Re: [PATCH v5 5/8] fat: restructure export_operations

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> +	if (MSDOS_SB(inode->i_sb)->options.nfs == FAT_NFS_NOSTALE_RO) {
> +		/* Use i_pos for ino. This is used as fileid of nfs. */
> +		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);

BTW, what number is used for root dir? If it is 0 (0 is special ino in
glibc), we have to use MSDOS_ROOT_INO instead.

> +#define FAT_FID_SIZE_WITHOUT_PARENT (offsetof(struct fat_fid, \
> +					      parent_i_pos_hi)/4)

(offset parent_i_pos_hi) / 4 == 2. Wrong.

> +#define FAT_FID_SIZE_WITH_PARENT (sizeof(struct fat_fid)/4)

4 should be sizeof(u32). Or simplely use immediate value.

> +static int
> +fat_encode_fh_nostale(struct inode *inode, __u32 *fh, int *lenp,
> +		      struct inode *parent)
> +{
> +	int len = *lenp;
> +	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
> +	struct fat_fid *fid = (struct fat_fid *) fh;
> +	loff_t i_pos;
> +	int type = FILEID_FAT_WITHOUT_PARENT;
> +
> +	if (parent && (len < FAT_FID_SIZE_WITH_PARENT)) {
> +		*lenp = FAT_FID_SIZE_WITH_PARENT;
> +		return 255;

255 is now FILEID_INVALID, I think.

> +	} else if (len < FAT_FID_SIZE_WITHOUT_PARENT) {
> +		*lenp = FAT_FID_SIZE_WITHOUT_PARENT;
> +		return 255;
> +	}
> +
> +	i_pos = fat_i_pos_read(sbi, inode);
> +	*lenp = FAT_FID_SIZE_WITHOUT_PARENT;
> +	fid->i_gen = inode->i_generation;
> +	fid->i_pos_low = i_pos & 0xFFFFFFFF;
> +	fid->i_pos_hi = (i_pos >> 32) & 0xFFFF;
> +	if (parent) {
> +		i_pos = fat_i_pos_read(sbi, parent);
> +		fid->parent_i_pos_hi = (i_pos >> 32) & 0xFFFF;
> +		fid->parent_i_pos_low = i_pos & 0xFFFFFFFF;
> +		fid->parent_i_gen = parent->i_generation;
> +		type = FILEID_FAT_WITH_PARENT;
> +		*lenp = FAT_FID_SIZE_WITH_PARENT;
> +	}
> +
> +	return type;
> +}
> +
>  /**
>   * Map a NFS file handle to a corresponding dentry.
>   * The dentry may or may not be connected to the filesystem root.
>   */
> -struct dentry *fat_fh_to_dentry(struct super_block *sb, struct fid *fid,
> +static struct dentry *fat_fh_to_dentry(struct super_block *sb, struct fid *fid,
>  				int fh_len, int fh_type)
>  {
>  	return generic_fh_to_dentry(sb, fid, fh_len, fh_type,
>  				    fat_nfs_get_inode);
>  }
>  
> +static struct dentry *fat_fh_to_dentry_nostale(struct super_block *sb,
> +					       struct fid *fh, int fh_len,
> +					       int fh_type)
> +{
> +	struct inode *inode = NULL;
> +	struct fat_fid *fid = (struct fat_fid *)fh;
> +	loff_t i_pos;
> +
> +	switch (fh_type) {
> +	case FILEID_FAT_WITHOUT_PARENT:
> +		if (fh_len < FAT_FID_SIZE_WITHOUT_PARENT)
> +			return NULL;
> +	case FILEID_FAT_WITH_PARENT:
> +		if ((fh_len < FAT_FID_SIZE_WITH_PARENT) &&
> +			(fh_type == FILEID_FAT_WITH_PARENT))
> +			return NULL;

Do we have to care (FILEID_FAT_WITH_PARENT and fh_len < 5) here?

	if (fh_len < 2)
		return NULL;

	switch (fh_type) {
	case FILEID_INO32_GEN:
	case FILEID_INO32_GEN_PARENT:
		inode = get_inode(sb, fid->i32.ino, fid->i32.gen);
		break;
	}

	return d_obtain_alias(inode);

generic_fh_to_dentry() is above. I wonder why we have to care
fat_fid->parent* here.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 5/8] fat: restructure export_operations

[Namjae Jeon]

2012/12/3, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> +	if (MSDOS_SB(inode->i_sb)->options.nfs == FAT_NFS_NOSTALE_RO) {
>> +		/* Use i_pos for ino. This is used as fileid of nfs. */
>> +		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);
>
> BTW, what number is used for root dir? If it is 0 (0 is special ino in
> glibc), we have to use MSDOS_ROOT_INO instead.
we have used default root ino number which is MSDOS_ROOT_INO.
>
>> +#define FAT_FID_SIZE_WITHOUT_PARENT (offsetof(struct fat_fid, \
>> +					      parent_i_pos_hi)/4)
>
> (offset parent_i_pos_hi) / 4 == 2. Wrong.
 Yes, this needs correction. Since, at all the places the condition
was for ‘fh_len < 2’ so this error condition was never caught.
>
>> +#define FAT_FID_SIZE_WITH_PARENT (sizeof(struct fat_fid)/4)
>
> 4 should be sizeof(u32). Or simplely use immediate value.
Okay.
>
>> +static int
>> +fat_encode_fh_nostale(struct inode *inode, __u32 *fh, int *lenp,
>> +		      struct inode *parent)
>> +{
>> +	int len = *lenp;
>> +	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
>> +	struct fat_fid *fid = (struct fat_fid *) fh;
>> +	loff_t i_pos;
>> +	int type = FILEID_FAT_WITHOUT_PARENT;
>> +
>> +	if (parent && (len < FAT_FID_SIZE_WITH_PARENT)) {
>> +		*lenp = FAT_FID_SIZE_WITH_PARENT;
>> +		return 255;
>
> 255 is now FILEID_INVALID, I think.
Yes, right.
>
>> +	} else if (len < FAT_FID_SIZE_WITHOUT_PARENT) {
>> +		*lenp = FAT_FID_SIZE_WITHOUT_PARENT;
>> +		return 255;
>> +	}
>> +
>> +	i_pos = fat_i_pos_read(sbi, inode);
>> +	*lenp = FAT_FID_SIZE_WITHOUT_PARENT;
>> +	fid->i_gen = inode->i_generation;
>> +	fid->i_pos_low = i_pos & 0xFFFFFFFF;
>> +	fid->i_pos_hi = (i_pos >> 32) & 0xFFFF;
>> +	if (parent) {
>> +		i_pos = fat_i_pos_read(sbi, parent);
>> +		fid->parent_i_pos_hi = (i_pos >> 32) & 0xFFFF;
>> +		fid->parent_i_pos_low = i_pos & 0xFFFFFFFF;
>> +		fid->parent_i_gen = parent->i_generation;
>> +		type = FILEID_FAT_WITH_PARENT;
>> +		*lenp = FAT_FID_SIZE_WITH_PARENT;
>> +	}
>> +
>> +	return type;
>> +}
>> +
>>  /**
>>   * Map a NFS file handle to a corresponding dentry.
>>   * The dentry may or may not be connected to the filesystem root.
>>   */
>> -struct dentry *fat_fh_to_dentry(struct super_block *sb, struct fid *fid,
>> +static struct dentry *fat_fh_to_dentry(struct super_block *sb, struct fid
>> *fid,
>>  				int fh_len, int fh_type)
>>  {
>>  	return generic_fh_to_dentry(sb, fid, fh_len, fh_type,
>>  				    fat_nfs_get_inode);
>>  }
>>
>> +static struct dentry *fat_fh_to_dentry_nostale(struct super_block *sb,
>> +					       struct fid *fh, int fh_len,
>> +					       int fh_type)
>> +{
>> +	struct inode *inode = NULL;
>> +	struct fat_fid *fid = (struct fat_fid *)fh;
>> +	loff_t i_pos;
>> +
>> +	switch (fh_type) {
>> +	case FILEID_FAT_WITHOUT_PARENT:
>> +		if (fh_len < FAT_FID_SIZE_WITHOUT_PARENT)
>> +			return NULL;
>> +	case FILEID_FAT_WITH_PARENT:
>> +		if ((fh_len < FAT_FID_SIZE_WITH_PARENT) &&
>> +			(fh_type == FILEID_FAT_WITH_PARENT))
>> +			return NULL;
>
> Do we have to care (FILEID_FAT_WITH_PARENT and fh_len < 5) here?
>
> 	if (fh_len < 2)
> 		return NULL;
>
> 	switch (fh_type) {
> 	case FILEID_INO32_GEN:
> 	case FILEID_INO32_GEN_PARENT:
> 		inode = get_inode(sb, fid->i32.ino, fid->i32.gen);
> 		break;
> 	}
>
> 	return d_obtain_alias(inode);
>
> generic_fh_to_dentry() is above. I wonder why we have to care
> fat_fid->parent* here.
Let me think, if ‘subtree’ checking is enabled then we should check
the length condition over here also? Please share if there are any
other comments also.

Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v5 5/8] fat: restructure export_operations

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>>> +static struct dentry *fat_fh_to_dentry_nostale(struct super_block *sb,
>>> +					       struct fid *fh, int fh_len,
>>> +					       int fh_type)
>>> +{
>>> +	struct inode *inode = NULL;
>>> +	struct fat_fid *fid = (struct fat_fid *)fh;
>>> +	loff_t i_pos;
>>> +
>>> +	switch (fh_type) {
>>> +	case FILEID_FAT_WITHOUT_PARENT:
>>> +		if (fh_len < FAT_FID_SIZE_WITHOUT_PARENT)
>>> +			return NULL;
>>> +	case FILEID_FAT_WITH_PARENT:
>>> +		if ((fh_len < FAT_FID_SIZE_WITH_PARENT) &&
>>> +			(fh_type == FILEID_FAT_WITH_PARENT))
>>> +			return NULL;
>>
>> Do we have to care (FILEID_FAT_WITH_PARENT and fh_len < 5) here?
>>
>> 	if (fh_len < 2)
>> 		return NULL;
>>
>> 	switch (fh_type) {
>> 	case FILEID_INO32_GEN:
>> 	case FILEID_INO32_GEN_PARENT:
>> 		inode = get_inode(sb, fid->i32.ino, fid->i32.gen);
>> 		break;
>> 	}
>>
>> 	return d_obtain_alias(inode);
>>
>> generic_fh_to_dentry() is above. I wonder why we have to care
>> fat_fid->parent* here.
> Let me think, if ‘subtree’ checking is enabled then we should check
> the length condition over here also? Please share if there are any
> other comments also.

I'm not sure what did you mean. Where is "subtree" check you are
talking? This is fh_to_dentry(), so we don't use parent at all, so
length == 3 is enough?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 5/8] fat: restructure export_operations

[Namjae Jeon]

2012/12/4, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>>> +static struct dentry *fat_fh_to_dentry_nostale(struct super_block *sb,
>>>> +					       struct fid *fh, int fh_len,
>>>> +					       int fh_type)
>>>> +{
>>>> +	struct inode *inode = NULL;
>>>> +	struct fat_fid *fid = (struct fat_fid *)fh;
>>>> +	loff_t i_pos;
>>>> +
>>>> +	switch (fh_type) {
>>>> +	case FILEID_FAT_WITHOUT_PARENT:
>>>> +		if (fh_len < FAT_FID_SIZE_WITHOUT_PARENT)
>>>> +			return NULL;
>>>> +	case FILEID_FAT_WITH_PARENT:
>>>> +		if ((fh_len < FAT_FID_SIZE_WITH_PARENT) &&
>>>> +			(fh_type == FILEID_FAT_WITH_PARENT))
>>>> +			return NULL;
>>>
>>> Do we have to care (FILEID_FAT_WITH_PARENT and fh_len < 5) here?
>>>
>>> 	if (fh_len < 2)
>>> 		return NULL;
>>>
>>> 	switch (fh_type) {
>>> 	case FILEID_INO32_GEN:
>>> 	case FILEID_INO32_GEN_PARENT:
>>> 		inode = get_inode(sb, fid->i32.ino, fid->i32.gen);
>>> 		break;
>>> 	}
>>>
>>> 	return d_obtain_alias(inode);
>>>
>>> generic_fh_to_dentry() is above. I wonder why we have to care
>>> fat_fid->parent* here.
>> Let me think, if ‘subtree’ checking is enabled then we should check
>> the length condition over here also? Please share if there are any
>> other comments also.
>
> I'm not sure what did you mean. Where is "subtree" check you are
> talking? This is fh_to_dentry(), so we don't use parent at all, so
> length == 3 is enough?
With fileID type="FILEID_FAT_WITHOUT_PARENT", fhlen  should be '3'
With fileId type="FILEID_FAT_WITH_PARENT", fhlen should be '5'

While encoding, WITH_PARENT is selected when subtree check is enabled
on the NFS Server.

So, when decoding request is arrived-  fileid type will be among the '2' cases:
Now, in case of fh_to_dentry() - when we consider, that the reqquest
for fileid type WITH_PARENT
then i think the conditions in fh_to_dentry should be:

if((fh_type == FILEID_FAT_WITH_PARENT) && fh_len != 5)
	return NULL;
else if (fh_len != 3)
	return NULL;

So, we took care of these '2' conditions within the switch statement
based on the 'fh_type'. We can just change the comparision condition
from '<' to '!=':
switch (fh_type) {
+	case FILEID_FAT_WITHOUT_PARENT:
+		if (fh_len != FAT_FID_SIZE_WITHOUT_PARENT)
+			return NULL;
+	case FILEID_FAT_WITH_PARENT:
+		if ((fh_len != FAT_FID_SIZE_WITH_PARENT) &&
+			(fh_type == FILEID_FAT_WITH_PARENT))
+			return NULL;
+		i_pos = fid->i_pos_hi;
+		i_pos = (i_pos << 32) | (fid->i_pos_low);
+		inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);
+		break;
+	}

I think there is no need to push the comparision statements in the
begining similar to generic_fh_to_dentry.

Thanks OGAWA.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v5 5/8] fat: restructure export_operations

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>>> Let me think, if ‘subtree’ checking is enabled then we should check
>>> the length condition over here also? Please share if there are any
>>> other comments also.
>>
>> I'm not sure what did you mean. Where is "subtree" check you are
>> talking? This is fh_to_dentry(), so we don't use parent at all, so
>> length == 3 is enough?
> With fileID type="FILEID_FAT_WITHOUT_PARENT", fhlen  should be '3'
> With fileId type="FILEID_FAT_WITH_PARENT", fhlen should be '5'
>
> While encoding, WITH_PARENT is selected when subtree check is enabled
> on the NFS Server.
>
> So, when decoding request is arrived-  fileid type will be among the '2' cases:
> Now, in case of fh_to_dentry() - when we consider, that the reqquest
> for fileid type WITH_PARENT
> then i think the conditions in fh_to_dentry should be:
>
> if((fh_type == FILEID_FAT_WITH_PARENT) && fh_len != 5)
> 	return NULL;
> else if (fh_len != 3)
> 	return NULL;
>
> So, we took care of these '2' conditions within the switch statement
> based on the 'fh_type'. We can just change the comparision condition
> from '<' to '!=':
> switch (fh_type) {
> +	case FILEID_FAT_WITHOUT_PARENT:
> +		if (fh_len != FAT_FID_SIZE_WITHOUT_PARENT)
> +			return NULL;
> +	case FILEID_FAT_WITH_PARENT:
> +		if ((fh_len != FAT_FID_SIZE_WITH_PARENT) &&
> +			(fh_type == FILEID_FAT_WITH_PARENT))
> +			return NULL;
> +		i_pos = fid->i_pos_hi;
> +		i_pos = (i_pos << 32) | (fid->i_pos_low);
> +		inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);
> +		break;
> +	}
>
> I think there is no need to push the comparision statements in the
> begining similar to generic_fh_to_dentry.

I can understand what is doing. I'm asking why there is difference.

1) generic_fh_to_dentry() allows (*_PARENT && fh_len == 2).
2) fat_fh_to_dentry_nostale() doesn't allows (*_PARENT && fh_len == 3).

Why does logic has difference?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 5/8] fat: restructure export_operations

[Namjae Jeon]

2012/12/5, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>>> Let me think, if ‘subtree’ checking is enabled then we should check
>>>> the length condition over here also? Please share if there are any
>>>> other comments also.
>>>
>>> I'm not sure what did you mean. Where is "subtree" check you are
>>> talking? This is fh_to_dentry(), so we don't use parent at all, so
>>> length == 3 is enough?
>> With fileID type="FILEID_FAT_WITHOUT_PARENT", fhlen  should be '3'
>> With fileId type="FILEID_FAT_WITH_PARENT", fhlen should be '5'
>>
>> While encoding, WITH_PARENT is selected when subtree check is enabled
>> on the NFS Server.
>>
>> So, when decoding request is arrived-  fileid type will be among the '2'
>> cases:
>> Now, in case of fh_to_dentry() - when we consider, that the reqquest
>> for fileid type WITH_PARENT
>> then i think the conditions in fh_to_dentry should be:
>>
>> if((fh_type == FILEID_FAT_WITH_PARENT) && fh_len != 5)
>> 	return NULL;
>> else if (fh_len != 3)
>> 	return NULL;
>>
>> So, we took care of these '2' conditions within the switch statement
>> based on the 'fh_type'. We can just change the comparision condition
>> from '<' to '!=':
>> switch (fh_type) {
>> +	case FILEID_FAT_WITHOUT_PARENT:
>> +		if (fh_len != FAT_FID_SIZE_WITHOUT_PARENT)
>> +			return NULL;
>> +	case FILEID_FAT_WITH_PARENT:
>> +		if ((fh_len != FAT_FID_SIZE_WITH_PARENT) &&
>> +			(fh_type == FILEID_FAT_WITH_PARENT))
>> +			return NULL;
>> +		i_pos = fid->i_pos_hi;
>> +		i_pos = (i_pos << 32) | (fid->i_pos_low);
>> +		inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);
>> +		break;
>> +	}
>>
>> I think there is no need to push the comparision statements in the
>> begining similar to generic_fh_to_dentry.
>
> I can understand what is doing. I'm asking why there is difference.
>
> 1) generic_fh_to_dentry() allows (*_PARENT && fh_len == 2).
> 2) fat_fh_to_dentry_nostale() doesn't allows (*_PARENT && fh_len == 3).
>
> Why does logic has difference?

When we consider the generic routine of encode_fh() and the structure ‘fid’

struct fid {
      struct {
            u32 ino;
            u32 gen;
            u32 parent_ino;
            u32 parent_gen;
      } i32;
};

fh_len= 2(without parent)
fh_len=4(with parent)

Condition checking in export_encode_fh()
{

   if (parent && (len < 4)) {
                *max_len = 4;
                return FILEID_INVALID;
        } else if (len < 2) {
                *max_len = 2;
                return FILEID_INVALID;
        }
        ...
                len = 2;
        ...
                if (parent) {
..
                                len = 4;
                                type = FILEID_INO32_GEN_PARENT;
                }
…
}

The logic does take care of altering the length for the ‘2’ cases
with/without parent.
So, while encoding -> the care has been taken for length checking but
while decoding(generic_fh_to_dentry) the length check is not put in
place.
I think it should be done in the generic routine also.

It should be:
if ((fh_len != 2 && fh_len != 4) ||
                (fh_type != FILEID_INO32_GEN && fh_type !=
FILEID_INO32_GEN_PARENT))
    return NULL;

Please share your opinion.

Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v5 5/8] fat: restructure export_operations

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> I can understand what is doing. I'm asking why there is difference.
>>
>> 1) generic_fh_to_dentry() allows (*_PARENT && fh_len == 2).
>> 2) fat_fh_to_dentry_nostale() doesn't allows (*_PARENT && fh_len == 3).
>>
>> Why does logic has difference?
>
> When we consider the generic routine of encode_fh() and the structure ‘fid’
>
> struct fid {
>       struct {
>             u32 ino;
>             u32 gen;
>             u32 parent_ino;
>             u32 parent_gen;
>       } i32;
> };
>
> fh_len= 2(without parent)
> fh_len=4(with parent)
>
> Condition checking in export_encode_fh()
> {
>
>    if (parent && (len < 4)) {
>                 *max_len = 4;
>                 return FILEID_INVALID;
>         } else if (len < 2) {
>                 *max_len = 2;
>                 return FILEID_INVALID;
>         }
>         ...
>                 len = 2;
>         ...
>                 if (parent) {
> ..
>                                 len = 4;
>                                 type = FILEID_INO32_GEN_PARENT;
>                 }
> …
> }
>
> The logic does take care of altering the length for the ‘2’ cases
> with/without parent.
> So, while encoding -> the care has been taken for length checking but
> while decoding(generic_fh_to_dentry) the length check is not put in
> place.
> I think it should be done in the generic routine also.
>
> It should be:
> if ((fh_len != 2 && fh_len != 4) ||
>                 (fh_type != FILEID_INO32_GEN && fh_type !=
> FILEID_INO32_GEN_PARENT))
>     return NULL;
>
> Please share your opinion.

I know encode_fh(). But NFS is network protocol, and network can input
any data, and I guess the userland interface (open_by_handle()?) can be
any too.

And generic_fh_to_dentry()'s input verify choose to check the minimum
length only. But your logic choose the exact length.

I think the both is sane and correct. But I wonder why did you changed it.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 5/8] fat: restructure export_operations

[Namjae Jeon]

2012/12/5, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> I can understand what is doing. I'm asking why there is difference.
>>>
>>> 1) generic_fh_to_dentry() allows (*_PARENT && fh_len == 2).
>>> 2) fat_fh_to_dentry_nostale() doesn't allows (*_PARENT && fh_len == 3).
>>>
>>> Why does logic has difference?
>>
>> When we consider the generic routine of encode_fh() and the structure
>> ‘fid’
>>
>> struct fid {
>>       struct {
>>             u32 ino;
>>             u32 gen;
>>             u32 parent_ino;
>>             u32 parent_gen;
>>       } i32;
>> };
>>
>> fh_len= 2(without parent)
>> fh_len=4(with parent)
>>
>> Condition checking in export_encode_fh()
>> {
>>
>>    if (parent && (len < 4)) {
>>                 *max_len = 4;
>>                 return FILEID_INVALID;
>>         } else if (len < 2) {
>>                 *max_len = 2;
>>                 return FILEID_INVALID;
>>         }
>>         ...
>>                 len = 2;
>>         ...
>>                 if (parent) {
>> ..
>>                                 len = 4;
>>                                 type = FILEID_INO32_GEN_PARENT;
>>                 }
>> …
>> }
>>
>> The logic does take care of altering the length for the ‘2’ cases
>> with/without parent.
>> So, while encoding -> the care has been taken for length checking but
>> while decoding(generic_fh_to_dentry) the length check is not put in
>> place.
>> I think it should be done in the generic routine also.
>>
>> It should be:
>> if ((fh_len != 2 && fh_len != 4) ||
>>                 (fh_type != FILEID_INO32_GEN && fh_type !=
>> FILEID_INO32_GEN_PARENT))
>>     return NULL;
>>
>> Please share your opinion.
>
> I know encode_fh(). But NFS is network protocol, and network can input
> any data, and I guess the userland interface (open_by_handle()?) can be
> any too.
>
> And generic_fh_to_dentry()'s input verify choose to check the minimum
> length only. But your logic choose the exact length.
>
> I think the both is sane and correct. But I wonder why did you changed it.
There was no particular reason for us to put those conditions. It is
just we knew what fh lengths we have chosen for the 2 cases
WITH/WITHOUT parent.
i.e., we checked with encoded length.
Now, when I check the export functions of other filesystems(btrfs,
nilfs2, udf). They also adopt the same method of checking the exact
length and type.
If there is any particular reason, we will look into that and can also
updated on that.

Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v5 5/8] fat: restructure export_operations

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> I know encode_fh(). But NFS is network protocol, and network can input
>> any data, and I guess the userland interface (open_by_handle()?) can be
>> any too.
>>
>> And generic_fh_to_dentry()'s input verify choose to check the minimum
>> length only. But your logic choose the exact length.
>>
>> I think the both is sane and correct. But I wonder why did you changed it.
> There was no particular reason for us to put those conditions. It is
> just we knew what fh lengths we have chosen for the 2 cases
> WITH/WITHOUT parent.
> i.e., we checked with encoded length.
> Now, when I check the export functions of other filesystems(btrfs,
> nilfs2, udf). They also adopt the same method of checking the exact
> length and type.
> If there is any particular reason, we will look into that and can also
> updated on that.

OK. Then, just cleanup code, and let's use strict checking version.

Removing strange fallthrou, something like the following.

static struct dentry *fat_fh_to_dentry_nostale(struct super_block *sb,
					       struct fid *fh, int fh_len,
					       int fh_type)
{
	struct inode *inode = NULL;
	struct fat_fid *fid = (struct fat_fid *)fh;
	loff_t i_pos;

	switch (fh_type) {
	case FILEID_FAT_WITHOUT_PARENT:
		if (fh_len < FAT_FID_SIZE_WITHOUT_PARENT)
			return NULL;
		break;
	case FILEID_FAT_WITH_PARENT:
		if (fh_len < FAT_FID_SIZE_WITH_PARENT)
			return NULL;
		break;
	default:
		return NULL;
	}

	i_pos = fid->i_pos_hi;
	i_pos = (i_pos << 32) | (fid->i_pos_low);
	inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);

	return d_obtain_alias(inode);
}


-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 5/8] fat: restructure export_operations

[Namjae Jeon]

2012/12/6, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> I know encode_fh(). But NFS is network protocol, and network can input
>>> any data, and I guess the userland interface (open_by_handle()?) can be
>>> any too.
>>>
>>> And generic_fh_to_dentry()'s input verify choose to check the minimum
>>> length only. But your logic choose the exact length.
>>>
>>> I think the both is sane and correct. But I wonder why did you changed
>>> it.
>> There was no particular reason for us to put those conditions. It is
>> just we knew what fh lengths we have chosen for the 2 cases
>> WITH/WITHOUT parent.
>> i.e., we checked with encoded length.
>> Now, when I check the export functions of other filesystems(btrfs,
>> nilfs2, udf). They also adopt the same method of checking the exact
>> length and type.
>> If there is any particular reason, we will look into that and can also
>> updated on that.
>
> OK. Then, just cleanup code, and let's use strict checking version.
>
> Removing strange fallthrou, something like the following.
>
> static struct dentry *fat_fh_to_dentry_nostale(struct super_block *sb,
> 					       struct fid *fh, int fh_len,
> 					       int fh_type)
> {
> 	struct inode *inode = NULL;
> 	struct fat_fid *fid = (struct fat_fid *)fh;
> 	loff_t i_pos;
>
> 	switch (fh_type) {
> 	case FILEID_FAT_WITHOUT_PARENT:
> 		if (fh_len < FAT_FID_SIZE_WITHOUT_PARENT)
> 			return NULL;
> 		break;
> 	case FILEID_FAT_WITH_PARENT:
> 		if (fh_len < FAT_FID_SIZE_WITH_PARENT)
> 			return NULL;
> 		break;
> 	default:
> 		return NULL;
> 	}
>
> 	i_pos = fid->i_pos_hi;
> 	i_pos = (i_pos << 32) | (fid->i_pos_low);
> 	inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);
>
> 	return d_obtain_alias(inode);
> }
Okay. I will change like this on next patch-set.

Thanks OGAWA!
>
>
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>