Re: [PATCH][FAT] FAT dirent scan with hin take #3

Machida, Hiroyuki
2005-08-31T13:04:24Z

Hi,

Thank you checking code...

Pekka Enberg wrote:
> Hi,
>
:
snip

>>
>>This patch enables using hint information on scanning dir.
>>It achieves excellent performance with "ls -l" for over 1000 entries.
>>
>>* fat-dirscan-with-hint_3.patch for linux 2.6.13
>>
>> fs/fat/dir.c | 130 ++++++++++++++++++++++++++++++++++++++++++++---
>> fs/fat/inode.c | 13 ++++
>> include/linux/msdos_fs.h | 2
>> 3 files changed, 137 insertions(+), 8 deletions(-)
>>
>>Signed-off-by: Hiroyuki Machida <machida@sm.sony.co.jp> for CELF
>>
>>* modified files
>>
>>
>>--- linux-2.6.13/fs/fat/dir.c 2005-08-29 08:41:01.000000000 +0900
>>+++ linux-2.6.13-work/fs/fat/dir.c 2005-08-31 13:48:01.001119437 +0900
>>@@ -201,6 +201,88 @@ fat_shortname2uni(struct nls_table *nls,
>> * Return values: negative -> error, 0 -> not found, positive -> found,
>> * value is the total amount of slots, including the shortname entry.
>> */
>>+
>>+#define FAT_SCAN_SHIFT 4 /* 2x8 way scan hints */
>>+#define FAT_SCAN_NWAY (1<<FAT_SCAN_SHIFT)
>>+
>>+inline
>>+static int hint_allocate(struct inode *dir)
>>+{
>>+ loff_t *hints;
>>+ int err = 0;
>>+
>>+ if (!MSDOS_I(dir)->scan_hints) {
>
>
> Please consider moving this check to callers. Conditional allocation
> makes this bit strange API-wise. Or alternatively, give
> hint_allocate() a better name.

How about hint_allocate_conditional() ?

>
>>--- linux-2.6.13/fs/fat/inode.c 2005-08-29 08:41:01.000000000 +0900
>>+++ linux-2.6.13-work/fs/fat/inode.c 2005-08-31 12:59:54.292274060 +0900
>>@@ -242,6 +242,8 @@ static int fat_fill_inode(struct inode *
>> inode->i_version++;
>> inode->i_generation = get_seconds();
>>
>>+ init_MUTEX(&MSDOS_I(inode)->scan_lock);
>>+ MSDOS_I(inode)->scan_hints = 0;
>
>
> Please use NULL.

Ok,

>
>> if ((de->attr & ATTR_DIR) && !IS_FREE(de->name)) {
>> inode->i_generation &= ~1;
>> inode->i_mode = MSDOS_MKMODE(de->attr,
>>@@ -345,6 +347,15 @@ static void fat_delete_inode(struct inod
>> static void fat_clear_inode(struct inode *inode)
>> {
>> struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
>>+ loff_t *hints;
>>+
>>+ down(&MSDOS_I(inode)->scan_lock);
>>+ hints = (MSDOS_I(inode)->scan_hints);
>
>
> Pleas drop redundant parenthesis.

Ok,

>
>>+ if (hints) {
>>+ MSDOS_I(inode)->scan_hints = NULL;
>>+ }
>>+ up(&MSDOS_I(inode)->scan_lock);
>>+ kfree(hints);
>
>
> Why can't you do kfree() under scan_lock?
>
Just try to minimize blocked period.

>
>>@@ -1011,6 +1022,8 @@ static int fat_read_root(struct inode *i
>> struct msdos_sb_info *sbi = MSDOS_SB(sb);
>> int error;
>>
>>+ init_MUTEX(&MSDOS_I(inode)->scan_lock);
>>+ MSDOS_I(inode)->scan_hints = 0;
>
>
> Please use NULL here.

Ok.

> Pekka
> -
> 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/

--
Hiroyuki Machida machida@sm.sony.co.jp
SSW Dept. HENC, Sony Corp.