[PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

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

This patch enables rebuilding of directory inodes which are not present
in the cache.This is done by traversing the disk clusters to find the
directory entry of the parent directory and using its i_pos to build the
inode.
Do this only if the "nostale_ro" nfs mount option is specified.

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/nfs.c |  119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 118 insertions(+), 1 deletion(-)

diff --git a/fs/fat/nfs.c b/fs/fat/nfs.c
index 898bef4..6a9933a 100644
--- a/fs/fat/nfs.c
+++ b/fs/fat/nfs.c
@@ -211,6 +211,120 @@ static struct dentry *fat_fh_to_parent_nostale(struct super_block *sb,
 }
 
 /*
+ * Read the directory entries of 'search_clus' and find the entry
+ * which contains 'match_ipos' for the starting cluster.If the entry
+ * is found, rebuild its inode.
+ */
+static struct inode *fat_traverse_cluster(struct super_block *sb,
+				int search_clus, int match_ipos)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	struct buffer_head *bh;
+	sector_t blknr;
+	int parent_ipos, search_ipos;
+	int i;
+	struct msdos_dir_entry *de;
+	struct inode *inode = NULL;
+	int iterations = sbi->cluster_size >> sb->s_blocksize_bits;
+	blknr = fat_clus_to_blknr(sbi, search_clus);
+
+	do {
+		bh = sb_bread(sb, blknr);
+		if (!bh) {
+			fat_msg(sb, KERN_ERR,
+				"NFS:unable to read block(%llu) while traversing cluster(%d)",
+				(llu)blknr, search_clus);
+			inode = ERR_PTR(-EIO);
+			goto out;
+		}
+		de = (struct msdos_dir_entry *)bh->b_data;
+		for (i = 0; i < sbi->dir_per_block; i++) {
+			if (de[i].name[0] == FAT_ENT_FREE) {
+				/*Reached end of directory*/
+				brelse(bh);
+				inode = ERR_PTR(-ENODATA);
+				goto out;
+			}
+			if (de[i].name[0] == DELETED_FLAG)
+				continue;
+			if (de[i].attr == ATTR_EXT)
+				continue;
+			if (!(de[i].attr & ATTR_DIR))
+				continue;
+			else {
+				search_ipos = fat_get_start(sbi, &de[i]);
+				if (search_ipos == match_ipos) {
+					/*Success.Now build the inode*/
+					parent_ipos = (loff_t)i +
+					 (blknr << sbi->dir_per_block_bits);
+					inode = fat_build_inode(sb, &de[i],
+								parent_ipos);
+					brelse(bh);
+					goto out;
+				}
+			}
+		}
+		brelse(bh);
+		blknr += 1;
+	} while (--iterations > 0);
+out:
+	return inode;
+}
+
+/*
+ * Rebuild the parent for a directory that is not connected
+ *  to the filesystem root
+ */
+static
+struct inode *fat_rebuild_parent(struct super_block *sb, int parent_logstart)
+{
+	int search_clus, clus_to_match;
+	struct msdos_dir_entry *de;
+	struct inode *parent;
+	struct fat_entry fatent;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	sector_t blknr = fat_clus_to_blknr(sbi, parent_logstart);
+	struct buffer_head *parent_bh = sb_bread(sb, blknr);
+
+	if (!parent_bh) {
+		fat_msg(sb, KERN_ERR,
+			"NFS:unable to read cluster of parent directory");
+		return NULL;
+	}
+	de = (struct msdos_dir_entry *) parent_bh->b_data;
+	clus_to_match = fat_get_start(sbi, &de[0]);
+	search_clus = fat_get_start(sbi, &de[1]);
+	if (!search_clus) {
+		if (sbi->fat_bits == 32)
+			search_clus = sbi->root_cluster;
+		else
+			/* sbi->root_cluster is valid only for FAT32.
+			 * Compute search_cluster such that when
+			 * fat_traverse_cluster() is called for FAT 12/16,
+			 * fat_clus_to_blknr() in it gives us sbi->dir_start
+			 */
+			search_clus = ((int)(sbi->dir_start - sbi->data_start)
+					     >> ilog2(sbi->sec_per_clus))
+					     + FAT_START_ENT;
+	}
+	brelse(parent_bh);
+	do {
+		parent = fat_traverse_cluster(sb,
+					search_clus, clus_to_match);
+		if (IS_ERR(parent) || parent)
+			break;
+		fatent_init(&fatent);
+		search_clus = fat_ent_read(sb, &fatent, search_clus);
+		fatent_brelse(&fatent);
+		if (search_clus < 0 || search_clus == FAT_ENT_FREE)
+			break;
+	} while (search_clus != FAT_ENT_EOF);
+
+	return parent;
+
+}
+
+/*
  * Find the parent for a directory that is not currently connected to
  * the filesystem root.
  *
@@ -222,10 +336,13 @@ static struct dentry *fat_get_parent(struct dentry *child_dir)
 	struct buffer_head *bh = NULL;
 	struct msdos_dir_entry *de;
 	struct inode *parent_inode = NULL;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
 	if (!fat_get_dotdot_entry(child_dir->d_inode, &bh, &de)) {
-		int parent_logstart = fat_get_start(MSDOS_SB(sb), de);
+		int parent_logstart = fat_get_start(sbi, de);
 		parent_inode = fat_dget(sb, parent_logstart);
+		if (!parent_inode && sbi->options.nfs == FAT_NFS_NOSTALE_RO)
+			parent_inode = fat_rebuild_parent(sb, parent_logstart);
 	}
 	brelse(bh);
 
-- 
1.7.9.5

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> From: Namjae Jeon <namjae.jeon@samsung.com>
>
> This patch enables rebuilding of directory inodes which are not present
> in the cache.This is done by traversing the disk clusters to find the
> directory entry of the parent directory and using its i_pos to build the
> inode.
> Do this only if the "nostale_ro" nfs mount option is specified.

This became much better than before. However, we have to consolidate the
code with fat_search_long() finally.

E.g. this version is having the issue already fixed. If there is
corruption in fat cluster-chain, it lead to infinite
loop. fat_get_cluster() checks infinite loop by limit.

> 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/nfs.c |  119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fat/nfs.c b/fs/fat/nfs.c
> index 898bef4..6a9933a 100644
> --- a/fs/fat/nfs.c
> +++ b/fs/fat/nfs.c
> @@ -211,6 +211,120 @@ static struct dentry *fat_fh_to_parent_nostale(struct super_block *sb,
>  }
>  
>  /*
> + * Read the directory entries of 'search_clus' and find the entry
> + * which contains 'match_ipos' for the starting cluster.If the entry
> + * is found, rebuild its inode.
> + */
> +static struct inode *fat_traverse_cluster(struct super_block *sb,
> +				int search_clus, int match_ipos)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	struct buffer_head *bh;
> +	sector_t blknr;
> +	int parent_ipos, search_ipos;
> +	int i;
> +	struct msdos_dir_entry *de;
> +	struct inode *inode = NULL;
> +	int iterations = sbi->cluster_size >> sb->s_blocksize_bits;
> +	blknr = fat_clus_to_blknr(sbi, search_clus);
> +
> +	do {
> +		bh = sb_bread(sb, blknr);
> +		if (!bh) {
> +			fat_msg(sb, KERN_ERR,
> +				"NFS:unable to read block(%llu) while traversing cluster(%d)",
> +				(llu)blknr, search_clus);
> +			inode = ERR_PTR(-EIO);
> +			goto out;
> +		}
> +		de = (struct msdos_dir_entry *)bh->b_data;
> +		for (i = 0; i < sbi->dir_per_block; i++) {
> +			if (de[i].name[0] == FAT_ENT_FREE) {
> +				/*Reached end of directory*/
> +				brelse(bh);
> +				inode = ERR_PTR(-ENODATA);
> +				goto out;
> +			}
> +			if (de[i].name[0] == DELETED_FLAG)
> +				continue;
> +			if (de[i].attr == ATTR_EXT)
> +				continue;
> +			if (!(de[i].attr & ATTR_DIR))
> +				continue;
> +			else {
> +				search_ipos = fat_get_start(sbi, &de[i]);
> +				if (search_ipos == match_ipos) {
> +					/*Success.Now build the inode*/
> +					parent_ipos = (loff_t)i +
> +					 (blknr << sbi->dir_per_block_bits);
> +					inode = fat_build_inode(sb, &de[i],
> +								parent_ipos);
> +					brelse(bh);
> +					goto out;
> +				}
> +			}
> +		}
> +		brelse(bh);
> +		blknr += 1;
> +	} while (--iterations > 0);
> +out:
> +	return inode;
> +}
> +
> +/*
> + * Rebuild the parent for a directory that is not connected
> + *  to the filesystem root
> + */
> +static
> +struct inode *fat_rebuild_parent(struct super_block *sb, int parent_logstart)
> +{
> +	int search_clus, clus_to_match;
> +	struct msdos_dir_entry *de;
> +	struct inode *parent;
> +	struct fat_entry fatent;
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	sector_t blknr = fat_clus_to_blknr(sbi, parent_logstart);
> +	struct buffer_head *parent_bh = sb_bread(sb, blknr);
> +
> +	if (!parent_bh) {
> +		fat_msg(sb, KERN_ERR,
> +			"NFS:unable to read cluster of parent directory");
> +		return NULL;
> +	}
> +	de = (struct msdos_dir_entry *) parent_bh->b_data;
> +	clus_to_match = fat_get_start(sbi, &de[0]);
> +	search_clus = fat_get_start(sbi, &de[1]);
> +	if (!search_clus) {
> +		if (sbi->fat_bits == 32)
> +			search_clus = sbi->root_cluster;
> +		else
> +			/* sbi->root_cluster is valid only for FAT32.
> +			 * Compute search_cluster such that when
> +			 * fat_traverse_cluster() is called for FAT 12/16,
> +			 * fat_clus_to_blknr() in it gives us sbi->dir_start
> +			 */
> +			search_clus = ((int)(sbi->dir_start - sbi->data_start)
> +					     >> ilog2(sbi->sec_per_clus))
> +					     + FAT_START_ENT;
> +	}
> +	brelse(parent_bh);
> +	do {
> +		parent = fat_traverse_cluster(sb,
> +					search_clus, clus_to_match);
> +		if (IS_ERR(parent) || parent)
> +			break;
> +		fatent_init(&fatent);
> +		search_clus = fat_ent_read(sb, &fatent, search_clus);
> +		fatent_brelse(&fatent);
> +		if (search_clus < 0 || search_clus == FAT_ENT_FREE)
> +			break;
> +	} while (search_clus != FAT_ENT_EOF);
> +
> +	return parent;
> +
> +}
> +
> +/*
>   * Find the parent for a directory that is not currently connected to
>   * the filesystem root.
>   *
> @@ -222,10 +336,13 @@ static struct dentry *fat_get_parent(struct dentry *child_dir)
>  	struct buffer_head *bh = NULL;
>  	struct msdos_dir_entry *de;
>  	struct inode *parent_inode = NULL;
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>  
>  	if (!fat_get_dotdot_entry(child_dir->d_inode, &bh, &de)) {
> -		int parent_logstart = fat_get_start(MSDOS_SB(sb), de);
> +		int parent_logstart = fat_get_start(sbi, de);
>  		parent_inode = fat_dget(sb, parent_logstart);
> +		if (!parent_inode && sbi->options.nfs == FAT_NFS_NOSTALE_RO)
> +			parent_inode = fat_rebuild_parent(sb, parent_logstart);
>  	}
>  	brelse(bh);

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2012/12/3, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>
>> This patch enables rebuilding of directory inodes which are not present
>> in the cache.This is done by traversing the disk clusters to find the
>> directory entry of the parent directory and using its i_pos to build the
>> inode.
>> Do this only if the "nostale_ro" nfs mount option is specified.
>
> This became much better than before. However, we have to consolidate the
> code with fat_search_long() finally.
>
> E.g. this version is having the issue already fixed. If there is
> corruption in fat cluster-chain, it lead to infinite
> loop. fat_get_cluster() checks infinite loop by limit.
since, the focus this time was for NFS functionality for FAT (removing
ESTALE error). The changes were made in that context.

Later, we can make the changes as part of code reorganizing which can
be controlled via. Separate patches which do not have any impact on
default functionality and verification can be carried out in that
scope.



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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> 2012/12/3, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>
>>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>>
>>> This patch enables rebuilding of directory inodes which are not present
>>> in the cache.This is done by traversing the disk clusters to find the
>>> directory entry of the parent directory and using its i_pos to build the
>>> inode.
>>> Do this only if the "nostale_ro" nfs mount option is specified.
>>
>> This became much better than before. However, we have to consolidate the
>> code with fat_search_long() finally.
>>
>> E.g. this version is having the issue already fixed. If there is
>> corruption in fat cluster-chain, it lead to infinite
>> loop. fat_get_cluster() checks infinite loop by limit.
> since, the focus this time was for NFS functionality for FAT (removing
> ESTALE error). The changes were made in that context.
>
> Later, we can make the changes as part of code reorganizing which can
> be controlled via. Separate patches which do not have any impact on
> default functionality and verification can be carried out in that
> scope.

Right. But non-production code shouldn't go into linus tree. I meant, we
can test this patch series, but not yet production quality.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2012/12/4, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> 2012/12/3, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>>
>>>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>>>
>>>> This patch enables rebuilding of directory inodes which are not present
>>>> in the cache.This is done by traversing the disk clusters to find the
>>>> directory entry of the parent directory and using its i_pos to build
>>>> the
>>>> inode.
>>>> Do this only if the "nostale_ro" nfs mount option is specified.
>>>
>>> This became much better than before. However, we have to consolidate the
>>> code with fat_search_long() finally.
>>>
>>> E.g. this version is having the issue already fixed. If there is
>>> corruption in fat cluster-chain, it lead to infinite
>>> loop. fat_get_cluster() checks infinite loop by limit.
>> since, the focus this time was for NFS functionality for FAT (removing
>> ESTALE error). The changes were made in that context.
>>
>> Later, we can make the changes as part of code reorganizing which can
>> be controlled via. Separate patches which do not have any impact on
>> default functionality and verification can be carried out in that
>> scope.
>
> Right. But non-production code shouldn't go into linus tree. I meant, we
> can test this patch series, but not yet production quality.
Is there any other thing which seems potential issue than offsetof()?
if yes, which problem didn't lead to production quality do you think ?

+#define FAT_FID_SIZE_WITHOUT_PARENT (offsetof(struct fat_fid, \
+					      parent_i_pos_hi)/4)
Since this expression does not result proper integer value, so will it
be correct to directly put the value like
+#define FAT_FID_SIZE_WITHOUT_PARENT 3

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>>>> This became much better than before. However, we have to consolidate the
>>>> code with fat_search_long() finally.
>>>>
>>>> E.g. this version is having the issue already fixed. If there is
>>>> corruption in fat cluster-chain, it lead to infinite
>>>> loop. fat_get_cluster() checks infinite loop by limit.
>>> since, the focus this time was for NFS functionality for FAT (removing
>>> ESTALE error). The changes were made in that context.
>>>
>>> Later, we can make the changes as part of code reorganizing which can
>>> be controlled via. Separate patches which do not have any impact on
>>> default functionality and verification can be carried out in that
>>> scope.
>>
>> Right. But non-production code shouldn't go into linus tree. I meant, we
>> can test this patch series, but not yet production quality.
> Is there any other thing which seems potential issue than offsetof()?
> if yes, which problem didn't lead to production quality do you think ?
>
> +#define FAT_FID_SIZE_WITHOUT_PARENT (offsetof(struct fat_fid, \
> +					      parent_i_pos_hi)/4)
> Since this expression does not result proper integer value, so will it
> be correct to directly put the value like
> +#define FAT_FID_SIZE_WITHOUT_PARENT 3

The issue is what I explained in the above "E.g.".

Directory traversal logic should be consolidate with fat_search_log().
Otherwise, like this nfs implement, we will introduce already-fixed-problem
again. And we will be bothered to fix same issue in future.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2012/12/5, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>>>> This became much better than before. However, we have to consolidate
>>>>> the
>>>>> code with fat_search_long() finally.
>>>>>
>>>>> E.g. this version is having the issue already fixed. If there is
>>>>> corruption in fat cluster-chain, it lead to infinite
>>>>> loop. fat_get_cluster() checks infinite loop by limit.
>>>> since, the focus this time was for NFS functionality for FAT (removing
>>>> ESTALE error). The changes were made in that context.
>>>>
>>>> Later, we can make the changes as part of code reorganizing which can
>>>> be controlled via. Separate patches which do not have any impact on
>>>> default functionality and verification can be carried out in that
>>>> scope.
>>>
>>> Right. But non-production code shouldn't go into linus tree. I meant, we
>>> can test this patch series, but not yet production quality.
>> Is there any other thing which seems potential issue than offsetof()?
>> if yes, which problem didn't lead to production quality do you think ?
>>
>> +#define FAT_FID_SIZE_WITHOUT_PARENT (offsetof(struct fat_fid, \
>> +					      parent_i_pos_hi)/4)
>> Since this expression does not result proper integer value, so will it
>> be correct to directly put the value like
>> +#define FAT_FID_SIZE_WITHOUT_PARENT 3
>
> The issue is what I explained in the above "E.g.".
>
> Directory traversal logic should be consolidate with fat_search_log().
> Otherwise, like this nfs implement, we will introduce already-fixed-problem
> again. And we will be bothered to fix same issue in future.
Okay, We will check how we can consolidate the 2 paths to avoid any issue.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2012/12/5, Namjae Jeon <linkinjeon@gmail.com>:
> 2012/12/5, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>
>>>>>> This became much better than before. However, we have to consolidate
>>>>>> the
>>>>>> code with fat_search_long() finally.
>>>>>>
>>>>>> E.g. this version is having the issue already fixed. If there is
>>>>>> corruption in fat cluster-chain, it lead to infinite
>>>>>> loop. fat_get_cluster() checks infinite loop by limit.
>>>>> since, the focus this time was for NFS functionality for FAT (removing
>>>>> ESTALE error). The changes were made in that context.
>>>>>
>>>>> Later, we can make the changes as part of code reorganizing which can
>>>>> be controlled via. Separate patches which do not have any impact on
>>>>> default functionality and verification can be carried out in that
>>>>> scope.
>>>>
>>>> Right. But non-production code shouldn't go into linus tree. I meant,
>>>> we
>>>> can test this patch series, but not yet production quality.
>>> Is there any other thing which seems potential issue than offsetof()?
>>> if yes, which problem didn't lead to production quality do you think ?
>>>
>>> +#define FAT_FID_SIZE_WITHOUT_PARENT (offsetof(struct fat_fid, \
>>> +					      parent_i_pos_hi)/4)
>>> Since this expression does not result proper integer value, so will it
>>> be correct to directly put the value like
>>> +#define FAT_FID_SIZE_WITHOUT_PARENT 3
>>
>> The issue is what I explained in the above "E.g.".
>>
>> Directory traversal logic should be consolidate with fat_search_log().
>> Otherwise, like this nfs implement, we will introduce
>> already-fixed-problem
>> again. And we will be bothered to fix same issue in future.
> Okay, We will check how we can consolidate the 2 paths to avoid any issue.
Hi OGAWA.

On checking fat_search_long() logic, it is observed that it performs
name based lookup of the entries in a given directory.
In fat_get_parent(), we do not have such information to use the
existing API to reconstruct the  parent inode.
Could you give me some hint or guide to consolidate smoothly
fat_search_long and current traverse_logic ?

Thanks.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> Okay, We will check how we can consolidate the 2 paths to avoid any issue.
> Hi OGAWA.
>
> On checking fat_search_long() logic, it is observed that it performs
> name based lookup of the entries in a given directory.
> In fat_get_parent(), we do not have such information to use the
> existing API to reconstruct the  parent inode.
> Could you give me some hint or guide to consolidate smoothly
> fat_search_long and current traverse_logic ?

Hm, start with copy of fat_search_long() and refactoring it. With it, we
will be able to avoid the fixed bugs.

After that, we might be able to merge those somehow. Well, I'm not
pretty sure without doing it actually though.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2012/12/20, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> Okay, We will check how we can consolidate the 2 paths to avoid any
>>> issue.
>> Hi OGAWA.
>>
>> On checking fat_search_long() logic, it is observed that it performs
>> name based lookup of the entries in a given directory.
>> In fat_get_parent(), we do not have such information to use the
>> existing API to reconstruct the  parent inode.
>> Could you give me some hint or guide to consolidate smoothly
>> fat_search_long and current traverse_logic ?
>
> Hm, start with copy of fat_search_long() and refactoring it. With it, we
> will be able to avoid the fixed bugs.
>
> After that, we might be able to merge those somehow. Well, I'm not
> pretty sure without doing it actually though.
Hi OGAWA.

When we checked to merge it with fat_search_long, we really did not
find any possibility of code reusing for fat_traverse_cluster.
It will not be proper ? In case of fat_search_long()-> it is
performing a name based lookup in a particular directory.
While for reconnection with parent from NFS ? we do not have the name
of the parent directory. We are relying on ‘i_pos’ ? on disk position
of directory entry.
So, on first request for lookup for “..” (i.e if we call
fat_search_long(child_dir->d_inode, MSDOS_DOTDOT,2,slot_info) it will
return the directory entry for “..” itself.  From this entry we can
read the “log start” which is the starting cluster of the parent
directory, but instead we need the “directory entry” where this is
stored.
So, from this level we need to go further one level up and read the
parent ->parent-> cluster. After reading that cluster, we need to do a
lookup of the “required ipos” in this directory block.
i.e., if the path is A/B/C ? and we call the get_parent() from ‘C’. We
need to read the directory block contents of ‘A’ and from those
‘directory entries' compare the log_start with the log_start value of
B, which was obtained by doing a lookup “..” in C.
So, Instead of it, we suggest we fix the “infinite-loop” condition in
fat_traverse_logic and retain the code.
of course, we will test it with corrupted FATfs.
Please share your thoughts on this.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2012/12/21, Namjae Jeon <linkinjeon@gmail.com>:
> 2012/12/20, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>
>>>> Okay, We will check how we can consolidate the 2 paths to avoid any
>>>> issue.
>>> Hi OGAWA.
>>>
>>> On checking fat_search_long() logic, it is observed that it performs
>>> name based lookup of the entries in a given directory.
>>> In fat_get_parent(), we do not have such information to use the
>>> existing API to reconstruct the  parent inode.
>>> Could you give me some hint or guide to consolidate smoothly
>>> fat_search_long and current traverse_logic ?
>>
>> Hm, start with copy of fat_search_long() and refactoring it. With it, we
>> will be able to avoid the fixed bugs.
>>
>> After that, we might be able to merge those somehow. Well, I'm not
>> pretty sure without doing it actually though.
Hi OGAWA.

When we checked to merge it with fat_search_long, we really did not
find any possibility of code reusing for fat_traverse_cluster.
It will not be proper. In case of fat_search_long()-> it is performing
a name based lookup in a particular directory.
While for reconnection with parent from NFS, we do not have the name
of the parent directory. We are relying on ‘i_pos’ on disk position of
directory entry.
So, on first request for lookup for “..” (i.e if we call
fat_search_long(child_dir->d_inode, MSDOS_DOTDOT,2,slot_info) it will
return the directory entry for “..” itself.  From this entry we can
read the “log start” which is the starting cluster of the parent
directory, but instead we need the “directory entry” where this is
stored.
So, from this level we need to go further one level up and read the
parent ->parent-> cluster. After reading that cluster, we need to do a
lookup of the “required ipos” in this directory block.
i.e., if the path is A/B/C and we call the get_parent() from ‘C’. We
need to read the directory block contents of ‘A’ and from those
‘directory entries' compare the log_start with the log_start value of
B, which was obtained by doing a lookup “..” in C.
So, Instead of it, we suggest we fix the “infinite-loop” condition in
fat_traverse_logic and retain the code.
of course, we will test it with corrupted FATfs.
Please share your thoughts on this.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>>> Hm, start with copy of fat_search_long() and refactoring it. With it, we
>>> will be able to avoid the fixed bugs.
>>>
>>> After that, we might be able to merge those somehow. Well, I'm not
>>> pretty sure without doing it actually though.
> Hi OGAWA.
>
> When we checked to merge it with fat_search_long, we really did not
> find any possibility of code reusing for fat_traverse_cluster.
> It will not be proper. In case of fat_search_long()-> it is performing
> a name based lookup in a particular directory.
> While for reconnection with parent from NFS, we do not have the name
> of the parent directory. We are relying on ‘i_pos’ on disk position of
> directory entry.
> So, on first request for lookup for “..” (i.e if we call
> fat_search_long(child_dir->d_inode, MSDOS_DOTDOT,2,slot_info) it will
> return the directory entry for “..” itself.  From this entry we can
> read the “log start” which is the starting cluster of the parent
> directory, but instead we need the “directory entry” where this is
> stored.
> So, from this level we need to go further one level up and read the
> parent ->parent-> cluster. After reading that cluster, we need to do a
> lookup of the “required ipos” in this directory block.
> i.e., if the path is A/B/C and we call the get_parent() from ‘C’. We
> need to read the directory block contents of ‘A’ and from those
> ‘directory entries' compare the log_start with the log_start value of
> B, which was obtained by doing a lookup “..” in C.
> So, Instead of it, we suggest we fix the “infinite-loop” condition in
> fat_traverse_logic and retain the code.
> of course, we will test it with corrupted FATfs.
> Please share your thoughts on this.

Yes, we can't use fat_search_long() as is, of course. However, we can
share the basic algorithm and code.

The both are doing,

1) traverse the blocks chained by ->i_start.
2) get the record (dirent) from blocks.
3) check the detail of record

The difference is only (3), right? I know, the code has many differences
though. The actual logic are almost same.

And see, e.g., fat_get_cluster() is checking several unexpected
state. We have to care about corrupting data. It is not only
"infinite-loop" case.  And why I'm saying it is better to share code.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2012/12/21, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>>> Hm, start with copy of fat_search_long() and refactoring it. With it,
>>>> we
>>>> will be able to avoid the fixed bugs.
>>>>
>>>> After that, we might be able to merge those somehow. Well, I'm not
>>>> pretty sure without doing it actually though.
>> Hi OGAWA.
>>
>> When we checked to merge it with fat_search_long, we really did not
>> find any possibility of code reusing for fat_traverse_cluster.
>> It will not be proper. In case of fat_search_long()-> it is performing
>> a name based lookup in a particular directory.
>> While for reconnection with parent from NFS, we do not have the name
>> of the parent directory. We are relying on ‘i_pos’ on disk position of
>> directory entry.
>> So, on first request for lookup for “..” (i.e if we call
>> fat_search_long(child_dir->d_inode, MSDOS_DOTDOT,2,slot_info) it will
>> return the directory entry for “..” itself.  From this entry we can
>> read the “log start” which is the starting cluster of the parent
>> directory, but instead we need the “directory entry” where this is
>> stored.
>> So, from this level we need to go further one level up and read the
>> parent ->parent-> cluster. After reading that cluster, we need to do a
>> lookup of the “required ipos” in this directory block.
>> i.e., if the path is A/B/C and we call the get_parent() from ‘C’. We
>> need to read the directory block contents of ‘A’ and from those
>> ‘directory entries' compare the log_start with the log_start value of
>> B, which was obtained by doing a lookup “..” in C.
>> So, Instead of it, we suggest we fix the “infinite-loop” condition in
>> fat_traverse_logic and retain the code.
>> of course, we will test it with corrupted FATfs.
>> Please share your thoughts on this.
>
> Yes, we can't use fat_search_long() as is, of course. However, we can
> share the basic algorithm and code.
>
> The both are doing,
>
> 1) traverse the blocks chained by ->i_start.
> 2) get the record (dirent) from blocks.
> 3) check the detail of record
>
> The difference is only (3), right? I know, the code has many differences
> though. The actual logic are almost same.
>
> And see, e.g., fat_get_cluster() is checking several unexpected
> state. We have to care about corrupting data. It is not only
> "infinite-loop" case.  And why I'm saying it is better to share code.

Regarding unexpected conditions,
When we compare the unexpected conditions in fat_get_cluster:
We get these as conditions which can arise:
fat_end_read() returns:
 1) < 0
 2) FAT_ENT_FREE
 3) FAT_ENT_EOF
And the last is
      4) Preventing the infinite looping of cluster chain.

Our patch is already covering the cases:
 case 1) , 2) =>  if (search_clus < 0 || search_clus == FAT_ENT_FREE)
 about  case 3) => while (search_clus != FAT_ENT_EOF);

while for Case 4:
We can make changes like this:

@@ -179,8 +179,9 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
        struct inode *parent_inode = NULL;
        struct msdos_sb_info *sbi = MSDOS_SB(sb);
        int parent_logstart;
-       int search_clus, clus_to_match;
+       int search_clus, clus_to_match, clus_count = 0;
        sector_t blknr;
+       const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;

        if (!fat_get_dotdot_entry(child_dir->d_inode, &dotdot_bh, &de)) {
                parent_logstart = fat_get_start(sbi, de);
@@ -223,6 +224,14 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
                                                search_clus, clus_to_match);
                                if (IS_ERR(parent_inode) || parent_inode)
                                        break;
+                               if (++clus_count > limit) {
+                                       fat_fs_error_ratelimit(sb,
+                                       "%s: detected the cluster chain loop"
+                                       " while reading directory entries from"
+                                       " cluster %d", __func__, search_clus);
+                                       parent_inode = ERR_PTR(-EIO);
+                                       break;
+                               }
                                fatent_init(&fatent);
                                search_clus = fat_ent_read(sb, &fatent,
                                                           search_clus);
-- 
1.7.7.6

Are we missing anything more in this?

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> Yes, we can't use fat_search_long() as is, of course. However, we can
>> share the basic algorithm and code.
>>
>> The both are doing,
>>
>> 1) traverse the blocks chained by ->i_start.
>> 2) get the record (dirent) from blocks.
>> 3) check the detail of record
>>
>> The difference is only (3), right? I know, the code has many differences
>> though. The actual logic are almost same.
>>
>> And see, e.g., fat_get_cluster() is checking several unexpected
>> state. We have to care about corrupting data. It is not only
>> "infinite-loop" case.  And why I'm saying it is better to share code.
>
> Regarding unexpected conditions,
> When we compare the unexpected conditions in fat_get_cluster:
> We get these as conditions which can arise:
> fat_end_read() returns:
>  1) < 0
>  2) FAT_ENT_FREE
>  3) FAT_ENT_EOF
> And the last is
>       4) Preventing the infinite looping of cluster chain.
>
> Our patch is already covering the cases:
>  case 1) , 2) =>  if (search_clus < 0 || search_clus == FAT_ENT_FREE)
>  about  case 3) => while (search_clus != FAT_ENT_EOF);
>
> while for Case 4:
> We can make changes like this:
>
> @@ -179,8 +179,9 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
>         struct inode *parent_inode = NULL;
>         struct msdos_sb_info *sbi = MSDOS_SB(sb);
>         int parent_logstart;
> -       int search_clus, clus_to_match;
> +       int search_clus, clus_to_match, clus_count = 0;
>         sector_t blknr;
> +       const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;
>
>         if (!fat_get_dotdot_entry(child_dir->d_inode, &dotdot_bh, &de)) {
>                 parent_logstart = fat_get_start(sbi, de);
> @@ -223,6 +224,14 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
>                                                 search_clus, clus_to_match);
>                                 if (IS_ERR(parent_inode) || parent_inode)
>                                         break;
> +                               if (++clus_count > limit) {
> +                                       fat_fs_error_ratelimit(sb,
> +                                       "%s: detected the cluster chain loop"
> +                                       " while reading directory entries from"
> +                                       " cluster %d", __func__, search_clus);
> +                                       parent_inode = ERR_PTR(-EIO);
> +                                       break;
> +                               }
>                                 fatent_init(&fatent);
>                                 search_clus = fat_ent_read(sb, &fatent,
>                                                            search_clus);

and it is copy of fat_get_cluster(), right? It would be sane as start
though, it is true to increase maintain cost, and it makes more similar
to fat_search_long() path. It is why I said, copy at first, and
refactoring after that.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> Yes, we can't use fat_search_long() as is, of course. However, we can
>>> share the basic algorithm and code.
>>>
>>> The both are doing,
>>>
>>> 1) traverse the blocks chained by ->i_start.
>>> 2) get the record (dirent) from blocks.
>>> 3) check the detail of record
>>>
>>> The difference is only (3), right? I know, the code has many differences
>>> though. The actual logic are almost same.
>>>
>>> And see, e.g., fat_get_cluster() is checking several unexpected
>>> state. We have to care about corrupting data. It is not only
>>> "infinite-loop" case.  And why I'm saying it is better to share code.
>>
>> Regarding unexpected conditions,
>> When we compare the unexpected conditions in fat_get_cluster:
>> We get these as conditions which can arise:
>> fat_end_read() returns:
>>  1) < 0
>>  2) FAT_ENT_FREE
>>  3) FAT_ENT_EOF
>> And the last is
>>       4) Preventing the infinite looping of cluster chain.
>>
>> Our patch is already covering the cases:
>>  case 1) , 2) =>  if (search_clus < 0 || search_clus == FAT_ENT_FREE)
>>  about  case 3) => while (search_clus != FAT_ENT_EOF);
>>
>> while for Case 4:
>> We can make changes like this:
>>
>> @@ -179,8 +179,9 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
>>         struct inode *parent_inode = NULL;
>>         struct msdos_sb_info *sbi = MSDOS_SB(sb);
>>         int parent_logstart;
>> -       int search_clus, clus_to_match;
>> +       int search_clus, clus_to_match, clus_count = 0;
>>         sector_t blknr;
>> +       const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;
>>
>>         if (!fat_get_dotdot_entry(child_dir->d_inode, &dotdot_bh, &de)) {
>>                 parent_logstart = fat_get_start(sbi, de);
>> @@ -223,6 +224,14 @@ struct dentry *fat_get_parent(struct dentry *child_dir)
>>                                                 search_clus, clus_to_match);
>>                                 if (IS_ERR(parent_inode) || parent_inode)
>>                                         break;
>> +                               if (++clus_count > limit) {
>> +                                       fat_fs_error_ratelimit(sb,
>> +                                       "%s: detected the cluster chain loop"
>> +                                       " while reading directory entries from"
>> +                                       " cluster %d", __func__, search_clus);
>> +                                       parent_inode = ERR_PTR(-EIO);
>> +                                       break;
>> +                               }
>>                                 fatent_init(&fatent);
>>                                 search_clus = fat_ent_read(sb, &fatent,
>>                                                            search_clus);
>
> and it is copy of fat_get_cluster(), right? It would be sane as start
> though, it is true to increase maintain cost, and it makes more similar
> to fat_search_long() path. It is why I said, copy at first, and
> refactoring after that.

BTW, fat_search_long() was wrong as similar function. Actually it would
be fat_scan(), because we don't care longname entry.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

>
> BTW, fat_search_long() was wrong as similar function. Actually it would
> be fat_scan(), because we don't care longname entry.
Hi OGAWA.

We rewrite patch as your suggestion using dummy inode. Would please
you review below patch code ?

Thanks.

--------------------------------------------------------------------------------------------
Subject: [PATCH] fat (exportfs): rebuild directory-inode if fat_dget()
 fails

This patch enables rebuilding of directory inodes which are not present
in the cache.This is done by traversing the disk clusters to find the
directory entry of the parent directory and using its i_pos to build the
inode.

Do this only if the "nostale_ro" nfs mount option is specified.

---
 fs/fat/dir.c   |   23 +++++++++++++++++++++++
 fs/fat/fat.h   |    3 +++
 fs/fat/inode.c |    2 +-
 fs/fat/nfs.c   |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 695c15c..ac97f34 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -975,6 +975,29 @@ int fat_scan(struct inode *dir, const unsigned char *name,

 EXPORT_SYMBOL_GPL(fat_scan);

+/*
+ * Scans a directory for a given logstart.
+ * Returns an error code or zero.
+ */
+int fat_scan_logstart(struct inode *dir, int i_logstart,
+		      struct fat_slot_info *sinfo)
+{
+	struct super_block *sb = dir->i_sb;
+
+	sinfo->slot_off = 0;
+	sinfo->bh = NULL;
+	while (fat_get_short_entry(dir, &sinfo->slot_off, &sinfo->bh,
+				   &sinfo->de) >= 0) {
+		if (fat_get_start(MSDOS_SB(sb), sinfo->de) == i_logstart) {
+			sinfo->slot_off -= sizeof(*sinfo->de);
+			sinfo->nr_slots = 1;
+			sinfo->i_pos = fat_make_i_pos(sb, sinfo->bh, sinfo->de);
+			return 0;
+		}
+	}
+	return -ENOENT;
+}
+
 static int __fat_remove_entries(struct inode *dir, loff_t pos, int nr_slots)
 {
 	struct super_block *sb = dir->i_sb;
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 73f15b8..d882c01 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -291,6 +291,8 @@ extern int fat_dir_empty(struct inode *dir);
 extern int fat_subdirs(struct inode *dir);
 extern int fat_scan(struct inode *dir, const unsigned char *name,
 		    struct fat_slot_info *sinfo);
+extern int fat_scan_logstart(struct inode *dir, int i_logstart,
+			     struct fat_slot_info *sinfo);
 extern int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
 				struct msdos_dir_entry **de);
 extern int fat_alloc_new_dir(struct inode *dir, struct timespec *ts);
@@ -370,6 +372,7 @@ extern int fat_fill_super(struct super_block *sb,
void *data, int silent,

 extern int fat_flush_inodes(struct super_block *sb, struct inode *i1,
 		            struct inode *i2);
+extern int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de);
 static inline unsigned long fat_dir_hash(int logstart)
 {
 	return hash_32(logstart, FAT_HASH_BITS);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 491320b..c4c286a 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -384,7 +384,7 @@ static int fat_calc_dir_size(struct inode *inode)
 }

 /* doesn't deal with root inode */
-static int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
+int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 	int error;
diff --git a/fs/fat/nfs.c b/fs/fat/nfs.c
index 08ff9fa..e94da33 100644
--- a/fs/fat/nfs.c
+++ b/fs/fat/nfs.c
@@ -213,6 +213,53 @@ static struct dentry
*fat_fh_to_parent_nostale(struct super_block *sb,
 }

 /*
+ * Rebuild the parent for a directory that is not connected
+ *  to the filesystem root
+ */
+static
+struct inode *fat_rebuild_parent(struct super_block *sb, int parent_logstart)
+{
+	int search_clus, clus_to_match;
+	struct msdos_dir_entry *de;
+	struct inode *parent = NULL;
+	struct inode *dummy_grand_parent = NULL;
+	struct fat_slot_info sinfo;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	sector_t blknr = fat_clus_to_blknr(sbi, parent_logstart);
+	struct buffer_head *parent_bh = sb_bread(sb, blknr);
+	if (!parent_bh) {
+		fat_msg(sb, KERN_ERR,
+			"unable to read cluster of parent directory");
+		return NULL;
+	}
+
+	de = (struct msdos_dir_entry *) parent_bh->b_data;
+	clus_to_match = fat_get_start(sbi, &de[0]);
+	search_clus = fat_get_start(sbi, &de[1]);
+
+	dummy_grand_parent = fat_dget(sb, search_clus);
+	if (!dummy_grand_parent) {
+		dummy_grand_parent = new_inode(sb);
+		if (!dummy_grand_parent) {
+			brelse(parent_bh);
+			return parent;
+		}
+
+		dummy_grand_parent->i_ino = iunique(sb, MSDOS_ROOT_INO);
+		fat_fill_inode(dummy_grand_parent, &de[1]);
+		MSDOS_I(dummy_grand_parent)->i_pos = -1;
+	}
+
+	if (!fat_scan_logstart(dummy_grand_parent, clus_to_match, &sinfo))
+		parent = fat_build_inode(sb, sinfo.de, sinfo.i_pos);
+
+	brelse(parent_bh);
+	iput(dummy_grand_parent);
+
+	return parent;
+}
+
+/*
  * Find the parent for a directory that is not currently connected to
  * the filesystem root.
  *
@@ -224,10 +271,13 @@ static struct dentry *fat_get_parent(struct
dentry *child_dir)
 	struct buffer_head *bh = NULL;
 	struct msdos_dir_entry *de;
 	struct inode *parent_inode = NULL;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);

 	if (!fat_get_dotdot_entry(child_dir->d_inode, &bh, &de)) {
-		int parent_logstart = fat_get_start(MSDOS_SB(sb), de);
+		int parent_logstart = fat_get_start(sbi, de);
 		parent_inode = fat_dget(sb, parent_logstart);
+		if (!parent_inode && sbi->options.nfs == FAT_NFS_NOSTALE_RO)
+			parent_inode = fat_rebuild_parent(sb, parent_logstart);
 	}
 	brelse(bh);

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> We rewrite patch as your suggestion using dummy inode. Would please
> you review below patch code ?

Looks like good as initial. Clean and shorter.

Next is, we have to think about race. I.e. if real inode was made, what
happens? Is there no race?

Thanks.

> Subject: [PATCH] fat (exportfs): rebuild directory-inode if fat_dget()
>  fails
>
> This patch enables rebuilding of directory inodes which are not present
> in the cache.This is done by traversing the disk clusters to find the
> directory entry of the parent directory and using its i_pos to build the
> inode.
>
> Do this only if the "nostale_ro" nfs mount option is specified.
>
> ---
>  fs/fat/dir.c   |   23 +++++++++++++++++++++++
>  fs/fat/fat.h   |    3 +++
>  fs/fat/inode.c |    2 +-
>  fs/fat/nfs.c   |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 695c15c..ac97f34 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -975,6 +975,29 @@ int fat_scan(struct inode *dir, const unsigned char *name,
>
>  EXPORT_SYMBOL_GPL(fat_scan);
>
> +/*
> + * Scans a directory for a given logstart.
> + * Returns an error code or zero.
> + */
> +int fat_scan_logstart(struct inode *dir, int i_logstart,
> +		      struct fat_slot_info *sinfo)
> +{
> +	struct super_block *sb = dir->i_sb;
> +
> +	sinfo->slot_off = 0;
> +	sinfo->bh = NULL;
> +	while (fat_get_short_entry(dir, &sinfo->slot_off, &sinfo->bh,
> +				   &sinfo->de) >= 0) {
> +		if (fat_get_start(MSDOS_SB(sb), sinfo->de) == i_logstart) {
> +			sinfo->slot_off -= sizeof(*sinfo->de);
> +			sinfo->nr_slots = 1;
> +			sinfo->i_pos = fat_make_i_pos(sb, sinfo->bh, sinfo->de);
> +			return 0;
> +		}
> +	}
> +	return -ENOENT;
> +}
> +
>  static int __fat_remove_entries(struct inode *dir, loff_t pos, int nr_slots)
>  {
>  	struct super_block *sb = dir->i_sb;
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 73f15b8..d882c01 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -291,6 +291,8 @@ extern int fat_dir_empty(struct inode *dir);
>  extern int fat_subdirs(struct inode *dir);
>  extern int fat_scan(struct inode *dir, const unsigned char *name,
>  		    struct fat_slot_info *sinfo);
> +extern int fat_scan_logstart(struct inode *dir, int i_logstart,
> +			     struct fat_slot_info *sinfo);
>  extern int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
>  				struct msdos_dir_entry **de);
>  extern int fat_alloc_new_dir(struct inode *dir, struct timespec *ts);
> @@ -370,6 +372,7 @@ extern int fat_fill_super(struct super_block *sb,
> void *data, int silent,
>
>  extern int fat_flush_inodes(struct super_block *sb, struct inode *i1,
>  		            struct inode *i2);
> +extern int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de);
>  static inline unsigned long fat_dir_hash(int logstart)
>  {
>  	return hash_32(logstart, FAT_HASH_BITS);
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 491320b..c4c286a 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -384,7 +384,7 @@ static int fat_calc_dir_size(struct inode *inode)
>  }
>
>  /* doesn't deal with root inode */
> -static int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
> +int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
>  {
>  	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
>  	int error;
> diff --git a/fs/fat/nfs.c b/fs/fat/nfs.c
> index 08ff9fa..e94da33 100644
> --- a/fs/fat/nfs.c
> +++ b/fs/fat/nfs.c
> @@ -213,6 +213,53 @@ static struct dentry
> *fat_fh_to_parent_nostale(struct super_block *sb,
>  }
>
>  /*
> + * Rebuild the parent for a directory that is not connected
> + *  to the filesystem root
> + */
> +static
> +struct inode *fat_rebuild_parent(struct super_block *sb, int parent_logstart)
> +{
> +	int search_clus, clus_to_match;
> +	struct msdos_dir_entry *de;
> +	struct inode *parent = NULL;
> +	struct inode *dummy_grand_parent = NULL;
> +	struct fat_slot_info sinfo;
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	sector_t blknr = fat_clus_to_blknr(sbi, parent_logstart);
> +	struct buffer_head *parent_bh = sb_bread(sb, blknr);
> +	if (!parent_bh) {
> +		fat_msg(sb, KERN_ERR,
> +			"unable to read cluster of parent directory");
> +		return NULL;
> +	}
> +
> +	de = (struct msdos_dir_entry *) parent_bh->b_data;
> +	clus_to_match = fat_get_start(sbi, &de[0]);
> +	search_clus = fat_get_start(sbi, &de[1]);
> +
> +	dummy_grand_parent = fat_dget(sb, search_clus);
> +	if (!dummy_grand_parent) {
> +		dummy_grand_parent = new_inode(sb);
> +		if (!dummy_grand_parent) {
> +			brelse(parent_bh);
> +			return parent;
> +		}
> +
> +		dummy_grand_parent->i_ino = iunique(sb, MSDOS_ROOT_INO);
> +		fat_fill_inode(dummy_grand_parent, &de[1]);
> +		MSDOS_I(dummy_grand_parent)->i_pos = -1;
> +	}
> +
> +	if (!fat_scan_logstart(dummy_grand_parent, clus_to_match, &sinfo))
> +		parent = fat_build_inode(sb, sinfo.de, sinfo.i_pos);
> +
> +	brelse(parent_bh);
> +	iput(dummy_grand_parent);
> +
> +	return parent;
> +}
> +
> +/*
>   * Find the parent for a directory that is not currently connected to
>   * the filesystem root.
>   *
> @@ -224,10 +271,13 @@ static struct dentry *fat_get_parent(struct
> dentry *child_dir)
>  	struct buffer_head *bh = NULL;
>  	struct msdos_dir_entry *de;
>  	struct inode *parent_inode = NULL;
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>
>  	if (!fat_get_dotdot_entry(child_dir->d_inode, &bh, &de)) {
> -		int parent_logstart = fat_get_start(MSDOS_SB(sb), de);
> +		int parent_logstart = fat_get_start(sbi, de);
>  		parent_inode = fat_dget(sb, parent_logstart);
> +		if (!parent_inode && sbi->options.nfs == FAT_NFS_NOSTALE_RO)
> +			parent_inode = fat_rebuild_parent(sb, parent_logstart);
>  	}
>  	brelse(bh);

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2013/1/20, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> We rewrite patch as your suggestion using dummy inode. Would please
>> you review below patch code ?
>
> Looks like good as initial. Clean and shorter.
>
> Next is, we have to think about race. I.e. if real inode was made, what
> happens? Is there no race?
Hi OGAWA.

Although checking several routines to check hang case you said, I
didn't find anything.
And There is no any race on test result also. Am I missing something ?
Let me know your opinion.

Thanks.
>
> Thanks.
>
>> Subject: [PATCH] fat (exportfs): rebuild directory-inode if fat_dget()
>>  fails
>>
>> This patch enables rebuilding of directory inodes which are not present
>> in the cache.This is done by traversing the disk clusters to find the
>> directory entry of the parent directory and using its i_pos to build the
>> inode.
>>
>> Do this only if the "nostale_ro" nfs mount option is specified.
>>
>> ---
>>  fs/fat/dir.c   |   23 +++++++++++++++++++++++
>>  fs/fat/fat.h   |    3 +++
>>  fs/fat/inode.c |    2 +-
>>  fs/fat/nfs.c   |   52
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  4 files changed, 78 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
>> index 695c15c..ac97f34 100644
>> --- a/fs/fat/dir.c
>> +++ b/fs/fat/dir.c
>> @@ -975,6 +975,29 @@ int fat_scan(struct inode *dir, const unsigned char
>> *name,
>>
>>  EXPORT_SYMBOL_GPL(fat_scan);
>>
>> +/*
>> + * Scans a directory for a given logstart.
>> + * Returns an error code or zero.
>> + */
>> +int fat_scan_logstart(struct inode *dir, int i_logstart,
>> +		      struct fat_slot_info *sinfo)
>> +{
>> +	struct super_block *sb = dir->i_sb;
>> +
>> +	sinfo->slot_off = 0;
>> +	sinfo->bh = NULL;
>> +	while (fat_get_short_entry(dir, &sinfo->slot_off, &sinfo->bh,
>> +				   &sinfo->de) >= 0) {
>> +		if (fat_get_start(MSDOS_SB(sb), sinfo->de) == i_logstart) {
>> +			sinfo->slot_off -= sizeof(*sinfo->de);
>> +			sinfo->nr_slots = 1;
>> +			sinfo->i_pos = fat_make_i_pos(sb, sinfo->bh, sinfo->de);
>> +			return 0;
>> +		}
>> +	}
>> +	return -ENOENT;
>> +}
>> +
>>  static int __fat_remove_entries(struct inode *dir, loff_t pos, int
>> nr_slots)
>>  {
>>  	struct super_block *sb = dir->i_sb;
>> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
>> index 73f15b8..d882c01 100644
>> --- a/fs/fat/fat.h
>> +++ b/fs/fat/fat.h
>> @@ -291,6 +291,8 @@ extern int fat_dir_empty(struct inode *dir);
>>  extern int fat_subdirs(struct inode *dir);
>>  extern int fat_scan(struct inode *dir, const unsigned char *name,
>>  		    struct fat_slot_info *sinfo);
>> +extern int fat_scan_logstart(struct inode *dir, int i_logstart,
>> +			     struct fat_slot_info *sinfo);
>>  extern int fat_get_dotdot_entry(struct inode *dir, struct buffer_head
>> **bh,
>>  				struct msdos_dir_entry **de);
>>  extern int fat_alloc_new_dir(struct inode *dir, struct timespec *ts);
>> @@ -370,6 +372,7 @@ extern int fat_fill_super(struct super_block *sb,
>> void *data, int silent,
>>
>>  extern int fat_flush_inodes(struct super_block *sb, struct inode *i1,
>>  		            struct inode *i2);
>> +extern int fat_fill_inode(struct inode *inode, struct msdos_dir_entry
>> *de);
>>  static inline unsigned long fat_dir_hash(int logstart)
>>  {
>>  	return hash_32(logstart, FAT_HASH_BITS);
>> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
>> index 491320b..c4c286a 100644
>> --- a/fs/fat/inode.c
>> +++ b/fs/fat/inode.c
>> @@ -384,7 +384,7 @@ static int fat_calc_dir_size(struct inode *inode)
>>  }
>>
>>  /* doesn't deal with root inode */
>> -static int fat_fill_inode(struct inode *inode, struct msdos_dir_entry
>> *de)
>> +int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
>>  {
>>  	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
>>  	int error;
>> diff --git a/fs/fat/nfs.c b/fs/fat/nfs.c
>> index 08ff9fa..e94da33 100644
>> --- a/fs/fat/nfs.c
>> +++ b/fs/fat/nfs.c
>> @@ -213,6 +213,53 @@ static struct dentry
>> *fat_fh_to_parent_nostale(struct super_block *sb,
>>  }
>>
>>  /*
>> + * Rebuild the parent for a directory that is not connected
>> + *  to the filesystem root
>> + */
>> +static
>> +struct inode *fat_rebuild_parent(struct super_block *sb, int
>> parent_logstart)
>> +{
>> +	int search_clus, clus_to_match;
>> +	struct msdos_dir_entry *de;
>> +	struct inode *parent = NULL;
>> +	struct inode *dummy_grand_parent = NULL;
>> +	struct fat_slot_info sinfo;
>> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>> +	sector_t blknr = fat_clus_to_blknr(sbi, parent_logstart);
>> +	struct buffer_head *parent_bh = sb_bread(sb, blknr);
>> +	if (!parent_bh) {
>> +		fat_msg(sb, KERN_ERR,
>> +			"unable to read cluster of parent directory");
>> +		return NULL;
>> +	}
>> +
>> +	de = (struct msdos_dir_entry *) parent_bh->b_data;
>> +	clus_to_match = fat_get_start(sbi, &de[0]);
>> +	search_clus = fat_get_start(sbi, &de[1]);
>> +
>> +	dummy_grand_parent = fat_dget(sb, search_clus);
>> +	if (!dummy_grand_parent) {
>> +		dummy_grand_parent = new_inode(sb);
>> +		if (!dummy_grand_parent) {
>> +			brelse(parent_bh);
>> +			return parent;
>> +		}
>> +
>> +		dummy_grand_parent->i_ino = iunique(sb, MSDOS_ROOT_INO);
>> +		fat_fill_inode(dummy_grand_parent, &de[1]);
>> +		MSDOS_I(dummy_grand_parent)->i_pos = -1;
>> +	}
>> +
>> +	if (!fat_scan_logstart(dummy_grand_parent, clus_to_match, &sinfo))
>> +		parent = fat_build_inode(sb, sinfo.de, sinfo.i_pos);
>> +
>> +	brelse(parent_bh);
>> +	iput(dummy_grand_parent);
>> +
>> +	return parent;
>> +}
>> +
>> +/*
>>   * Find the parent for a directory that is not currently connected to
>>   * the filesystem root.
>>   *
>> @@ -224,10 +271,13 @@ static struct dentry *fat_get_parent(struct
>> dentry *child_dir)
>>  	struct buffer_head *bh = NULL;
>>  	struct msdos_dir_entry *de;
>>  	struct inode *parent_inode = NULL;
>> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>>
>>  	if (!fat_get_dotdot_entry(child_dir->d_inode, &bh, &de)) {
>> -		int parent_logstart = fat_get_start(MSDOS_SB(sb), de);
>> +		int parent_logstart = fat_get_start(sbi, de);
>>  		parent_inode = fat_dget(sb, parent_logstart);
>> +		if (!parent_inode && sbi->options.nfs == FAT_NFS_NOSTALE_RO)
>> +			parent_inode = fat_rebuild_parent(sb, parent_logstart);
>>  	}
>>  	brelse(bh);
>
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> 2013/1/20, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>
>>> We rewrite patch as your suggestion using dummy inode. Would please
>>> you review below patch code ?
>>
>> Looks like good as initial. Clean and shorter.
>>
>> Next is, we have to think about race. I.e. if real inode was made, what
>> happens? Is there no race?
> Hi OGAWA.
>
> Although checking several routines to check hang case you said, I
> didn't find anything.
> And There is no any race on test result also. Am I missing something ?
> Let me know your opinion.

Hm, it's read-only. So, there may not be race for now, I'm sure there is
race on write path though.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2013/1/26, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> 2013/1/20, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>>
>>>> We rewrite patch as your suggestion using dummy inode. Would please
>>>> you review below patch code ?
>>>
>>> Looks like good as initial. Clean and shorter.
>>>
>>> Next is, we have to think about race. I.e. if real inode was made, what
>>> happens? Is there no race?
>> Hi OGAWA.
>>
>> Although checking several routines to check hang case you said, I
>> didn't find anything.
>> And There is no any race on test result also. Am I missing something ?
>> Let me know your opinion.
>
> Hm, it's read-only. So, there may not be race for now, I'm sure there is
> race on write path though.
Yes, right. We checked/tested on read-only.
Maybe have you found race with rename and unlink ?
If yes, I think we can fix this issue with lock like this.

+                       mutex_lock(&MSDOS_SB(sb)->s_lock);
                         parent_inode = fat_rebuild_parent(sb, parent_logstart);
+                       mutex_unlock(&MSDOS_SB(sb)->s_lock);

Let me know your opinion.
Thanks.
>
> Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>>> Although checking several routines to check hang case you said, I
>>> didn't find anything.
>>> And There is no any race on test result also. Am I missing something ?
>>> Let me know your opinion.
>>
>> Hm, it's read-only. So, there may not be race for now, I'm sure there is
>> race on write path though.
> Yes, right. We checked/tested on read-only.
> Maybe have you found race with rename and unlink ?
> If yes, I think we can fix this issue with lock like this.
>
> +                       mutex_lock(&MSDOS_SB(sb)->s_lock);
>                          parent_inode = fat_rebuild_parent(sb, parent_logstart);
> +                       mutex_unlock(&MSDOS_SB(sb)->s_lock);

It is any changes to directory. ->s_lock is not preferred. We need only
per-directory lock (i.e. dir->i_mutex).

To do this, we need more bigger changes though. E.g. register temporary
inode to central list. Then, find it when building real inode. If found
temporary, grab it, and make update it as real inode.

Yes, this is a bit complex. But we would need something like this for
write support.

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

Re: [PATCH v5 7/8] fat (exportfs): rebuild directory-inode if fat_dget() fails

[Namjae Jeon]

2013/1/28, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>>> Although checking several routines to check hang case you said, I
>>>> didn't find anything.
>>>> And There is no any race on test result also. Am I missing something ?
>>>> Let me know your opinion.
>>>
>>> Hm, it's read-only. So, there may not be race for now, I'm sure there is
>>> race on write path though.
>> Yes, right. We checked/tested on read-only.
>> Maybe have you found race with rename and unlink ?
>> If yes, I think we can fix this issue with lock like this.
>>
>> +                       mutex_lock(&MSDOS_SB(sb)->s_lock);
>>                          parent_inode = fat_rebuild_parent(sb,
>> parent_logstart);
>> +                       mutex_unlock(&MSDOS_SB(sb)->s_lock);
>
> It is any changes to directory. ->s_lock is not preferred. We need only
> per-directory lock (i.e. dir->i_mutex).
>
> To do this, we need more bigger changes though. E.g. register temporary
> inode to central list. Then, find it when building real inode. If found
> temporary, grab it, and make update it as real inode.
>
> Yes, this is a bit complex. But we would need something like this for
> write support.

First Thanks for review and help. We will try to fix it as your
suggestion now for the Write path.
There is one suggestion. As per discussion before, your suggestion was
that, we will merge read-only support for FAT exportfs first.
And the current patch set has no issues for read-only.
So If you accept, Can I try to re-send current patch-set (read-only)
first? And we will then additionally start to fix issues related with
write path step by step.
Let me know your opinion.

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