miss-sync issues on sync mount (miss-sync on utime)

fs/fat/file.c int fat_notify_change(struct dentry *den · line 189

Quoted 3 lines · Hiroyuki

fs/fat/file.c +3 -0

+	if ( (!error) && IS_SYNC(inode)) {
+		error = write_inode_now(inode, 1);
+	}

We don't need to sync the data pages at all here. And I think it is not right place for doing this. If we need this, since we need to see O_SYNC for fchxxx() VFS would be right place to do it.

But, personally, I'd like to kill the "-o sync" stuff for these independent meta data rather. Then...

Review

Quoted 3 lines · Hiroyuki

(quoted diff) +3 -0

+	if ( (!error) && IS_SYNC(inode)) {
+		error = write_inode_now(inode, 1);
+	}

We don't need to sync the data pages at all here. And I think it is not right place for doing this. If we need this, since we need to see O_SYNC for fchxxx() VFS would be right place to do it.

But, personally, I'd like to kill the "-o sync" stuff for these independent meta data rather. Then...

I revise a previous patch. Now checking dirty flag is done at vfs layer, as OGAWA-san said. I realized ext2_sync_inode() is good for syncing inode without it's data. I moved it to vfs layer and rename it to sync_inode_wodata().

The first patch attached here is changes on vfs layer. Second patch attached at the next mail is changes on ext2 fs.

Quoted 9 lines · OGAWA Hirofumi

Review

Quoted 3 lines · Hiroyuki

(quoted diff) +3 -0

+    if ( (!error) && IS_SYNC(inode)) {
+        error = write_inode_now(inode, 1);
+    }

Quoted 6 lines

We don't need to sync the data pages at all here. And I think it is not right place for doing this. If we need this, since we need to see O_SYNC for fchxxx() VFS would be right place to do it.

But, personally, I'd like to kill the "-o sync" stuff for these independent meta data rather. Then...

------------------------------------------------------------------------

I revise a previous patch. Now checking dirty flag is done at vfs layer, as OGAWA-san said. I realized ext2_sync_inode() is good for syncing inode without it's data. I moved it to vfs layer and rename it to sync_inode_wodata().

The first patch attached here is changes on vfs layer. Second patch attached at the next mail is changes on ext2 fs.

Quoted 9 lines · OGAWA Hirofumi

Review

Quoted 3 lines · Hiroyuki

(quoted diff) +3 -0

+    if ( (!error) && IS_SYNC(inode)) {
+        error = write_inode_now(inode, 1);
+    }

Quoted 6 lines

We don't need to sync the data pages at all here. And I think it is not right place for doing this. If we need this, since we need to see O_SYNC for fchxxx() VFS would be right place to do it.

But, personally, I'd like to kill the "-o sync" stuff for these independent meta data rather. Then...

------------------------------------------------------------------------

This patch adds inode-sync after attribute changes, if needed.

* fs-sync-attr.patch for 2.6.13

 fs/fs-writeback.c      |   19 +++++++++++++++++++
 fs/open.c              |   12 ++++++++++++
 include/linux/fs.h     |    1 +
 4 files changed, 32 insertions(+)

Signed-off-by Hiroyuki Machida <[email protected]>

This patch adds inode-sync after attribute changes, if needed.

* fs-sync-attr.patch for 2.6.13

 fs/fs-writeback.c      |   19 +++++++++++++++++++
 fs/open.c              |   12 ++++++++++++
 include/linux/fs.h     |    1 +
 4 files changed, 32 insertions(+)

Signed-off-by Hiroyuki Machida <[email protected]>

fs/fs-writeback.c +8 -0

@@ -593,6 +593,25 @@ int sync_inode(struct inode *inode, stru
 EXPORT_SYMBOL(sync_inode);
 
 /**
+ * sync_inode_wodata - sync(write and wait) inode to disk, without it's data.
+ * @inode: the inode to sync
+ *
+ * sync_inode_wodata() will write an inode  then wait.  It will also
+ * correctly update the inode on its superblock's dirty inode lists 
+ * and will update inode->i_state.
+ *
+ * The caller must have a ref on the inode.

Quoted 13 lines

/**
+ * sync_inode_wodata - sync(write and wait) inode to disk, without it's data.
+ * @inode: the inode to sync
+ *
+ * sync_inode_wodata() will write an inode then wait. It will also
+ * correctly update the inode on its superblock's dirty inode lists
+ * and will update inode->i_state.
+ *
+ * The caller must have a ref on the inode.
+ */

fs/fs-writeback.c +3 -0

+int sync_inode_wodata(struct inode *inode)
+{
+	struct writeback_control wbc = {

fs/fs-writeback.c int sync_inode(struct inode *inode, stru · line 613

Quoted 6 lines

fs/fs-writeback.c +6 -0

+		.sync_mode = WB_SYNC_ALL, /* wait */
+		.nr_to_write = 0,/* no data to be written */
+	};
+	return sync_inode(inode, &wbc);
+}
+

I think this function duplicates write_inode_now()?

Quoted 13 lines · Andrew Morton

Quoted 13 lines

/**
+ * sync_inode_wodata - sync(write and wait) inode to disk, without it's data.
+ * @inode: the inode to sync
+ *
+ * sync_inode_wodata() will write an inode then wait. It will also
+ * correctly update the inode on its superblock's dirty inode lists
+ * and will update inode->i_state.
+ *
+ * The caller must have a ref on the inode.
+ */

(quoted diff) +3 -0

+int sync_inode_wodata(struct inode *inode)
+{
+	struct writeback_control wbc = {

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+		.sync_mode = WB_SYNC_ALL, /* wait */
+		.nr_to_write = 0,/* no data to be written */
+	};
+	return sync_inode(inode, &wbc);
+}
+

I think this function duplicates write_inode_now()?

write_inode_now() seems to write data pages, but this doesn't write (.nr_to_write = 0).

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Isn't write_inode_now() buggy? If !mapping_cap_writeback_dirty() we should still write the inode itself?

Quoted 2 lines

Isn't write_inode_now() buggy? If !mapping_cap_writeback_dirty() we should still write the inode itself?

Indeed. It seems we should write the dirty inode to backing device's buffers. sync_sb_inodes() too? If so, really buggy.. I'll check it.

Quoted 3 lines · Andrew Morton

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Indeed. We use the generic_osync_inode() for it?

Quoted 8 lines · OGAWA Hirofumi

Quoted 3 lines · Andrew Morton

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Quoted 1 line

Indeed. We use the generic_osync_inode() for it?

Please let me confirm.
Using generic_osync_inode(inode, NULL, OSYNC_INODE) instaed of sync_inode_wodata(inode) is peferable for changes on fs/open.c, even it would write data. Is it correct?

Quoted 13 lines · Hiroyuki

Quoted 9 lines · OGAWA Hirofumi

Quoted 3 lines · Andrew Morton

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Quoted 7 lines

Quoted 1 line

Indeed. We use the generic_osync_inode() for it?

Please let me confirm.
Using generic_osync_inode(inode, NULL, OSYNC_INODE) instaed of sync_inode_wodata(inode) is peferable for changes on fs/open.c, even it would write data. Is it correct?

I don't know. It depends on what you're actually trying to do, and I don't recall anyone having described that!

Quoted 5 lines · OGAWA Hirofumi

Quoted 3 lines · Andrew Morton

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Quoted 1 line

Indeed. We use the generic_osync_inode() for it?

Please let me confirm.
Using generic_osync_inode(inode, NULL, OSYNC_INODE) instaed of sync_inode_wodata(inode) is peferable for changes on fs/open.c, even it would write data. Is it correct?

Quoted 11 lines · Hiroyuki

Quoted 8 lines · OGAWA Hirofumi

Quoted 3 lines · Andrew Morton

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Indeed. We use the generic_osync_inode() for it?

Quoted 4 lines

Please let me confirm.
Using generic_osync_inode(inode, NULL, OSYNC_INODE) instaed of sync_inode_wodata(inode) is peferable for changes on fs/open.c, even it would write data. Is it correct?

No, I only thought the interface is good. I don't know why it writes data pages even if OSYNC_INODE only.

Quoted 23 lines · OGAWA Hirofumi

Quoted 10 lines · Hiroyuki

Quoted 8 lines · OGAWA Hirofumi

Quoted 3 lines · Andrew Morton

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Quoted 7 lines

Quoted 1 line

Indeed. We use the generic_osync_inode() for it?

Please let me confirm.
Using generic_osync_inode(inode, NULL, OSYNC_INODE) instaed of sync_inode_wodata(inode) is peferable for changes on fs/open.c, even it would write data. Is it correct?

Quoted 2 lines

No, I only thought the interface is good. I don't know why it writes data pages even if OSYNC_INODE only.

I checked 2.6.13 tree, following functions call generic_osync_inode(). However noone calls it with OSYNC_INODE. SO I can't find intention of it's usage.

Does anyone know why generic_osync_inode() trys to write data page, even if OSYNC_INODE only passed ?

- fs/reiserfs/file.c
reiserfs_file_write() OSYNC_METADATA | OSYNC_DATA
- mm/filemap.c
sync_page_range() OSYNC_METADATA
sync_page_range_nolock() OSYNC_METADATA
generic_file_direct_write OSYNC_METADATA

Quoted 23 lines · OGAWA Hirofumi

Quoted 10 lines · Hiroyuki

Quoted 8 lines · OGAWA Hirofumi

Quoted 3 lines · Andrew Morton

However there's not much point in writing a brand-new function when write_inode_now() almost does the right thing. We can share the implementation within fs-writeback.c.

Quoted 7 lines

Quoted 1 line

Indeed. We use the generic_osync_inode() for it?

Please let me confirm.
Using generic_osync_inode(inode, NULL, OSYNC_INODE) instaed of sync_inode_wodata(inode) is peferable for changes on fs/open.c, even it would write data. Is it correct?

Quoted 2 lines

No, I only thought the interface is good. I don't know why it writes data pages even if OSYNC_INODE only.

I checked 2.6.13 tree, following functions call generic_osync_inode(). However noone calls it with OSYNC_INODE. SO I can't find intention of it's usage.

Does anyone know why generic_osync_inode() trys to write data page, even if OSYNC_INODE only passed ?

- fs/reiserfs/file.c
reiserfs_file_write() OSYNC_METADATA | OSYNC_DATA
- mm/filemap.c
sync_page_range() OSYNC_METADATA
sync_page_range_nolock() OSYNC_METADATA
generic_file_direct_write OSYNC_METADATA

Isn't write_inode_now() buggy? If !mapping_cap_writeback_dirty() we should still write the inode itself?

diff -puN fs/fs-writeback.c~write_inode_now-write-inode-if-not-bdi_cap_no_writeback fs/fs-writeback.c