LinuxLists.cc - [PATCH] let fat handle MS

[permalink] [raw]

Subject: [PATCH] let vfat handle MS_SYNCHRONOUS flag

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

Hi,
Same patch for vfat.

Signed-off-by: Colin Leroy <[email protected]>
--- 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:

2004-11-23 19:37:09

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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

2004-11-23 20:29:35

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

Colin Leroy <[email protected]> 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 <[email protected]>

2004-11-24 03:17:52

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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.

2004-11-24 03:20:31

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

On Wed, Nov 24, 2004 at 05:24:36AM +0900, OGAWA Hirofumi wrote:
> Colin Leroy <[email protected]> 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.

2004-11-24 05:01:49

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

Matt Mackall <[email protected]> writes:

> On Wed, Nov 24, 2004 at 05:24:36AM +0900, OGAWA Hirofumi wrote:
>> Colin Leroy <[email protected]> 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 <[email protected]>

2004-11-24 05:36:51

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

On Wed, Nov 24, 2004 at 02:00:54PM +0900, OGAWA Hirofumi wrote:
> Matt Mackall <[email protected]> writes:
>
> > On Wed, Nov 24, 2004 at 05:24:36AM +0900, OGAWA Hirofumi wrote:
> >> Colin Leroy <[email protected]> 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 <[email protected]>

--
Mathematics is the supreme nostalgia of our time.

2004-11-24 05:41:38

by Bernd Eckenfels

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

In article <[email protected]> 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

2004-11-24 06:26:39

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

Matt Mackall <[email protected]> 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 <[email protected]>

2004-11-24 06:40:37

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

OGAWA Hirofumi <[email protected]> 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....

2004-11-24 06:57:51

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

Matt Mackall <[email protected]> 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.

2004-11-24 07:40:05

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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

2004-11-24 07:40:05

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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

2004-11-24 07:53:38

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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.

2004-11-24 08:41:27

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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

2004-11-24 10:42:59

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

On Tue, Nov 23, 2004 at 10:40:02PM -0800, Andrew Morton wrote:
> OGAWA Hirofumi <[email protected]> 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.

2004-11-24 14:28:28

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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

2004-11-24 14:47:12

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

Andrew Morton <[email protected]> writes:

> OGAWA Hirofumi <[email protected]> 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 <[email protected]>

2004-11-24 14:47:14

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

Colin Leroy <[email protected]> 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 <[email protected]>

2004-11-24 15:09:40

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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 <[email protected]>
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;
}

2004-11-24 19:13:04

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

Colin Leroy <[email protected]> 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 <[email protected]>

2004-11-25 00:35:17

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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 <[email protected]>
--- 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

2004-11-26 20:16:35

[permalink] [raw]

Subject: Re: [PATCH] let fat handle MS_SYNCHRONOUS flag

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

2004-11-27 06:58:01