Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
2012-12-13 (목), 07:44 +0900, Namjae Jeon: > 2012/12/13, Leon Romanovsky : > > On Wed, Dec 12, 2012 at 8:50 AM, Fubo Chen wrote: > >> On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky wrote: > >>> On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen wrote: > > On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon > 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 signature.asc Description: This is a digitally signed message part
Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
2012/12/13, Leon Romanovsky : > On Wed, Dec 12, 2012 at 8:50 AM, Fubo Chen wrote: >> On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky wrote: >>> On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen wrote: On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon 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 | l...@leon.nu > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
On Wed, Dec 12, 2012 at 8:50 AM, Fubo Chen wrote: > On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky wrote: >> On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen wrote: >>> >>> On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon 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 | l...@leon.nu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
On Wed, Dec 12, 2012 at 5:12 PM, Leon Romanovsky wrote: > On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen wrote: >> >> On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
On Wed, Dec 12, 2012 at 7:05 AM, Fubo Chen wrote: > > On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon 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 majord...@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 | l...@leon.nu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
On Wed, Dec 12, 2012 at 3:37 PM, Namjae Jeon 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
From: Namjae Jeon
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
Signed-off-by: Amit Sahrawat
Signed-off-by: Jaegeuk Kim
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
2012/12/12, Jaegeuk Kim :
> 2012-12-12 (수), 00:10 +0900, Namjae Jeon:
>> From: Namjae Jeon
>>
>> 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
>> Signed-off-by: Amit Sahrawat
>> ---
>> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
2012-12-12 (수), 00:10 +0900, Namjae Jeon:
> From: Namjae Jeon
>
> 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
> Signed-off-by: Amit Sahrawat
> ---
> 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
signature.asc
Description: This is a digitally signed message part
[PATCH] f2fs: fix up f2fs_get_parent issue to retrieve correct parent inode number
From: Namjae Jeon
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
Signed-off-by: Amit Sahrawat
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
