ext4: sync the directory inode in ext4_sync_parent()

ext4 has taken the stance that, in the absence of a journal, when an fsync/fdatasync of an inode is done, the parent directory should be sync'ed if this inode entry is new. ext4_sync_parent(), which implements this, does indeed sync the dirent pages for parent directories, but it does not sync the directory *inode*. This patch fixes this.

I tested this using a power fail test, which panics a machine running a file server getting requests from a client. Without this patch, on about every other test run, the server is missing many, many files that had been synced. With this patch, on > 6 runs, I see zero files being lost.

Signed-off-by Curt Wohlgemuth <[email protected]>

---
 fs/ext4/fsync.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

fs/ext4/fsync.c +1 -0

index 7f74019..a276348 100644
@@ -128,6 +128,7 @@ extern int ext4_flush_completed_IO(struct inode *inode)
 static void ext4_sync_parent(struct inode *inode)
 {
 	struct dentry *dentry = NULL;
+	int ret;
 
 	while (inode && ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY)) {
 		ext4_clear_inode_state(inode, EXT4_STATE_NEWENTRY);

@@ -136,7 +137,15 @@ static void ext4_sync_parent(struct inode *inode)
 		if (!dentry || !dentry->d_parent || !dentry->d_parent->d_inode)
 			break;

fs/ext4/fsync.c +1 -0

+		}
 	}
 }
 

Quoted 37 lines · Curt Wohlgemuth

ext4 has taken the stance that, in the absence of a journal, when an fsync/fdatasync of an inode is done, the parent directory should be sync'ed if this inode entry is new. ext4_sync_parent(), which implements this, does indeed sync the dirent pages for parent directories, but it does not sync the directory *inode*.  This patch fixes this.

I tested this using a power fail test, which panics a machine running a file server getting requests from a client.  Without this patch, on about every other test run, the server is missing many, many files that had been synced. With this patch, on > 6 runs, I see zero files being lost.

Signed-off-by Curt Wohlgemuth <[email protected]>

---
 fs/ext4/fsync.c |   11 ++++++++++-

 1 files changed, 10 insertions(+), 1 deletions(-)

fs/ext4/fsync.c +3 -1

index 7f74019..a276348 100644
@@ -128,6 +128,7 @@ extern int ext4_flush_completed_IO(struct inode *inode)
  static void ext4_sync_parent(struct inode *inode)
  {
       struct dentry *dentry = NULL;
+     int ret;
 
       while (inode && ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY)) {
               ext4_clear_inode_state(inode, EXT4_STATE_NEWENTRY);

@@ -136,7 +137,15 @@ static void ext4_sync_parent(struct inode *inode)
               if (!dentry || !dentry->d_parent || !dentry->d_parent->d_inode)
                       break;
               inode = dentry->d_parent->d_inode;
-             sync_mapping_buffers(inode->i_mapping);
+             ret = sync_mapping_buffers(inode->i_mapping);
+             if (! ret) {

fs/ext4/fsync.c static void ext4_sync_parent(struct inode *inode) · line 147

Quoted 6 lines

fs/ext4/fsync.c +6 -0

+                     struct writeback_control wbc = {
+                             .sync_mode = WB_SYNC_ALL,
+                             .nr_to_write = 0, /* metadata-only; caller
+                                                  takes care of data */
+                     };
+                     (void)sync_inode(inode, &wbc);

I know ext4_sync_parent was a void already, but why don't we send errors back up through ext4_sync_file?

-Eric

Quoted 4 lines

fs/ext4/fsync.c +1 -0

+             }
       }
  }
 

Well, you could argue that a failure to sync the parent shouldn't cause the inode's fsync() to fail, but I probably wouldn't :-) . I'll resend a patch that implements this.

Thanks,
Curt