Re: [PATCH][FAT] FAT dirent scan with hin take #2
OGAWA Hirofumi <[EMAIL PROTECTED]> writes: > This patch couldn't compile. I assume you post a wrong patch...? ^ version -- OGAWA Hirofumi <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
"Machida, Hiroyuki" <[EMAIL PROTECTED]> writes: > Here is a revised version of dirent scan patch, mentioned at > following E-mail. > > This patch addresses performance damages on "ls | xargs xxx" and > reverse order scan which are reported to the previous patch. > > With this patch, fat_search_long() and fat_scan() use hint value > as start of scan. For each directory holds multiple hint value entries. > The entry would be selected by hash value based on scan target name and > PID. Hint value would be calculated based on the entry previously found > entry, so that the hint can cover backward neighborhood. This patch couldn't compile. I assume you post a wrong patch...? The code is strange... Is the hint value related to the previously accessed entry? This seems to be randomly cacheing the recent access position... Is it your intention of this patch? -- OGAWA Hirofumi <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
On Tue, Aug 30, 2005 at 11:55:46AM +0300, Pekka Enberg wrote:
> Hi,
>
> Some more.
>
> On 8/30/05, Machida, Hiroyuki <[EMAIL PROTECTED]> wrote:
> > --- old/fs/fat/inode.c 2005-08-29 09:38:53.308587787 +0900
> > +++ new/fs/fat/inode.c 2005-08-29 09:39:33.889555606 +0900
> > @@ -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);
> > + void *hints;
> > +
> > + down(&(MSDOS_I(inode)->scan_lock);
> > + hints = (void *)(MSDOS_I(inode)->scan_hints);
> > + if (hints) {
> > + MSDOS_I(inode)->scan_hints = 0;
> > + }
> > + up(&(MSDOS_I(inode)->scan_lock);
> > + if (hints) kfree(hints);
>
> Please just make the local variable hints of type loff_t * to get rid
> of the pointless casting.
For voids no casting is needed anyway, the type exists for the very
reason of being compatible with any other.
> > if (is_bad_inode(inode))
> > return;
> > @@ -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;
>
> Use NULLs instead of 0 for pointers to keep new code sparse-clean.
It's a good idea for readability, too.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
Hi,
Some more.
On 8/30/05, Machida, Hiroyuki <[EMAIL PROTECTED]> wrote:
> --- old/fs/fat/inode.c 2005-08-29 09:38:53.308587787 +0900
> +++ new/fs/fat/inode.c 2005-08-29 09:39:33.889555606 +0900
> @@ -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);
> + void *hints;
> +
> + down(&(MSDOS_I(inode)->scan_lock);
> + hints = (void *)(MSDOS_I(inode)->scan_hints);
> + if (hints) {
> + MSDOS_I(inode)->scan_hints = 0;
> + }
> + up(&(MSDOS_I(inode)->scan_lock);
> + if (hints) kfree(hints);
Please just make the local variable hints of type loff_t * to get rid
of the pointless casting.
>
> if (is_bad_inode(inode))
> return;
> @@ -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;
Use NULLs instead of 0 for pointers to keep new code sparse-clean.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
Hi,
Some coding style nitpicks.
On 8/30/05, Machida, Hiroyuki <[EMAIL PROTECTED]> wrote:
> +inline
> +static int hint_allocate(struct inode *dir)
> +{
> + void *hints;
> + int err = 0;
> +
> + if (!MSDOS_I(dir)->scan_hints) {
> + hints = kmalloc(FAT_SCAN_NWAY*sizeof(loff_t),GFP_KERNEL);
> + if (hints)
> + memset(hints, 0, FAT_SCAN_NWAY*sizeof(loff_t));
> + else
> + err = -ENOMEM;
Please consider using kcalloc().
> +
> + down(_I(dir)->scan_lock);
> + if (MSDOS_I(dir)->scan_hints) err = -EINVAL;
Please put the statement after if clause to a separate line. The above
makes code very hard to read.
> + if (!err) MSDOS_I(dir)->scan_hints = hints;
> + up(_I(dir)->scan_lock);
> + if (err == -EINVAL) {
> + if (hints) kfree(hints);
kfree() can handle NULLs just fine so please drop the redundant check.
Pekka
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
Hi,
Some coding style nitpicks.
On 8/30/05, Machida, Hiroyuki [EMAIL PROTECTED] wrote:
+inline
+static int hint_allocate(struct inode *dir)
+{
+ void *hints;
+ int err = 0;
+
+ if (!MSDOS_I(dir)-scan_hints) {
+ hints = kmalloc(FAT_SCAN_NWAY*sizeof(loff_t),GFP_KERNEL);
+ if (hints)
+ memset(hints, 0, FAT_SCAN_NWAY*sizeof(loff_t));
+ else
+ err = -ENOMEM;
Please consider using kcalloc().
+
+ down(MSDOS_I(dir)-scan_lock);
+ if (MSDOS_I(dir)-scan_hints) err = -EINVAL;
Please put the statement after if clause to a separate line. The above
makes code very hard to read.
+ if (!err) MSDOS_I(dir)-scan_hints = hints;
+ up(MSDOS_I(dir)-scan_lock);
+ if (err == -EINVAL) {
+ if (hints) kfree(hints);
kfree() can handle NULLs just fine so please drop the redundant check.
Pekka
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
Hi,
Some more.
On 8/30/05, Machida, Hiroyuki [EMAIL PROTECTED] wrote:
--- old/fs/fat/inode.c 2005-08-29 09:38:53.308587787 +0900
+++ new/fs/fat/inode.c 2005-08-29 09:39:33.889555606 +0900
@@ -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);
+ void *hints;
+
+ down((MSDOS_I(inode)-scan_lock);
+ hints = (void *)(MSDOS_I(inode)-scan_hints);
+ if (hints) {
+ MSDOS_I(inode)-scan_hints = 0;
+ }
+ up((MSDOS_I(inode)-scan_lock);
+ if (hints) kfree(hints);
Please just make the local variable hints of type loff_t * to get rid
of the pointless casting.
if (is_bad_inode(inode))
return;
@@ -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;
Use NULLs instead of 0 for pointers to keep new code sparse-clean.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
On Tue, Aug 30, 2005 at 11:55:46AM +0300, Pekka Enberg wrote:
Hi,
Some more.
On 8/30/05, Machida, Hiroyuki [EMAIL PROTECTED] wrote:
--- old/fs/fat/inode.c 2005-08-29 09:38:53.308587787 +0900
+++ new/fs/fat/inode.c 2005-08-29 09:39:33.889555606 +0900
@@ -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);
+ void *hints;
+
+ down((MSDOS_I(inode)-scan_lock);
+ hints = (void *)(MSDOS_I(inode)-scan_hints);
+ if (hints) {
+ MSDOS_I(inode)-scan_hints = 0;
+ }
+ up((MSDOS_I(inode)-scan_lock);
+ if (hints) kfree(hints);
Please just make the local variable hints of type loff_t * to get rid
of the pointless casting.
For voids no casting is needed anyway, the type exists for the very
reason of being compatible with any other.
if (is_bad_inode(inode))
return;
@@ -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;
Use NULLs instead of 0 for pointers to keep new code sparse-clean.
It's a good idea for readability, too.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
Machida, Hiroyuki [EMAIL PROTECTED] writes: Here is a revised version of dirent scan patch, mentioned at following E-mail. This patch addresses performance damages on ls | xargs xxx and reverse order scan which are reported to the previous patch. With this patch, fat_search_long() and fat_scan() use hint value as start of scan. For each directory holds multiple hint value entries. The entry would be selected by hash value based on scan target name and PID. Hint value would be calculated based on the entry previously found entry, so that the hint can cover backward neighborhood. This patch couldn't compile. I assume you post a wrong patch...? The code is strange... Is the hint value related to the previously accessed entry? This seems to be randomly cacheing the recent access position... Is it your intention of this patch? -- OGAWA Hirofumi [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][FAT] FAT dirent scan with hin take #2
OGAWA Hirofumi [EMAIL PROTECTED] writes: This patch couldn't compile. I assume you post a wrong patch...? ^ version -- OGAWA Hirofumi [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
