[PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Namjae Jeon]

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

Test Case:
[NFS Client]
ls -lR .

[NFS Server]
while [ 1 ]
do
echo 3 > /proc/sys/vm/drop_caches
done

Error on NFS Client: "No such file or directory"

When cache is dropped at the server, it results in lookup failure at the
NFS client due to non-connection with the parent. The default path is it
initiates a lookup by calculating the hash value for the name, even though
the hash values stored on the disk for "." and ".." is maintained as zero,
which results in failure from find_in_block due to not matching HASH values.
Fix up, by using the correct hashing values for these entries.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/dir.c  |    4 ++--
 fs/f2fs/hash.c |    4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index b4e24f3..e1f66df 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -540,13 +540,13 @@ int f2fs_make_empty(struct inode *inode, struct inode *parent)
 
 	de = &dentry_blk->dentry[0];
 	de->name_len = cpu_to_le16(1);
-	de->hash_code = 0;
+	de->hash_code = f2fs_dentry_hash(".", 1);
 	de->ino = cpu_to_le32(inode->i_ino);
 	memcpy(dentry_blk->filename[0], ".", 1);
 	set_de_type(de, inode);
 
 	de = &dentry_blk->dentry[1];
-	de->hash_code = 0;
+	de->hash_code = f2fs_dentry_hash("..", 2);
 	de->name_len = cpu_to_le16(2);
 	de->ino = cpu_to_le32(parent->i_ino);
 	memcpy(dentry_blk->filename[1], "..", 2);
diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c
index a60f042..30ca4a2 100644
--- a/fs/f2fs/hash.c
+++ b/fs/f2fs/hash.c
@@ -76,6 +76,10 @@ f2fs_hash_t f2fs_dentry_hash(const char *name, int len)
 	const char *p;
 	__u32 in[8], buf[4];
 
+	/* dot and dotdot dentries should have zero-value hash code */
+	if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
+		return 0;
+
 	/* Initialize the default seed for the hash checksum functions */
 	buf[0] = 0x67452301;
 	buf[1] = 0xefcdab89;
-- 
1.7.9.5

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Fubo Chen]

On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:
> +       /* dot and dotdot dentries should have zero-value hash code */
> +       if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
> +               return 0;

That looks suspicious. If memcmp(name, "..", 2) == 0 then always
memcmp(name, ".", 1) == 0. Why two tests ?

Fubo.

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Leon Romanovsky]

On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
>
> On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:
> > +       /* dot and dotdot dentries should have zero-value hash code */
> > +       if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
> > +               return 0;
>
> That looks suspicious. If memcmp(name, "..", 2) == 0 then always
> memcmp(name, ".", 1) == 0. Why two tests ?
It is not the case vice versa, so you still need to do two checks.
You need to distinguish dot(.), dotdot(..) and something with dot at
the beginning (for example - .o)

>
> Fubo.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Fubo Chen]

On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky <leon@leon.nu> wrote:
> On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
>>
>> On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>> > +       /* dot and dotdot dentries should have zero-value hash code */
>> > +       if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
>> > +               return 0;
>>
>> That looks suspicious. If memcmp(name, "..", 2) == 0 then always
>> memcmp(name, ".", 1) == 0. Why two tests ?
>
> It is not the case vice versa, so you still need to do two checks.
> You need to distinguish dot(.), dotdot(..) and something with dot at
> the beginning (for example - .o)

Thanks for replying. I understand that the intention is what you
explained. But to me the code says something else: "if the first byte
of name is a dot, return 0". Did I see that correctly ?

Fubo.

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Leon Romanovsky]

On Wed, Dec 12, 2012 at 8:50 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
> On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky <leon@leon.nu> wrote:
>> On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
>>>
>>> On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:
>>> > +       /* dot and dotdot dentries should have zero-value hash code */
>>> > +       if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
>>> > +               return 0;
>>>
>>> That looks suspicious. If memcmp(name, "..", 2) == 0 then always
>>> memcmp(name, ".", 1) == 0. Why two tests ?
>>
>> It is not the case vice versa, so you still need to do two checks.
>> You need to distinguish dot(.), dotdot(..) and something with dot at
>> the beginning (for example - .o)
>
> Thanks for replying. I understand that the intention is what you
> explained. But to me the code says something else: "if the first byte
> of name is a dot, return 0". Did I see that correctly ?
Excellent catch, I agree with you It will also return 0 for every file
which starts from the dot.
The right solution must take name length into account.

>
> Fubo.



-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Namjae Jeon]

2012/12/13, Leon Romanovsky <leon@leon.nu>:
> On Wed, Dec 12, 2012 at 8:50 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
>> On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky <leon@leon.nu> wrote:
>>> On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
>>>>
>>>> On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon <linkinjeon@gmail.com>
>>>> wrote:
>>>> > +       /* dot and dotdot dentries should have zero-value hash code
>>>> > */
>>>> > +       if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
>>>> > +               return 0;
>>>>
>>>> That looks suspicious. If memcmp(name, "..", 2) == 0 then always
>>>> memcmp(name, ".", 1) == 0. Why two tests ?
>>>
>>> It is not the case vice versa, so you still need to do two checks.
>>> You need to distinguish dot(.), dotdot(..) and something with dot at
>>> the beginning (for example - .o)
>>
>> Thanks for replying. I understand that the intention is what you
>> explained. But to me the code says something else: "if the first byte
>> of name is a dot, return 0". Did I see that correctly ?
> Excellent catch, I agree with you It will also return 0 for every file
> which starts from the dot.
> The right solution must take name length into account.
Hi.
Agree. I will change it on v2 patch.
Thanks for review.
>
>>
>> Fubo.
>
>
>
> --
> Leon Romanovsky | Independent Linux Consultant
>         www.leon.nu | leon@leon.nu
>

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

2012-12-13 (목), 07:44 +0900, Namjae Jeon:
> 2012/12/13, Leon Romanovsky <leon@leon.nu>:
> > On Wed, Dec 12, 2012 at 8:50 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
> >> On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky <leon@leon.nu> wrote:
> >>> On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen <fubo.chen@gmail.com> wrote:
> >>>>
> >>>> On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon <linkinjeon@gmail.com>
> >>>> wrote:
> >>>> > +       /* dot and dotdot dentries should have zero-value hash code
> >>>> > */
> >>>> > +       if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
> >>>> > +               return 0;
> >>>>
> >>>> That looks suspicious. If memcmp(name, "..", 2) == 0 then always
> >>>> memcmp(name, ".", 1) == 0. Why two tests ?
> >>>
> >>> It is not the case vice versa, so you still need to do two checks.
> >>> You need to distinguish dot(.), dotdot(..) and something with dot at
> >>> the beginning (for example - .o)
> >>
> >> Thanks for replying. I understand that the intention is what you
> >> explained. But to me the code says something else: "if the first byte
> >> of name is a dot, return 0". Did I see that correctly ?
> > Excellent catch, I agree with you It will also return 0 for every file
> > which starts from the dot.
> > The right solution must take name length into account.
> Hi.
> Agree. I will change it on v2 patch.
> Thanks for review.

Nice catch!
I made a mistake initially before. Sorry Namjae.
Thanks to all of you.

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Namjae Jeon]

2012/12/12, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> 2012-12-12 (수), 00:10 +0900, Namjae Jeon:
>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>
>> Test Case:
>> [NFS Client]
>> ls -lR .
>>
>> [NFS Server]
>> while [ 1 ]
>> do
>> echo 3 > /proc/sys/vm/drop_caches
>> done
>>
>> Error on NFS Client: "No such file or directory"
>>
>> When cache is dropped at the server, it results in lookup failure at the
>> NFS client due to non-connection with the parent. The default path is it
>> initiates a lookup by calculating the hash value for the name, even
>> though
>> the hash values stored on the disk for "." and ".." is maintained as
>> zero,
>> which results in failure from find_in_block due to not matching HASH
>> values.
>> Fix up, by using the correct hashing values for these entries and
>> then initiating lookup request.
>>
>> Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
>> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
>> ---
>>  fs/f2fs/dir.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index b4e24f3..6cf39db 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -184,7 +184,7 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode
>> *dir,
>>  	int namelen = child->len;
>>  	unsigned long npages = dir_blocks(dir);
>>  	struct f2fs_dir_entry *de = NULL;
>> -	f2fs_hash_t name_hash;
>> +	f2fs_hash_t name_hash = 0;
>>  	unsigned int max_depth;
>>  	unsigned int level;
>>
>> @@ -193,7 +193,8 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode
>> *dir,
>>
>>  	*res_page = NULL;
>>
>> -	name_hash = f2fs_dentry_hash(name, namelen);
>> +	if (strcmp(name, ".") && (strcmp(name, "..")))
>> +		name_hash = f2fs_dentry_hash(name, namelen);
>
> Got it.
> For code consistency, could you rewrite the patch like below?
> Thanks,
Sure, I will resend the patch at tonight.
Thanks!
>
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index b4e24f3..e1f66df 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -540,13 +540,13 @@ int f2fs_make_empty(struct inode *inode, struct
> inode *parent)
>
>  	de = &dentry_blk->dentry[0];
>  	de->name_len = cpu_to_le16(1);
> -	de->hash_code = 0;
> +	de->hash_code = f2fs_dentry_hash(".", 1);
>  	de->ino = cpu_to_le32(inode->i_ino);
>  	memcpy(dentry_blk->filename[0], ".", 1);
>  	set_de_type(de, inode);
>
>  	de = &dentry_blk->dentry[1];
> -	de->hash_code = 0;
> +	de->hash_code = f2fs_dentry_hash("..", 2);
>  	de->name_len = cpu_to_le16(2);
>  	de->ino = cpu_to_le32(parent->i_ino);
>  	memcpy(dentry_blk->filename[1], "..", 2);
> diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c
> index a60f042..30ca4a2 100644
> --- a/fs/f2fs/hash.c
> +++ b/fs/f2fs/hash.c
> @@ -76,6 +76,10 @@ f2fs_hash_t f2fs_dentry_hash(const char *name, int
> len)
>  	const char *p;
>  	__u32 in[8], buf[4];
>
> +	/* dot and dotdot dentries should have zero-value hash code */
> +	if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
> +		return 0;
> +
>  	/* Initialize the default seed for the hash checksum functions */
>  	buf[0] = 0x67452301;
>  	buf[1] = 0xefcdab89;
> --
> 1.8.0.1.250.gb7973fb
>
>
>
> --
> Jaegeuk Kim
> Samsung
>

Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 3012 bytes --]

2012-12-12 (수), 00:10 +0900, Namjae Jeon:
> From: Namjae Jeon <namjae.jeon@samsung.com>
> 
> Test Case:
> [NFS Client]
> ls -lR .
> 
> [NFS Server]
> while [ 1 ]
> do
> echo 3 > /proc/sys/vm/drop_caches
> done
> 
> Error on NFS Client: "No such file or directory"
> 
> When cache is dropped at the server, it results in lookup failure at the
> NFS client due to non-connection with the parent. The default path is it
> initiates a lookup by calculating the hash value for the name, even though
> the hash values stored on the disk for "." and ".." is maintained as zero,
> which results in failure from find_in_block due to not matching HASH values.
> Fix up, by using the correct hashing values for these entries and
> then initiating lookup request.
> 
> Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
> ---
>  fs/f2fs/dir.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index b4e24f3..6cf39db 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -184,7 +184,7 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
>  	int namelen = child->len;
>  	unsigned long npages = dir_blocks(dir);
>  	struct f2fs_dir_entry *de = NULL;
> -	f2fs_hash_t name_hash;
> +	f2fs_hash_t name_hash = 0;
>  	unsigned int max_depth;
>  	unsigned int level;
>  
> @@ -193,7 +193,8 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
>  
>  	*res_page = NULL;
>  
> -	name_hash = f2fs_dentry_hash(name, namelen);
> +	if (strcmp(name, ".") && (strcmp(name, "..")))
> +		name_hash = f2fs_dentry_hash(name, namelen);

Got it.
For code consistency, could you rewrite the patch like below?
Thanks,

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index b4e24f3..e1f66df 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -540,13 +540,13 @@ int f2fs_make_empty(struct inode *inode, struct
inode *parent)
 
 	de = &dentry_blk->dentry[0];
 	de->name_len = cpu_to_le16(1);
-	de->hash_code = 0;
+	de->hash_code = f2fs_dentry_hash(".", 1);
 	de->ino = cpu_to_le32(inode->i_ino);
 	memcpy(dentry_blk->filename[0], ".", 1);
 	set_de_type(de, inode);
 
 	de = &dentry_blk->dentry[1];
-	de->hash_code = 0;
+	de->hash_code = f2fs_dentry_hash("..", 2);
 	de->name_len = cpu_to_le16(2);
 	de->ino = cpu_to_le32(parent->i_ino);
 	memcpy(dentry_blk->filename[1], "..", 2);
diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c
index a60f042..30ca4a2 100644
--- a/fs/f2fs/hash.c
+++ b/fs/f2fs/hash.c
@@ -76,6 +76,10 @@ f2fs_hash_t f2fs_dentry_hash(const char *name, int
len)
 	const char *p;
 	__u32 in[8], buf[4];
 
+	/* dot and dotdot dentries should have zero-value hash code */
+	if (!memcmp(name, ".", 1) || !memcmp(name, "..", 2))
+		return 0;
+
 	/* Initialize the default seed for the hash checksum functions */
 	buf[0] = 0x67452301;
 	buf[1] = 0xefcdab89;
-- 
1.8.0.1.250.gb7973fb



-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number

[Namjae Jeon]

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

Test Case:
[NFS Client]
ls -lR .

[NFS Server]
while [ 1 ]
do
echo 3 > /proc/sys/vm/drop_caches
done

Error on NFS Client: "No such file or directory"

When cache is dropped at the server, it results in lookup failure at the
NFS client due to non-connection with the parent. The default path is it
initiates a lookup by calculating the hash value for the name, even though
the hash values stored on the disk for "." and ".." is maintained as zero,
which results in failure from find_in_block due to not matching HASH values.
Fix up, by using the correct hashing values for these entries and
then initiating lookup request.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 fs/f2fs/dir.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index b4e24f3..6cf39db 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -184,7 +184,7 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 	int namelen = child->len;
 	unsigned long npages = dir_blocks(dir);
 	struct f2fs_dir_entry *de = NULL;
-	f2fs_hash_t name_hash;
+	f2fs_hash_t name_hash = 0;
 	unsigned int max_depth;
 	unsigned int level;
 
@@ -193,7 +193,8 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 
 	*res_page = NULL;
 
-	name_hash = f2fs_dentry_hash(name, namelen);
+	if (strcmp(name, ".") && (strcmp(name, "..")))
+		name_hash = f2fs_dentry_hash(name, namelen);
 	max_depth = F2FS_I(dir)->i_current_depth;
 
 	for (level = 0; level < max_depth; level++) {
-- 
1.7.9.5