[PATCH] let fat handle MS_SYNCHRONOUS flag

[PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

Hi,

this patch is an RFC patch not to be applied.
It adds MS_SYNCHRONOUS support to FAT filesystem, so that less
filesystem breakage happen when disconnecting an USB key, for 
example. I'd like to have comments about it, because as it 
seems to work fine here, I'm not used to fs drivers and could
have made mistakes.
Thanks,

Signed-off-by: Colin Leroy <colin@colino.net>
--- a/fs/fat/dir.c	2004-11-18 19:42:41.704777744 +0100
+++ b/fs/fat/dir.c	2004-11-18 14:36:44.000000000 +0100
@@ -736,6 +736,7 @@
 {
 	struct buffer_head *bh;
 	struct msdos_dir_entry *de;
+	struct super_block *sb;
 	__le16 date, time;
 
 	bh = fat_extend_dir(dir);
@@ -764,6 +765,11 @@
 	dir->i_atime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 	mark_inode_dirty(dir);
 
+	sb = dir->i_sb;
+
+	if (sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
+
 	return 0;
 }
 
--- a/fs/fat/file.c	2004-10-18 23:53:44.000000000 +0200
+++ b/fs/fat/file.c	2004-11-18 14:57:03.000000000 +0100
@@ -74,21 +74,34 @@
 {
 	struct inode *inode = filp->f_dentry->d_inode;
 	int retval;
+	struct super_block *sb = inode->i_sb;
+	struct buffer_head *bh = NULL;
 
 	retval = generic_file_write(filp, buf, count, ppos);
 	if (retval > 0) {
 		inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
 		mark_inode_dirty(inode);
+		if (sb->s_flags & MS_SYNCHRONOUS) {
+			bh = sb_bread(sb, MSDOS_SB(sb)->fsinfo_sector);
+			if (bh != NULL) {
+				sync_dirty_buffer(bh);
+				brelse(bh);
+			} else {
+				BUG_ON(1);
+			}
+		}
 	}
 	return retval;
 }
 
 void fat_truncate(struct inode *inode)
 {
+	struct super_block *sb = inode->i_sb;
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 	const unsigned int cluster_size = sbi->cluster_size;
 	int nr_clusters;
+	struct buffer_head *bh = NULL;
 
 	/* 
 	 * This protects against truncating a file bigger than it was then
@@ -105,4 +118,8 @@
 	unlock_kernel();
 	inode->i_ctime = inode->i_mtime = CURRENT_TIME;
 	mark_inode_dirty(inode);
+	if (sb->s_flags & MS_SYNCHRONOUS) {
+		bh = sb_bread(sb, sbi->fsinfo_sector);
+		sync_dirty_buffer(bh);
+	}
 }
--- a/fs/fat/inode.c	2004-11-18 19:42:41.710776832 +0100
+++ b/fs/fat/inode.c	2004-11-18 15:00:55.000000000 +0100
@@ -1273,8 +1273,12 @@
 	}
 	spin_unlock(&sbi->inode_hash_lock);
 	mark_buffer_dirty(bh);
-	brelse(bh);
 	unlock_kernel();
+
+	if (sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
+	brelse(bh);
+
 	return 0;
 }
 

[PATCH] let vfat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 18 Nov 2004 at 19h11, Colin Leroy wrote:

Hi, 
Same patch for vfat.

Signed-off-by: Colin Leroy <colin@colino.net>
--- a/fs/vfat/namei.c	2004-10-18 23:54:37.000000000 +0200
+++ b/fs/vfat/namei.c	2004-11-18 18:41:52.000000000 +0100
@@ -743,6 +743,8 @@
 	(*de)->adate = (*de)->cdate = (*de)->date;
 
 	mark_buffer_dirty(*bh);
+	if (dir->i_sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(*bh);
 
 	/* slots can't be less than 1 */
 	sinfo_out->long_slots = slots - 1;
@@ -844,7 +846,6 @@
 	if (res < 0)
 		goto out;
 	inode = fat_build_inode(sb, de, sinfo.i_pos, &res);
-	brelse(bh);
 	if (!inode)
 		goto out;
 	res = 0;
@@ -854,7 +855,10 @@
 	dir->i_version++;
 	dentry->d_time = dentry->d_parent->d_inode->i_version;
 	d_instantiate(dentry,inode);
+	if (sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
 out:
+	brelse(bh);
 	unlock_kernel();
 	return res;
 }
@@ -871,6 +875,7 @@
 	mark_inode_dirty(dir);
 	de->name[0] = DELETED_FLAG;
 	mark_buffer_dirty(bh);
+
 	/* remove the longname */
 	offset = sinfo->longname_offset; de = NULL;
 	for (i = sinfo->long_slots; i > 0; --i) {
@@ -880,6 +885,9 @@
 		de->attr = ATTR_NONE;
 		mark_buffer_dirty(bh);
 	}
+	if (dir->i_sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
+
 	brelse(bh);
 }
 
@@ -903,7 +911,7 @@
 	dentry->d_inode->i_mtime = dentry->d_inode->i_atime = CURRENT_TIME;
 	fat_detach(dentry->d_inode);
 	mark_inode_dirty(dentry->d_inode);
-	/* releases bh */
+	/* releases bh and syncs it if necessary */
 	vfat_remove_entry(dir,&sinfo,bh,de);
 	dir->i_nlink--;
 out:
@@ -926,7 +934,7 @@
 	dentry->d_inode->i_mtime = dentry->d_inode->i_atime = CURRENT_TIME;
 	fat_detach(dentry->d_inode);
 	mark_inode_dirty(dentry->d_inode);
-	/* releases bh */
+	/* releases bh and syncs it if necessary */
 	vfat_remove_entry(dir,&sinfo,bh,de);
 out:
 	unlock_kernel();
@@ -956,6 +964,10 @@
 	dir->i_version++;
 	dir->i_nlink++;
 	inode->i_nlink = 2; /* no need to mark them dirty */
+
+	if (sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
+	
 	res = fat_new_dir(inode, dir, 1);
 	if (res < 0)
 		goto mkdir_failed;
@@ -972,7 +984,7 @@
 	inode->i_mtime = inode->i_atime = CURRENT_TIME;
 	fat_detach(inode);
 	mark_inode_dirty(inode);
-	/* releases bh */
+	/* releases bh ands syncs if necessary */
 	vfat_remove_entry(dir,&sinfo,bh,de);
 	iput(inode);
 	dir->i_nlink--;
@@ -1057,6 +1069,8 @@
 			new_dir->i_nlink++;
 			mark_inode_dirty(new_dir);
 		}
+		if (new_dir->i_sb->s_flags & MS_SYNCHRONOUS)
+			sync_dirty_buffer(dotdot_bh);
 	}
 
 rename_done:

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 18 Nov 2004 at 19h11, Colin Leroy wrote:

Hi, 

> this patch is an RFC patch not to be applied.

no one has any comments ? Is it so broken ? ;)

-- 
Colin
  6a65 206e 2761 7272 6976 6520 7061 7320
  e020 6372 6f69 7265 2071 7527 696c 2079
  2065 6e20 6169 7420 756e 2071 7569 2061
  2072 6567 6172 64e9 20e7 6120 5c21 0a00

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[OGAWA Hirofumi]

Colin Leroy <colin@colino.net> writes:

> It adds MS_SYNCHRONOUS support to FAT filesystem, so that less
> filesystem breakage happen when disconnecting an USB key, for 
> example. I'd like to have comments about it, because as it 
> seems to work fine here, I'm not used to fs drivers and could
> have made mistakes.

What cases should these patches guarantee that users can unplug the
USB key?  And can we guarantee the same cases by improving autofs or
the similar stuff?

I'm not sure sync-mode is proper for it...
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Matt Mackall]

On Thu, Nov 18, 2004 at 07:49:59PM +0100, Colin Leroy wrote:
> Hi,
> 
> this patch is an RFC patch not to be applied.
> It adds MS_SYNCHRONOUS support to FAT filesystem, so that less
> filesystem breakage happen when disconnecting an USB key, for 
> example. I'd like to have comments about it, because as it 
> seems to work fine here, I'm not used to fs drivers and could
> have made mistakes.
> Thanks,
> 
> +			if (bh != NULL) {
> +				sync_dirty_buffer(bh);
> +				brelse(bh);
> +			} else {
> +				BUG_ON(1);

This construct is really weird.

How about:

BUG_ON(!bh);
sync_dirty_buffer(bh);
brelse(bh);

Concept seems good, and the implementation otherwise looks good at
first glance.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Matt Mackall]

On Wed, Nov 24, 2004 at 05:24:36AM +0900, OGAWA Hirofumi wrote:
> Colin Leroy <colin@colino.net> writes:
> 
> > It adds MS_SYNCHRONOUS support to FAT filesystem, so that less
> > filesystem breakage happen when disconnecting an USB key, for 
> > example. I'd like to have comments about it, because as it 
> > seems to work fine here, I'm not used to fs drivers and could
> > have made mistakes.
> 
> What cases should these patches guarantee that users can unplug the
> USB key?  And can we guarantee the same cases by improving autofs or
> the similar stuff?

Well there can be no guarantees - there will always be a race between
flush and hot unplug. If we're careful with write ordering, we can
perhaps arrange to avoid the worst sorts of corruption, provided the
device does the right thing when it's in the middle of an IO.

But generally I think this is a good idea as it shrinks the window.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[OGAWA Hirofumi]

Matt Mackall <mpm@selenic.com> writes:

> On Wed, Nov 24, 2004 at 05:24:36AM +0900, OGAWA Hirofumi wrote:
>> Colin Leroy <colin@colino.net> writes:
>> 
>> > It adds MS_SYNCHRONOUS support to FAT filesystem, so that less
>> > filesystem breakage happen when disconnecting an USB key, for 
>> > example. I'd like to have comments about it, because as it 
>> > seems to work fine here, I'm not used to fs drivers and could
>> > have made mistakes.
>> 
>> What cases should these patches guarantee that users can unplug the
>> USB key?  And can we guarantee the same cases by improving autofs or
>> the similar stuff?
>
> Well there can be no guarantees - there will always be a race between
> flush and hot unplug. If we're careful with write ordering, we can
> perhaps arrange to avoid the worst sorts of corruption, provided the
> device does the right thing when it's in the middle of an IO.
>
> But generally I think this is a good idea as it shrinks the window.

Things which I want to say here - do we really need the bogus
sync-mode?

Current fatfs is not keeping the consistency of data on the disk at
all.  So, after all, the data on a disk is corrupting until all
syscalls finish, right?

If so, isn't this too slow? I doubt this is good solution for this
problem (USB key unplugging)...

Well, it seems good as start of sync-mode though.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Matt Mackall]

On Wed, Nov 24, 2004 at 02:00:54PM +0900, OGAWA Hirofumi wrote:
> Matt Mackall <mpm@selenic.com> writes:
> 
> > On Wed, Nov 24, 2004 at 05:24:36AM +0900, OGAWA Hirofumi wrote:
> >> Colin Leroy <colin@colino.net> writes:
> >> 
> >> > It adds MS_SYNCHRONOUS support to FAT filesystem, so that less
> >> > filesystem breakage happen when disconnecting an USB key, for 
> >> > example. I'd like to have comments about it, because as it 
> >> > seems to work fine here, I'm not used to fs drivers and could
> >> > have made mistakes.
> >> 
> >> What cases should these patches guarantee that users can unplug the
> >> USB key?  And can we guarantee the same cases by improving autofs or
> >> the similar stuff?
> >
> > Well there can be no guarantees - there will always be a race between
> > flush and hot unplug. If we're careful with write ordering, we can
> > perhaps arrange to avoid the worst sorts of corruption, provided the
> > device does the right thing when it's in the middle of an IO.
> >
> > But generally I think this is a good idea as it shrinks the window.
> 
> Things which I want to say here - do we really need the bogus
> sync-mode?

I'm not sure why you say it's bogus. Ext2 for instance has long had a
mount option similar to this and it makes sense in volatile
environments. Having the flag in the superblock seems a sensible way
of doing it as well.
 
> Current fatfs is not keeping the consistency of data on the disk at
> all.  So, after all, the data on a disk is corrupting until all
> syscalls finish, right?

This is to protect against usage patters like mv a b, oh look, it's
done, unplug. Not lots of active readers/writers.

> If so, isn't this too slow? I doubt this is good solution for this
> problem (USB key unplugging)...

Perhaps. I think the behavior should be to honor the superblock flag
by default, overridable with -o sync|async at mount.
 
> Well, it seems good as start of sync-mode though.
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Bernd Eckenfels]

In article <87pt237se1.fsf@devron.myhome.or.jp> you wrote:
> Current fatfs is not keeping the consistency of data on the disk at
> all.  So, after all, the data on a disk is corrupting until all
> syscalls finish, right?

Me thinks, fat is pretty robust, and with the small window it improves,
because most users will wait until the operation finished and unplug then.
Which is enough delay for scheduled sync, but not for delayed flushed blocks.

> If so, isn't this too slow? I doubt this is good solution for this
> problem (USB key unplugging)...

Hmm... Actually a "better" solution which involves a journalling filesystem
is not an option for most fat users becaue they need the interoperability.

So the only real solutions you have (beside educating the user to wait for
sync) is to make the corruption window smaller and perhaps do some
reordering on meta data changed to lower the dangers of corruptions.

> Well, it seems good as start of sync-mode though.

You mean, to build sync writes at the right places in the fatfs? I guess yes
that would be even better, so you have kind of soft updates for fat :)
However dont know how reliable the flash controllers are with reordering (if
they are somewhat smart)

How about having configurable sync intervalls per fs/device and default them
to 0 for removeable media?

Greetings
Bernd

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[OGAWA Hirofumi]

Matt Mackall <mpm@selenic.com> writes:

>> Things which I want to say here - do we really need the bogus
>> sync-mode?
>
> I'm not sure why you say it's bogus. Ext2 for instance has long had a
> mount option similar to this and it makes sense in volatile
> environments. Having the flag in the superblock seems a sensible way
> of doing it as well.

AFAIK, EXT2 doesn't update all metadata synchronously in sync-mode.

>> Current fatfs is not keeping the consistency of data on the disk at
>> all.  So, after all, the data on a disk is corrupting until all
>> syscalls finish, right?
>
> This is to protect against usage patters like mv a b, oh look, it's
> done, unplug. Not lots of active readers/writers.

I think we don't need synchronous update for it, probably we just need
to flush the buffers on each operations.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Andrew Morton]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
> AFAIK, EXT2 doesn't update all metadata synchronously in sync-mode.

It does.

I'm actually surprised to discover that [v]fat doesn't support `-o sync'. 
It's probably a quite practical way of handling these various hotpluggable
gadgets and would be a popular addition.

It does have the downside that it'll teach our users bad practices....

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Matt Mackall]

On Tue, Nov 23, 2004 at 10:40:02PM -0800, Andrew Morton wrote:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
> >
> > AFAIK, EXT2 doesn't update all metadata synchronously in sync-mode.
> 
> It does.
> 
> I'm actually surprised to discover that [v]fat doesn't support `-o sync'. 
> It's probably a quite practical way of handling these various hotpluggable
> gadgets and would be a popular addition.
> 
> It does have the downside that it'll teach our users bad practices....

Dunno. Right now they'll still probably have an occassional kernel
crash to contend with from device disappearing from under an fs, but
at least they won't have lost all the photos on their camera...

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Andrew Morton]

Matt Mackall <mpm@selenic.com> wrote:
>
> > It does have the downside that it'll teach our users bad practices....
> 
> Dunno. Right now they'll still probably have an occassional kernel
> crash to contend with from device disappearing from under an fs, but
> at least they won't have lost all the photos on their camera...

Yes, but after a fisaco like that, they'll remember to type `umount'!

Maybe I wasn't cut out for a career as an educator.

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 24 Nov 2004 at 06h11, Bernd Eckenfels wrote:

Hi, 

> Hmm... Actually a "better" solution which involves a journalling filesystem
> is not an option for most fat users becaue they need the interoperability.

Yes, an usb key with ext2 fs is quite useless when you have to tell your 
friends they can't give you this or that file because they run windows :)

-- 
Colin

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 23 Nov 2004 at 19h11, Matt Mackall wrote:

Hi, 

> BUG_ON(!bh);
> sync_dirty_buffer(bh);
> brelse(bh);

I wasn't sure sync_dirty_buffer and brelse checked for nullity :)

> Concept seems good, and the implementation otherwise looks good at
> first glance.

Cool :) Should I submit an updated patch to Andrew for -mm ?

Thanks for all the comments. I agree with what most people said, sync
mode is not a guarantee that everything will be fine, but it helps in
most cases.

As for the speed, from what I checked it slows down IO a bit, of course,
but that is within tolerable bounds imho. 
-- 
Colin

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Matt Mackall]

On Wed, Nov 24, 2004 at 08:34:30AM +0100, Colin Leroy wrote:
> On 23 Nov 2004 at 19h11, Matt Mackall wrote:
> 
> Hi, 
> 
> > BUG_ON(!bh);
> > sync_dirty_buffer(bh);
> > brelse(bh);
> 
> I wasn't sure sync_dirty_buffer and brelse checked for nullity :)

It may, that wasn't my point. Your original patch had BUG_ON(1) which
by itself was weird (use BUG() instead). But then it was in the else
part of an if statement. So it read like:

if (bh) {
   ...
} else {
  if (1)
     BUG(); /* stop kernel */
}

BUG is for reporting things that should never happen (otherwise you'd
actually handle them) and so should be used in such a way that they
don't complicate the code flow.

> > Concept seems good, and the implementation otherwise looks good at
> > first glance.
> 
> Cool :) Should I submit an updated patch to Andrew for -mm ?

Probably ought to go through Ogawa, if he can be convinced to take it.
Please take a look at adding -o sync and -o async options to override
the superblock flag first.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 23 Nov 2004 at 23h11, Matt Mackall wrote:

Hi, 

> Probably ought to go through Ogawa, if he can be convinced to take it.
> Please take a look at adding -o sync and -o async options to override
> the superblock flag first.

Isn't it this option that sets the superblock flag ? I didn't look at 
mount's source, but according from ext2 source and fs/namespace.c, 
this is it...

-- 
Colin

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[OGAWA Hirofumi]

Andrew Morton <akpm@osdl.org> writes:

> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>>
>> AFAIK, EXT2 doesn't update all metadata synchronously in sync-mode.
>
> It does.

Umm... I was thinking that ext2 is using the delayed-write if it can
keep the consistency of metadata on-disk.

> I'm actually surprised to discover that [v]fat doesn't support `-o sync'. 
> It's probably a quite practical way of handling these various hotpluggable
> gadgets and would be a popular addition.

OK. If peoples really want this, I don't have objections.  I'll check
the patch.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[OGAWA Hirofumi]

Colin Leroy <colin@colino.net> writes:

> @@ -764,6 +765,11 @@
>  	dir->i_atime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
>  	mark_inode_dirty(dir);
>  
> +	sb = dir->i_sb;
> +
> +	if (sb->s_flags & MS_SYNCHRONOUS)
> +		sync_dirty_buffer(bh);
> +

This bh is already released.

>  	retval = generic_file_write(filp, buf, count, ppos);
>  	if (retval > 0) {
>  		inode->i_mtime = inode->i_ctime = CURRENT_TIME;
>  		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
>  		mark_inode_dirty(inode);
> +		if (sb->s_flags & MS_SYNCHRONOUS) {
> +			bh = sb_bread(sb, MSDOS_SB(sb)->fsinfo_sector);
> +			if (bh != NULL) {
> +				sync_dirty_buffer(bh);
> +				brelse(bh);
> +			} else {
> +				BUG_ON(1);
> +			}
> +		}

FAT12/16 doesn't have FSINFO sector.  And if sb_bread() returns NULL,
we should return the valid error.

> @@ -105,4 +118,8 @@
>  	unlock_kernel();
>  	inode->i_ctime = inode->i_mtime = CURRENT_TIME;
>  	mark_inode_dirty(inode);
> +	if (sb->s_flags & MS_SYNCHRONOUS) {
> +		bh = sb_bread(sb, sbi->fsinfo_sector);
> +		sync_dirty_buffer(bh);
> +	}

Ditto.

Aren't you forgetting to update the inode and various metadata (e.g. FAT)?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 24 Nov 2004 at 23h11, OGAWA Hirofumi wrote:

Hi, 

> > +			bh = sb_bread(sb, MSDOS_SB(sb)->fsinfo_sector);
> > +			if (bh != NULL) {
> > +				sync_dirty_buffer(bh);
> > +				brelse(bh);
> > +			} else {
> > +				BUG_ON(1);
> > +			}
> > +		}
> 
> FAT12/16 doesn't have FSINFO sector.  And if sb_bread() returns NULL,
> we should return the valid error.

ok, this is probably not the correct way to get the buffer_head for a 
given inode... Do you know what I should use?

-- 
Colin

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 24 Nov 2004 at 23h11, OGAWA Hirofumi wrote:

Hi, 

> Aren't you forgetting to update the inode and various metadata (e.g. FAT)?

Don't really know what to do about this one; where should I look ?

this second patch seems a step in the right direction to me, based off your
previous comments:

Signed-off-by: Colin Leroy <colin@colino.net>
diff -ur /tmp/linux-2.6.9/fs/fat/dir.c fs/fat/dir.c
--- /tmp/linux-2.6.9/fs/fat/dir.c	2004-11-24 15:57:17.776119552 +0100
+++ fs/fat/dir.c	2004-11-24 15:58:41.664366600 +0100
@@ -760,10 +760,14 @@
 	de[1].start = cpu_to_le16(MSDOS_I(parent)->i_logstart);
 	de[1].starthi = cpu_to_le16(MSDOS_I(parent)->i_logstart>>16);
 	mark_buffer_dirty(bh);
-	brelse(bh);
 	dir->i_atime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 	mark_inode_dirty(dir);
 
+	if (dir->i_sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
+
+	brelse(bh);
+
 	return 0;
 }
 
diff -ur /tmp/linux-2.6.9/fs/fat/file.c fs/fat/file.c
--- /tmp/linux-2.6.9/fs/fat/file.c	2004-10-18 23:53:44.000000000 +0200
+++ fs/fat/file.c	2004-11-24 15:49:22.375391448 +0100
@@ -74,18 +74,30 @@
 {
 	struct inode *inode = filp->f_dentry->d_inode;
 	int retval;
+	struct super_block *sb = inode->i_sb;
 
 	retval = generic_file_write(filp, buf, count, ppos);
 	if (retval > 0) {
 		inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
 		mark_inode_dirty(inode);
+		if (sb->s_flags & MS_SYNCHRONOUS) {
+			struct buffer_head *bh;
+			loff_t i_pos = MSDOS_I(inode)->i_pos;
+			bh = sb_bread(sb, 
+				i_pos >> MSDOS_SB(sb)->dir_per_block_bits);
+			if (!bh)
+				return -EIO;
+			sync_dirty_buffer(bh);
+			brelse(bh);
+		}
 	}
 	return retval;
 }
 
 void fat_truncate(struct inode *inode)
 {
+	struct super_block *sb = inode->i_sb;
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 	const unsigned int cluster_size = sbi->cluster_size;
 	int nr_clusters;
@@ -105,4 +117,13 @@
 	unlock_kernel();
 	inode->i_ctime = inode->i_mtime = CURRENT_TIME;
 	mark_inode_dirty(inode);
+	if (sb->s_flags & MS_SYNCHRONOUS) {
+		struct buffer_head *bh;
+		loff_t i_pos = MSDOS_I(inode)->i_pos;
+		bh = sb_bread(sb, i_pos >> MSDOS_SB(sb)->dir_per_block_bits);
+		if (!bh)
+			return;
+		sync_dirty_buffer(bh);
+		brelse(bh);
+	}
 }
diff -ur /tmp/linux-2.6.9/fs/fat/inode.c fs/fat/inode.c
--- /tmp/linux-2.6.9/fs/fat/inode.c	2004-11-24 15:57:17.783118488 +0100
+++ fs/fat/inode.c	2004-11-18 15:00:55.000000000 +0100
@@ -1273,8 +1273,12 @@
 	}
 	spin_unlock(&sbi->inode_hash_lock);
 	mark_buffer_dirty(bh);
-	brelse(bh);
 	unlock_kernel();
+
+	if (sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
+	brelse(bh);
+
 	return 0;
 }
 

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[OGAWA Hirofumi]

Colin Leroy <colin@colino.net> writes:

> On 24 Nov 2004 at 23h11, OGAWA Hirofumi wrote:
>
> Hi, 
>
>> Aren't you forgetting to update the inode and various metadata (e.g. FAT)?
>
> Don't really know what to do about this one; where should I look ?
>
> this second patch seems a step in the right direction to me, based off your
> previous comments:

Ah, I understood the intention of that code now. Probably we would
need the following for writing inode.

int fat_sync_inode(struct inode *inode)
{
	return fat_write_inode(inode, 1);
}

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

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 25 Nov 2004 at 03h11, OGAWA Hirofumi wrote:

Hi, 

> int fat_sync_inode(struct inode *inode)
> {
> 	return fat_write_inode(inode, 1);
> }

Thanks for the hint. Here's a third patch.

Signed-off-by: Colin Leroy <colin@colino.net>
--- a/fs/fat/dir.c	2004-11-24 15:57:17.000000000 +0100
+++ fs/fat/dir.c	2004-11-24 21:21:51.783396408 +0100
@@ -760,10 +760,16 @@
 	de[1].start = cpu_to_le16(MSDOS_I(parent)->i_logstart);
 	de[1].starthi = cpu_to_le16(MSDOS_I(parent)->i_logstart>>16);
 	mark_buffer_dirty(bh);
-	brelse(bh);
 	dir->i_atime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 	mark_inode_dirty(dir);
 
+	if (dir->i_sb->s_flags & MS_SYNCHRONOUS) {
+		sync_dirty_buffer(bh);
+		fat_sync_inode(dir);
+	}
+
+	brelse(bh);
+
 	return 0;
 }
 
--- a/fs/fat/fatfs_syms.c	2004-11-24 15:57:17.000000000 +0100
+++ fs/fat/fatfs_syms.c	2004-11-24 21:05:18.665605752 +0100
@@ -19,6 +19,7 @@
 EXPORT_SYMBOL(fat_detach);
 EXPORT_SYMBOL(fat_build_inode);
 EXPORT_SYMBOL(fat_fill_super);
+EXPORT_SYMBOL(fat_sync_inode);
 EXPORT_SYMBOL(fat_search_long);
 EXPORT_SYMBOL(fat_scan);
 EXPORT_SYMBOL(fat_add_entries);
--- a/fs/fat/file.c	2004-10-18 23:53:44.000000000 +0200
+++ fs/fat/file.c	2004-11-24 21:06:56.271767360 +0100
@@ -74,18 +74,22 @@
 {
 	struct inode *inode = filp->f_dentry->d_inode;
 	int retval;
+	struct super_block *sb = inode->i_sb;
 
 	retval = generic_file_write(filp, buf, count, ppos);
 	if (retval > 0) {
 		inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
 		mark_inode_dirty(inode);
+		if (sb->s_flags & MS_SYNCHRONOUS)
+			fat_sync_inode(inode);
 	}
 	return retval;
 }
 
 void fat_truncate(struct inode *inode)
 {
+	struct super_block *sb = inode->i_sb;
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 	const unsigned int cluster_size = sbi->cluster_size;
 	int nr_clusters;
@@ -105,4 +109,6 @@
 	unlock_kernel();
 	inode->i_ctime = inode->i_mtime = CURRENT_TIME;
 	mark_inode_dirty(inode);
+	if (sb->s_flags & MS_SYNCHRONOUS)
+		fat_sync_inode(inode);
 }
--- a/fs/fat/inode.c	2004-11-24 15:57:17.000000000 +0100
+++ fs/fat/inode.c	2004-11-24 21:04:23.031063488 +0100
@@ -1224,6 +1224,11 @@
 	return 0;
 }
 
+int fat_sync_inode(struct inode *inode)
+{
+	return fat_write_inode(inode, 1);
+}
+
 static int fat_write_inode(struct inode *inode, int wait)
 {
 	struct super_block *sb = inode->i_sb;
@@ -1273,8 +1278,12 @@
 	}
 	spin_unlock(&sbi->inode_hash_lock);
 	mark_buffer_dirty(bh);
-	brelse(bh);
 	unlock_kernel();
+
+	if (sb->s_flags & MS_SYNCHRONOUS)
+		sync_dirty_buffer(bh);
+	brelse(bh);
+
 	return 0;
 }
 
--- a/include/linux/msdos_fs.h	2004-11-24 15:57:20.000000000 +0100
+++ b/include/linux/msdos_fs.h	2004-11-24 21:06:17.858607048 +0100
@@ -266,6 +266,7 @@
 			struct msdos_dir_entry *de, loff_t i_pos, int
*res); int fat_fill_super(struct super_block *sb, void *data, int
silent, 		   struct inode_operations *fs_dir_inode_ops,
int isvfat);+int fat_sync_inode(struct inode *inode);
 extern int fat_notify_change(struct dentry * dentry, struct iattr *
attr); 
 /* fat/misc.c */


-- 
Colin
  "That's the difference between me and the rest of the world! 
   Happiness isn't good enough for me! I demand euphoria!"
		-- Calvin

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 25 Nov 2004 at 21h11, OGAWA Hirofumi wrote:

Hi, 

> > Thanks for the hint. Here's a third patch.
> 
> Looks good as start. Thanks.

Thank you for your help. Do I push to Andrew?

-- 
Colin

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Colin Leroy]

On 25 Nov 2004 at 22h11, OGAWA Hirofumi wrote:

Hi, 

> > Thank you for your help. Do I push to Andrew?
> 
> I have some patches which conflict with this patch.  So, I'll
> submit the patches after fixing it.

Ok. I suppose you'll handle the vfat one too?

-- 
Colin

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[Robert Hardy]

On Thu, 25 Nov 2004, Colin Leroy wrote:
> On 25 Nov 2004 at 22h11, OGAWA Hirofumi wrote:
>>> Thank you for your help. Do I push to Andrew?
>> I have some patches which conflict with this patch.  So, I'll
>> submit the patches after fixing it.
> Ok. I suppose you'll handle the vfat one too?

I am very interested in seeing this sync vfat patch committed.

Can you please provide current status on vfat sync support?

Also are there newer patches for the vfat support?

Regards,
Rob

-- 
---------------------"Happiness is understanding."----------------------
Robert Hardy, B.Eng Computer Systems                  C.E.O. Webcon Inc.
rhardy <at> webcon <dot> ca    GPG Key available

Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

[OGAWA Hirofumi]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

Robert Hardy <rhardy@webcon.ca> writes:

> On Thu, 25 Nov 2004, Colin Leroy wrote:
>> On 25 Nov 2004 at 22h11, OGAWA Hirofumi wrote:
>>>> Thank you for your help. Do I push to Andrew?
>>> I have some patches which conflict with this patch.  So, I'll
>>> submit the patches after fixing it.
>> Ok. I suppose you'll handle the vfat one too?
>
> I am very interested in seeing this sync vfat patch committed.
>
> Can you please provide current status on vfat sync support?
>
> Also are there newer patches for the vfat support?

Currently, the many buffers is still not writing synchronously. And I'm
rewriting the update-path for an update dependency and some optimization,
now.

Current patches is here.

[I'm offline from today to January 2 or 3.]

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


[-- Attachment #2: patchset.tar.bz2 --]
[-- Type: application/octet-stream, Size: 33467 bytes --]