2006-10-31 15:08:53

by Holden Karau

[permalink] [raw]
Subject: [PATCH 1/1] fat: improve sync performance by grouping writes revised

From: Holden Karau <[email protected]> http://www.holdenkarau.com
This is an attempt at improving fat_mirror_bhs in sync mode [namely it
writes all of the data for a backup block, and then blocks untill
finished]. The old behavior would write & block in smaller chunks, so
this should be slightly faster. It also removes the fix me requesting
that it be fixed to behave this way :-)
Signed-off-by: Holden Karau <[email protected]>
---
This is an improved version of an earlier patch, I've cleaned up the
formatting and it now also groups the write for the primary fat block
[so even if the user has only 2 FATs it should still be slightly
faster]. If no one objects, I'd like to see if it would be possible to
put this in the -mm tree to make sure it doesn't eat anyones fs :-)
In case the patch gets mangled I've put it up at http://www.holdenkarau.com/~holden/projects/fat/001_improve_fat_sync_performance_revised.patch

And now for the actual patch:

--- a/fs/fat/fatent.c 2006-09-19 23:42:06.000000000 -0400
+++ b/fs/fat/fatent.c 2006-10-31 09:09:34.000000000 -0500
@@ -1,5 +1,6 @@
/*
* Copyright (C) 2004, OGAWA Hirofumi
+ * Copyright (C) 2006, Holden Karau [Xandros]
* Released under GPL v2.
*/

@@ -343,52 +344,65 @@ int fat_ent_read(struct inode *inode, st
return ops->ent_get(fatent);
}

-/* FIXME: We can write the blocks as more big chunk. */
-static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
- int nr_bhs)
+
+static int fat_mirror_bhs_optw(struct super_block *sb, struct buffer_head **bhs,
+ int nr_bhs , int wait)
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
- struct buffer_head *c_bh;
+ struct buffer_head *c_bh[nr_bhs*(sbi->fats)];
int err, n, copy;

+ /* Always wait if mounted -o sync */
+ if (sb->s_flags & MS_SYNCHRONOUS )
+ wait = 1;
err = 0;
for (copy = 1; copy < sbi->fats; copy++) {
sector_t backup_fat = sbi->fat_length * copy;
-
- for (n = 0; n < nr_bhs; n++) {
- c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
- if (!c_bh) {
+ for (n = 0 ; n < nr_bhs ; n++ ) {
+ c_bh[(copy-1)*nr_bhs+n] = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
+ if (!c_bh[(copy-1)*nr_bhs+n]) {
+ printk(KERN_CRIT "fat: out of memory while copying backup fat. possible data loss\n");
err = -ENOMEM;
goto error;
}
- memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
- set_buffer_uptodate(c_bh);
- mark_buffer_dirty(c_bh);
- if (sb->s_flags & MS_SYNCHRONOUS)
- err = sync_dirty_buffer(c_bh);
- brelse(c_bh);
- if (err)
- goto error;
+ memcpy(c_bh[(copy-1)*nr_bhs+n]->b_data, bhs[n]->b_data, sb->s_blocksize);
+ set_buffer_uptodate(c_bh[(copy-1)*nr_bhs+n]);
+ mark_buffer_dirty(c_bh[(copy-1)*nr_bhs+n]);
}
}
+
+ if (wait) {
+ for (n = 0 ; n < nr_bhs ; n++) {
+ printk("copying to %d to %d\n" ,n, nr_bhs*(sbi->fats-1)+n);
+ c_bh[nr_bhs*(sbi->fats-1)+n] = bhs[n];
+ printk("done , %p==%p\n" , c_bh[nr_bhs*(sbi->fats-1)+n] , bhs[n]);
+ }
+ }
+ err = fat_sync_bhs_optw( c_bh , nr_bhs*(sbi->fats+wait-1) , wait );
+ if (err)
+ goto error;
+ for (n = 0; n < nr_bhs*(sbi->fats-1); n++ ) {
+ brelse(c_bh[n]);
+ }
error:
return err;
}

+static inline int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
+ int nr_bhs )
+{
+ return fat_mirror_bhs_optw(sb , bhs , nr_bhs, 0);
+}
+
+
int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
int new, int wait)
{
struct super_block *sb = inode->i_sb;
struct fatent_operations *ops = MSDOS_SB(sb)->fatent_ops;
- int err;

ops->ent_put(fatent, new);
- if (wait) {
- err = fat_sync_bhs(fatent->bhs, fatent->nr_bhs);
- if (err)
- return err;
- }
- return fat_mirror_bhs(sb, fatent->bhs, fatent->nr_bhs);
+ return fat_mirror_bhs_optw(sb, fatent->bhs, fatent->nr_bhs , wait);
}

static inline int fat_ent_next(struct msdos_sb_info *sbi,
@@ -505,9 +519,9 @@ out:
fatent_brelse(&fatent);
if (!err) {
if (inode_needs_sync(inode))
- err = fat_sync_bhs(bhs, nr_bhs);
- if (!err)
- err = fat_mirror_bhs(sb, bhs, nr_bhs);
+ err = fat_mirror_bhs_optw(sb , bhs, nr_bhs , 1);
+ else
+ err = fat_mirror_bhs_optw(sb, bhs, nr_bhs , 0 );
}
for (i = 0; i < nr_bhs; i++)
brelse(bhs[i]);
@@ -549,11 +563,6 @@ int fat_free_clusters(struct inode *inod
}

if (nr_bhs + fatent.nr_bhs > MAX_BUF_PER_PAGE) {
- if (sb->s_flags & MS_SYNCHRONOUS) {
- err = fat_sync_bhs(bhs, nr_bhs);
- if (err)
- goto error;
- }
err = fat_mirror_bhs(sb, bhs, nr_bhs);
if (err)
goto error;
@@ -564,11 +573,6 @@ int fat_free_clusters(struct inode *inod
fat_collect_bhs(bhs, &nr_bhs, &fatent);
} while (cluster != FAT_ENT_EOF);

- if (sb->s_flags & MS_SYNCHRONOUS) {
- err = fat_sync_bhs(bhs, nr_bhs);
- if (err)
- goto error;
- }
err = fat_mirror_bhs(sb, bhs, nr_bhs);
error:
fatent_brelse(&fatent);
--- a/fs/fat/misc.c 2006-09-19 23:42:06.000000000 -0400
+++ b/fs/fat/misc.c 2006-10-31 09:08:01.000000000 -0500
@@ -194,19 +194,28 @@ void fat_date_unix2dos(int unix_date, __

EXPORT_SYMBOL_GPL(fat_date_unix2dos);

-int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
+
+
+int fat_sync_bhs_optw(struct buffer_head **bhs, int nr_bhs ,int wait)
{
int i, err = 0;

ll_rw_block(SWRITE, nr_bhs, bhs);
- for (i = 0; i < nr_bhs; i++) {
- wait_on_buffer(bhs[i]);
- if (buffer_eopnotsupp(bhs[i])) {
- clear_buffer_eopnotsupp(bhs[i]);
- err = -EOPNOTSUPP;
- } else if (!err && !buffer_uptodate(bhs[i]))
- err = -EIO;
+ if (wait) {
+ for (i = 0; i < nr_bhs; i++) {
+ wait_on_buffer(bhs[i]);
+ if (buffer_eopnotsupp(bhs[i])) {
+ clear_buffer_eopnotsupp(bhs[i]);
+ err = -EOPNOTSUPP;
+ } else if (!err && !buffer_uptodate(bhs[i]))
+ err = -EIO;
+ }
}
+
return err;
}

+inline int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs ) {
+ return fat_sync_bhs_optw(bhs , nr_bhs , 1);
+}
+
--- a/include/linux/msdos_fs.h 2006-09-19 23:42:06.000000000 -0400
+++ b/include/linux/msdos_fs.h 2006-10-25 18:53:50.000000000 -0400
@@ -419,6 +419,7 @@ extern int fat_chain_add(struct inode *i
extern int date_dos2unix(unsigned short time, unsigned short date);
extern void fat_date_unix2dos(int unix_date, __le16 *time, __le16 *date);
extern int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs);
+extern int fat_sync_bhs_optw(struct buffer_head **bhs, int nr_bhs, int wait);

int fat_cache_init(void);
void fat_cache_destroy(void);



2006-10-31 16:28:29

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

On Tue, Oct 31, 2006 at 10:03:08AM -0500, Holden Karau wrote:
> @@ -343,52 +344,65 @@ int fat_ent_read(struct inode *inode, st
> return ops->ent_get(fatent);
> }
>
> -/* FIXME: We can write the blocks as more big chunk. */
> -static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
> - int nr_bhs)
> +
> +static int fat_mirror_bhs_optw(struct super_block *sb, struct buffer_head **bhs,
> + int nr_bhs , int wait)
> {
> struct msdos_sb_info *sbi = MSDOS_SB(sb);
> - struct buffer_head *c_bh;
> + struct buffer_head *c_bh[nr_bhs*(sbi->fats)];
> int err, n, copy;
>
> + /* Always wait if mounted -o sync */
> + if (sb->s_flags & MS_SYNCHRONOUS )
> + wait = 1;
> err = 0;
> for (copy = 1; copy < sbi->fats; copy++) {
> sector_t backup_fat = sbi->fat_length * copy;
> -
> - for (n = 0; n < nr_bhs; n++) {
> - c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> - if (!c_bh) {
> + for (n = 0 ; n < nr_bhs ; n++ ) {
> + c_bh[(copy-1)*nr_bhs+n] = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> + if (!c_bh[(copy-1)*nr_bhs+n]) {
> + printk(KERN_CRIT "fat: out of memory while copying backup fat. possible data loss\n");

I don't like that at all.

> err = -ENOMEM;
> goto error;
> }
> - memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
> - set_buffer_uptodate(c_bh);
> - mark_buffer_dirty(c_bh);
> - if (sb->s_flags & MS_SYNCHRONOUS)
> - err = sync_dirty_buffer(c_bh);
> - brelse(c_bh);
> - if (err)
> - goto error;
> + memcpy(c_bh[(copy-1)*nr_bhs+n]->b_data, bhs[n]->b_data, sb->s_blocksize);
> + set_buffer_uptodate(c_bh[(copy-1)*nr_bhs+n]);
> + mark_buffer_dirty(c_bh[(copy-1)*nr_bhs+n]);
> }
> }
> +
> + if (wait) {
> + for (n = 0 ; n < nr_bhs ; n++) {
> + printk("copying to %d to %d\n" ,n, nr_bhs*(sbi->fats-1)+n);

Is this the right version of the patch? The printk should never be left in.
Plus, as far as I can tell, that whole loop is actually just memcpy().

2006-10-31 16:30:28

by Jörn Engel

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

On Tue, 31 October 2006 10:03:08 -0500, Holden Karau wrote:
> +static int fat_mirror_bhs_optw(struct super_block *sb, struct buffer_head **bhs,
> + int nr_bhs , int wait)
> {
> struct msdos_sb_info *sbi = MSDOS_SB(sb);
> - struct buffer_head *c_bh;
> + struct buffer_head *c_bh[nr_bhs*(sbi->fats)];

Variable-sized array on the kernel-stack? That can easily explode in
your hands. Unless you are _very_ sure about the bounds, you should
do an explicit kmalloc. And if you were that sure, you could just as
well have an array with fixed size.

> + if (sb->s_flags & MS_SYNCHRONOUS )
[...]
> + }
[...]
> + int nr_bhs )

Trailing whitespace in those lines.

J?rn

--
Prosperity makes friends, adversity tries them.
-- Publilius Syrus

2006-10-31 16:46:36

by Holden Karau

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

On 10/31/06, Matthew Wilcox <[email protected]> wrote:
> On Tue, Oct 31, 2006 at 10:03:08AM -0500, Holden Karau wrote:
> > @@ -343,52 +344,65 @@ int fat_ent_read(struct inode *inode, st
> > return ops->ent_get(fatent);
> > }
> >
> > -/* FIXME: We can write the blocks as more big chunk. */
> > -static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
> > - int nr_bhs)
> > +
> > +static int fat_mirror_bhs_optw(struct super_block *sb, struct buffer_head **bhs,
> > + int nr_bhs , int wait)
> > {
> > struct msdos_sb_info *sbi = MSDOS_SB(sb);
> > - struct buffer_head *c_bh;
> > + struct buffer_head *c_bh[nr_bhs*(sbi->fats)];
> > int err, n, copy;
> >
> > + /* Always wait if mounted -o sync */
> > + if (sb->s_flags & MS_SYNCHRONOUS )
> > + wait = 1;
> > err = 0;
> > for (copy = 1; copy < sbi->fats; copy++) {
> > sector_t backup_fat = sbi->fat_length * copy;
> > -
> > - for (n = 0; n < nr_bhs; n++) {
> > - c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> > - if (!c_bh) {
> > + for (n = 0 ; n < nr_bhs ; n++ ) {
> > + c_bh[(copy-1)*nr_bhs+n] = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> > + if (!c_bh[(copy-1)*nr_bhs+n]) {
> > + printk(KERN_CRIT "fat: out of memory while copying backup fat. possible data loss\n");
>
> I don't like that at all.
Not much to be done about that. The amount of memory required is
fairly small, but if its not there its not there.
>
> > err = -ENOMEM;
> > goto error;
> > }
> > - memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
> > - set_buffer_uptodate(c_bh);
> > - mark_buffer_dirty(c_bh);
> > - if (sb->s_flags & MS_SYNCHRONOUS)
> > - err = sync_dirty_buffer(c_bh);
> > - brelse(c_bh);
> > - if (err)
> > - goto error;
> > + memcpy(c_bh[(copy-1)*nr_bhs+n]->b_data, bhs[n]->b_data, sb->s_blocksize);
> > + set_buffer_uptodate(c_bh[(copy-1)*nr_bhs+n]);
> > + mark_buffer_dirty(c_bh[(copy-1)*nr_bhs+n]);
> > }
> > }
> > +
> > + if (wait) {
> > + for (n = 0 ; n < nr_bhs ; n++) {
> > + printk("copying to %d to %d\n" ,n, nr_bhs*(sbi->fats-1)+n);
>
> Is this the right version of the patch? The printk should never be left in.
> Plus, as far as I can tell, that whole loop is actually just memcpy().
whoops. That was in for debugging, I thought I took that out. The loop
structure is how it was before, but I don't see a way to get rid of
it, do you have an idea?
>


--
Cell: 613-276-1645

2006-10-31 16:54:25

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

Holden Karau <[email protected]> writes:

> From: Holden Karau <[email protected]> http://www.holdenkarau.com
> This is an attempt at improving fat_mirror_bhs in sync mode [namely it
> writes all of the data for a backup block, and then blocks untill
> finished]. The old behavior would write & block in smaller chunks, so
> this should be slightly faster. It also removes the fix me requesting
> that it be fixed to behave this way :-)

Please post the result of performance test. If it's fairly big, we
would be able to use async for mirror FAT. Instead, for hotplug device
we can provide the another option.
--
OGAWA Hirofumi <[email protected]>

2006-10-31 18:10:45

by Holden Karau

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

On 10/31/06, J?rn Engel <[email protected]> wrote:
> On Tue, 31 October 2006 10:03:08 -0500, Holden Karau wrote:
> > +static int fat_mirror_bhs_optw(struct super_block *sb, struct buffer_head **bhs,
> > + int nr_bhs , int wait)
> > {
> > struct msdos_sb_info *sbi = MSDOS_SB(sb);
> > - struct buffer_head *c_bh;
> > + struct buffer_head *c_bh[nr_bhs*(sbi->fats)];
>
> Variable-sized array on the kernel-stack? That can easily explode in
> your hands. Unless you are _very_ sure about the bounds, you should
> do an explicit kmalloc. And if you were that sure, you could just as
> well have an array with fixed size.
>
sbi->fats has a range of 2 to 4, but I suppose that might concievably
change if someone decides they want a fat filesystem with a lot of
backup FATs and modifies some other parts of the driver to support
that. I'll change it to use kmalloc.
> > + if (sb->s_flags & MS_SYNCHRONOUS )
> [...]
> > + }
> [...]
> > + int nr_bhs )
>
> Trailing whitespace in those lines.
..... oops. I'll fix that.
>
> J?rn
>
> --
> Prosperity makes friends, adversity tries them.
> -- Publilius Syrus
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>


--
Cell: 613-276-1645

2006-10-31 18:46:50

by Holden Karau

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

The performance increase is pretty small. Using an old external dirve
I had lying around I got:
diff -y stock/10k modified/10k
10240+0 records in | 1024+0
records in
10240+0 records out | 1024+0
records out
5242880 bytes transferred in 18.280922 seconds (286795 bytes/ | 524288
bytes transferred in 1.824985 seconds (287283 bytes/se
diff -y stock/1k modified/1k
1024+0 records in 1024+0
records in
1024+0 records out 1024+0
records out
524288 bytes transferred in 1.777250 seconds (295000 bytes/se | 524288
bytes transferred in 1.764748 seconds (297089 bytes/se

The usual disclaimer of any benchmarking applies, YMMV.

Cheers,

Holden :-)

On 10/31/06, OGAWA Hirofumi <[email protected]> wrote:
> Holden Karau <[email protected]> writes:
>
> > From: Holden Karau <[email protected]> http://www.holdenkarau.com
> > This is an attempt at improving fat_mirror_bhs in sync mode [namely it
> > writes all of the data for a backup block, and then blocks untill
> > finished]. The old behavior would write & block in smaller chunks, so
> > this should be slightly faster. It also removes the fix me requesting
> > that it be fixed to behave this way :-)
>
> Please post the result of performance test. If it's fairly big, we
> would be able to use async for mirror FAT. Instead, for hotplug device
> we can provide the another option.
> --
> OGAWA Hirofumi <[email protected]>
>


--
Cell: 613-276-1645

2006-10-31 20:24:10

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

"Holden Karau" <[email protected]> writes:

> The performance increase is pretty small. Using an old external dirve
> I had lying around I got:
> diff -y stock/10k modified/10k
> 10240+0 records in | 1024+0
> records in
> 10240+0 records out | 1024+0
> records out
> 5242880 bytes transferred in 18.280922 seconds (286795 bytes/ | 524288
> bytes transferred in 1.824985 seconds (287283 bytes/se

1024 records out 1.824985 seconds. Is there decrease case? I assume
the result is same. So, we would need different approach.

> diff -y stock/1k modified/1k
> 1024+0 records in 1024+0
> records in
> 1024+0 records out 1024+0
> records out
> 524288 bytes transferred in 1.777250 seconds (295000 bytes/se | 524288
> bytes transferred in 1.764748 seconds (297089 bytes/se
--
OGAWA Hirofumi <[email protected]>

2006-10-31 20:54:51

by Holden Karau

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

Keep in mind these results are with a slow external drive, but yah
fat32 sync performance is still really bad.

This patch is just meant to make fat32 sync performance better, not
necessarily make it usable for everyone [one step at a time and all
that]. FAT32 sync is quite slow (see
http://readlist.com/lists/vger.kernel.org/linux-kernel/22/111761.html
), so if you want fast performance [or have drives with limited write
cycles] async is almost definitely the way to go.

I have a patch [not this one] which adds a module param ("fast") which
syncs the FAT tables after 30 seconds of no write activity, but I
don't think the majority of people are interested in it.

On 10/31/06, OGAWA Hirofumi <[email protected]> wrote:
> "Holden Karau" <[email protected]> writes:
>
> > The performance increase is pretty small. Using an old external dirve
> > I had lying around I got:
> > diff -y stock/10k modified/10k
> > 10240+0 records in | 1024+0
> > records in
> > 10240+0 records out | 1024+0
> > records out
> > 5242880 bytes transferred in 18.280922 seconds (286795 bytes/ | 524288
> > bytes transferred in 1.824985 seconds (287283 bytes/se
>
> 1024 records out 1.824985 seconds. Is there decrease case? I assume
> the result is same. So, we would need different approach.
>
> > diff -y stock/1k modified/1k
> > 1024+0 records in 1024+0
> > records in
> > 1024+0 records out 1024+0
> > records out
> > 524288 bytes transferred in 1.777250 seconds (295000 bytes/se | 524288
> > bytes transferred in 1.764748 seconds (297089 bytes/se
> --
> OGAWA Hirofumi <[email protected]>
>


--
Cell: 613-276-1645

2006-11-01 02:33:14

by Holden Karau

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

Whoops, sorry about that. Yes it would improve performance on fat12 &
fat16 [although I haven't tested it with fat12]. What I meant by that
sentance is that, fat sync performance is pretty bad with or without
my patch, its just slightly less worse with my patch :-)

On 10/31/06, OGAWA Hirofumi <[email protected]> wrote:
> "Holden Karau" <[email protected]> writes:
>
> > This patch is just meant to make fat32 sync performance better, not
> > necessarily make it usable for everyone [one step at a time and all
> > that].
>
> Sorry, I can't see your point. The FAT12 and FAT16 also have backup FAT.
> And the your patch didn't make performance better, right?
> --
> OGAWA Hirofumi <[email protected]>
>


--
Cell: 613-276-1645

2006-11-01 03:10:47

by Holden Karau

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

I was thinking about the issue of running out of memory, while its not
particularly likely to happen except on devices with huge disks and
tiney amount of memory, it is a possibility. I can make it
fall-through to the previous way of doing things, does that sound like
a reasonable idea?

On 10/31/06, Holden Karau <[email protected]> wrote:
> On 10/31/06, Matthew Wilcox <[email protected]> wrote:
> > On Tue, Oct 31, 2006 at 10:03:08AM -0500, Holden Karau wrote:
> > > @@ -343,52 +344,65 @@ int fat_ent_read(struct inode *inode, st
> > > return ops->ent_get(fatent);
> > > }
> > >
> > > -/* FIXME: We can write the blocks as more big chunk. */
> > > -static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
> > > - int nr_bhs)
> > > +
> > > +static int fat_mirror_bhs_optw(struct super_block *sb, struct buffer_head **bhs,
> > > + int nr_bhs , int wait)
> > > {
> > > struct msdos_sb_info *sbi = MSDOS_SB(sb);
> > > - struct buffer_head *c_bh;
> > > + struct buffer_head *c_bh[nr_bhs*(sbi->fats)];
> > > int err, n, copy;
> > >
> > > + /* Always wait if mounted -o sync */
> > > + if (sb->s_flags & MS_SYNCHRONOUS )
> > > + wait = 1;
> > > err = 0;
> > > for (copy = 1; copy < sbi->fats; copy++) {
> > > sector_t backup_fat = sbi->fat_length * copy;
> > > -
> > > - for (n = 0; n < nr_bhs; n++) {
> > > - c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> > > - if (!c_bh) {
> > > + for (n = 0 ; n < nr_bhs ; n++ ) {
> > > + c_bh[(copy-1)*nr_bhs+n] = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> > > + if (!c_bh[(copy-1)*nr_bhs+n]) {
> > > + printk(KERN_CRIT "fat: out of memory while copying backup fat. possible data loss\n");
> >
> > I don't like that at all.
> Not much to be done about that. The amount of memory required is
> fairly small, but if its not there its not there.
> >
> > > err = -ENOMEM;
> > > goto error;
> > > }
> > > - memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
> > > - set_buffer_uptodate(c_bh);
> > > - mark_buffer_dirty(c_bh);
> > > - if (sb->s_flags & MS_SYNCHRONOUS)
> > > - err = sync_dirty_buffer(c_bh);
> > > - brelse(c_bh);
> > > - if (err)
> > > - goto error;
> > > + memcpy(c_bh[(copy-1)*nr_bhs+n]->b_data, bhs[n]->b_data, sb->s_blocksize);
> > > + set_buffer_uptodate(c_bh[(copy-1)*nr_bhs+n]);
> > > + mark_buffer_dirty(c_bh[(copy-1)*nr_bhs+n]);
> > > }
> > > }
> > > +
> > > + if (wait) {
> > > + for (n = 0 ; n < nr_bhs ; n++) {
> > > + printk("copying to %d to %d\n" ,n, nr_bhs*(sbi->fats-1)+n);
> >
> > Is this the right version of the patch? The printk should never be left in.
> > Plus, as far as I can tell, that whole loop is actually just memcpy().
> whoops. That was in for debugging, I thought I took that out. The loop
> structure is how it was before, but I don't see a way to get rid of
> it, do you have an idea?
> >
>
>
> --
> Cell: 613-276-1645
>


--
Cell: 613-276-1645

2006-11-01 00:58:58

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

"Holden Karau" <[email protected]> writes:

> This patch is just meant to make fat32 sync performance better, not
> necessarily make it usable for everyone [one step at a time and all
> that].

Sorry, I can't see your point. The FAT12 and FAT16 also have backup FAT.
And the your patch didn't make performance better, right?
--
OGAWA Hirofumi <[email protected]>

2006-11-01 13:50:18

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised

On Tue, Oct 31, 2006 at 10:10:39PM -0500, Holden Karau wrote:
> I was thinking about the issue of running out of memory, while its not
> particularly likely to happen except on devices with huge disks and
> tiney amount of memory, it is a possibility. I can make it
> fall-through to the previous way of doing things, does that sound like
> a reasonable idea?

Yes, or you could make it sync the ones for which you did have enough
memory, and then restart.