[PATCH v2] fat: eliminate iterations in fat_search_long and __fat_readdir in case of EOD

[PATCH v2] fat: eliminate iterations in fat_search_long and __fat_readdir in case of EOD

[Namjae Jeon]

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

When doing lookups via fat_search_long(), we can stop checking for
further entries if we detect End of Directory, i.e. if (de->name[0] ==
0x00).The current code traverses the cluster chain of a directory until a hit
is found or till the last cluster for that directory, ignoring the EOD
mark. Fix this.

Likewise,when readdir(3) is called, we can stop checking for further
entries in __fat_readdir() when we hit EOD.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Ravishankar N <ravi.n1@samsung.com>
---
 fs/fat/dir.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 58bf744..78dabf00 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -484,10 +484,10 @@ parse_record:
 		nr_slots = 0;
 		if (de->name[0] == DELETED_FLAG)
 			continue;
+		if (!de->name[0])
+			goto end_of_dir;
 		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
 			continue;
-		if (de->attr != ATTR_EXT && IS_FREE(de->name))
-			continue;
 		if (de->attr == ATTR_EXT) {
 			int status = fat_parse_long(inode, &cpos, &bh, &de,
 						    &unicode, &nr_slots);
@@ -608,8 +608,8 @@ parse_record:
 			goto record_end;
 		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
 			goto record_end;
-		if (de->attr != ATTR_EXT && IS_FREE(de->name))
-			goto record_end;
+		if (!de->name[0])
+			goto end_of_dir;
 	} else {
 		if ((de->attr & ATTR_VOLUME) || IS_FREE(de->name))
 			goto record_end;
-- 
1.7.9.5

Re: [PATCH v2] fat: eliminate iterations in fat_search_long and __fat_readdir in case of EOD

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> From: Namjae Jeon <namjae.jeon@samsung.com>
>
> When doing lookups via fat_search_long(), we can stop checking for
> further entries if we detect End of Directory, i.e. if (de->name[0] ==
> 0x00).The current code traverses the cluster chain of a directory until a hit
> is found or till the last cluster for that directory, ignoring the EOD
> mark. Fix this.
>
> Likewise,when readdir(3) is called, we can stop checking for further
> entries in __fat_readdir() when we hit EOD.

Don't we need to change fat_get_short_entry()? And did this work
correctly about f_pos for readdir?

I'm not thinking about f_pos deeply though, it may have something
wrong. Because it stops at middle of cluster.

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

Re: [PATCH v2] fat: eliminate iterations in fat_search_long and __fat_readdir in case of EOD

[Namjae Jeon]

2013/2/7, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>
>> When doing lookups via fat_search_long(), we can stop checking for
>> further entries if we detect End of Directory, i.e. if (de->name[0] ==
>> 0x00).The current code traverses the cluster chain of a directory until a
>> hit
>> is found or till the last cluster for that directory, ignoring the EOD
>> mark. Fix this.
>>
>> Likewise,when readdir(3) is called, we can stop checking for further
>> entries in __fat_readdir() when we hit EOD.
>
Hi OGAWA.
> Don't we need to change fat_get_short_entry()?
Yes, We need to change here. Thanks for review!

> And did this work correctly about f_pos for readdir?
Yes, sure. f_pos is work correctly about each directory entry. because
after name[0] == 0x00, there are no allocated directory entires.
>
> I'm not thinking about f_pos deeply though, it may have something
> wrong. Because it stops at middle of cluster.
Plz See the below descirption about name[0] in FAT spec.
---------------------------------------------------------------------------------------------------------------------
If DIR_Name[0] == 0x00, then the directory entry is free (same as for
0xE5), and there are no allocated directory entries after this one
(all of the DIR_Name[0] bytes in all of the entries after this one are
also set to 0).
The special 0 value, rather than the 0xE5 value, indicates to FAT file
system driver code that the rest of the entries in this directory do
not need to be examined because they are all free.
-----------------------------------------------------------------------------------------------------------------------

I think that lookuping entry till the end of cluster is not needed.
Let me know your opinion.

And Would you tell me your opinion about fat exportfs ?
Thanks!
>
> Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v2] fat: eliminate iterations in fat_search_long and __fat_readdir in case of EOD

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> And did this work correctly about f_pos for readdir?
> Yes, sure. f_pos is work correctly about each directory entry. because
> after name[0] == 0x00, there are no allocated directory entires.
>>
>> I'm not thinking about f_pos deeply though, it may have something
>> wrong. Because it stops at middle of cluster.
> Plz See the below descirption about name[0] in FAT spec.
> ---------------------------------------------------------------------------------------------------------------------
> If DIR_Name[0] == 0x00, then the directory entry is free (same as for
> 0xE5), and there are no allocated directory entries after this one
> (all of the DIR_Name[0] bytes in all of the entries after this one are
> also set to 0).
> The special 0 value, rather than the 0xE5 value, indicates to FAT file
> system driver code that the rest of the entries in this directory do
> not need to be examined because they are all free.
> -----------------------------------------------------------------------------------------------------------------------
>
> I think that lookuping entry till the end of cluster is not needed.
> Let me know your opinion.

I know it though. There is seek() and broken drivers adds entries after
name[0] == 0. I think we don't need to care much about broken drivers
though. Even if so, kernel should not be crash, and corrupts fs more.

> And Would you tell me your opinion about fat exportfs ?

Ah, I was thinking I did. But it seems I didn't actually. Can you post
full of series? So, I can review and probably we can start to test it.

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

Re: [PATCH v2] fat: eliminate iterations in fat_search_long and __fat_readdir in case of EOD

[Namjae Jeon]

2013/2/8, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> And did this work correctly about f_pos for readdir?
>> Yes, sure. f_pos is work correctly about each directory entry. because
>> after name[0] == 0x00, there are no allocated directory entires.
>>>
>>> I'm not thinking about f_pos deeply though, it may have something
>>> wrong. Because it stops at middle of cluster.
>> Plz See the below descirption about name[0] in FAT spec.
>> ---------------------------------------------------------------------------------------------------------------------
>> If DIR_Name[0] == 0x00, then the directory entry is free (same as for
>> 0xE5), and there are no allocated directory entries after this one
>> (all of the DIR_Name[0] bytes in all of the entries after this one are
>> also set to 0).
>> The special 0 value, rather than the 0xE5 value, indicates to FAT file
>> system driver code that the rest of the entries in this directory do
>> not need to be examined because they are all free.
>> -----------------------------------------------------------------------------------------------------------------------
>>
>> I think that lookuping entry till the end of cluster is not needed.
>> Let me know your opinion.
>
> I know it though. There is seek() and broken drivers adds entries after
> name[0] == 0. I think we don't need to care much about broken drivers
> though. Even if so, kernel should not be crash, and corrupts fs more.
Yes, Right. I understood. Plz ignore this patch.
>
>> And Would you tell me your opinion about fat exportfs ?
>
> Ah, I was thinking I did. But it seems I didn't actually. Can you post
> full of series? So, I can review and probably we can start to test it.
Okay, I will post full patch-set today.

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