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

["Machida, Hiroyuki" <machida@sm.sony.co.jp>]

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.