exofs_file_fsync

Various odd things going on here:

- no checks for I_DIRTY and friends, so it will always write out data
- filemap_write_and_wait is superflous, as it's already done by the caller
- write_inode_now is overkill as it also writes out data, better use sync_inode in a similar way to generic_file_fsync

Quoted 7 lines · Christoph Hellwig

Various odd things going on here:

- no checks for I_DIRTY and friends, so it will always write out data
- filemap_write_and_wait is superflous, as it's already done by the caller
- write_inode_now is overkill as it also writes out data, better use sync_inode in a similar way to generic_file_fsync

I'll look into it, but I think what I did here was to effectively "data sync" because I wanted a data sync on close and that was the only vector I already had that's called on close.

Quoted 9 lines · Boaz Harrosh

Quoted 7 lines · Christoph Hellwig

Various odd things going on here:

- no checks for I_DIRTY and friends, so it will always write out data
- filemap_write_and_wait is superflous, as it's already done by the caller
- write_inode_now is overkill as it also writes out data, better use sync_inode in a similar way to generic_file_fsync

Quoted 3 lines

I'll look into it, but I think what I did here was to effectively "data sync" because I wanted a data sync on close and that was the only vector I already had that's called on close.

fsync won't get called at close time. ->release is called on last close and ->flush on every close.

OK, I was just looking at that. thanks you saved me some digging. should I just open-code the generic_file_fsync minus the blocks thing then?

I'm busy with the truncate stuff, but I'll do this next. Do you need this ASAP?

Quoted 3 lines · Boaz Harrosh

OK, I was just looking at that. thanks you saved me some digging. should I just open-code the generic_file_fsync minus the blocks thing then?

sync_mapping_buffers is a no-op for you so you can keep it. The other difference is that you sync out the superblock at the end of your fsync implementation. That is rather unusual, but I don't know enough about exofs if you really need to update data in the superblock to commit file data to disk.

Quoted 2 lines

I'm busy with the truncate stuff, but I'll do this next. Do you need this ASAP?

I just noticed it while walking through the fsync implementations.

Quoted 11 lines · Christoph Hellwig

Quoted 3 lines · Boaz Harrosh

OK, I was just looking at that. thanks you saved me some digging. should I just open-code the generic_file_fsync minus the blocks thing then?

sync_mapping_buffers is a no-op for you so you can keep it. The other difference is that you sync out the superblock at the end of your fsync implementation. That is rather unusual, but I don't know enough about exofs if you really need to update data in the superblock to commit file data to disk.

Quoted 5 lines

Quoted 2 lines

I'm busy with the truncate stuff, but I'll do this next. Do you need this ASAP?

I just noticed it while walking through the fsync implementations.

OK Chritoff I would need your help Please.

It looks like what I need exactly is:
write_inode_now(inode, sync)

But write_inode_now() has one extra hunk over generic_file_fsync:

	if (sync)
		inode_sync_wait(inode);

Do you think I can get in trouble calling it from ->fsync

I don't like generic_file_fsync because it does not write my data since I don't have buffer_heads.

OK, I'm totally lost what does ->fsync need to do? only write the inode or the pages as well?

Boaz
---
git diff --stat -p -M fs/exofs/file.c

 fs/exofs/file.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

fs/exofs/file.c +0 -0

index f9bfe2b..9b3555e 100644

fs/exofs/file.c +2 -2

 	/* sync the inode attributes */
-	ret = write_inode_now(inode, 1);
+	ret = write_inode_now(inode, datasync);
 
 	/* This is a good place to write the sb */
 	/* TODO: Sechedule an sb-sync on create */
 	sb = inode->i_sb;
 	if (sb->s_dirt)
-		exofs_sync_fs(sb, 1);
+		exofs_sync_fs(sb, datasync);
 
 	return ret;
 }

You think I also need a:

	if (!(inode->i_state & I_DIRTY))
		return ret;
	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
		return ret;

before the write_inode_now call ?

OK Chritoff I would need your help Please.

It looks like what I need exactly is:
write_inode_now(inode, sync)

But write_inode_now() has one extra hunk over generic_file_fsync:

	if (sync)
		inode_sync_wait(inode);

Do you think I can get in trouble calling it from ->fsync

I don't like generic_file_fsync because it does not write my data since I don't have buffer_heads.

OK, I'm totally lost what does ->fsync need to do? only write the inode or the pages as well?

The pages are written by vfs_fsync_range. You need to make sure that the inode back, or if the datasync flag is set only changes to the inode require to find that data are written back. That's basically a messy wording for you can skip dirtiness of timestamp updates if the datasync flag is set.

Note that your patch below is very wrong for this, as it does a lot of asynchronous activity if the datasync flag is not set.

As per Christoph advise: no need to call filemap_write_and_wait(). In exofs all metadata is at the inode so just writing the pages and inode is all is needed. fsync implies this must be done synchronously.

FIXME: remove the sb_sync and fix that sb_update better.

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/exofs/file.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

fs/exofs/file.c +5 -3

index f9bfe2b..54bb17d 100644
@@ -30,9 +30,6 @@
  * along with exofs; if not, write to the Free Software
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
-
-#include <linux/buffer_head.h>
-
 #include "exofs.h"
 
 static int exofs_release_file(struct inode *inode, struct file *filp)

@@ -40,19 +37,25 @@ static int exofs_release_file(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/* exofs_file_fsync - flush the inode to disk
+ *
+ *   @datasync is not used. All metadata is written in one place regardless.
+ *   the writeout is synchronous
+ */
 static int exofs_file_fsync(struct file *filp, int datasync)

fs/exofs/file.c +2 -6

+	};
 
-	ret = filemap_write_and_wait(mapping);
-	if (ret)
-		return ret;
-
-	/* sync the inode attributes */
-	ret = write_inode_now(inode, 1);
+	ret = sync_inode(inode, &wbc);
 
 	/* This is a good place to write the sb */
 	/* TODO: Sechedule an sb-sync on create */

+/* exofs_file_fsync - flush the inode to disk
+ *
+ * @datasync is not used. All metadata is written in one place regardless.
+ * the writeout is synchronous
+ */

Quoted 5 lines · Boaz Harrosh

+/* exofs_file_fsync - flush the inode to disk
+ *
+ * @datasync is not used. All metadata is written in one place regardless.
+ * the writeout is synchronous
+ */

You still want to check it. It's really only need for checking the various dirty flags in the inode, which you're still missing.

fs/exofs/file.c static int exofs_release_file(struct inode *inode, struct file *filp) · line 53

Quoted 3 lines

fs/exofs/file.c +3 -0

+	struct writeback_control wbc = {
+		.sync_mode = WB_SYNC_ALL,
+		.nr_to_write = LONG_MAX,

By setting a nr_to_write you still write out data (at least in theory).

I'd recommend just copying the code from generic_file_fsync..

Quoted 6 lines

fs/exofs/file.c static int exofs_release_file(struct inode *inode, struct file *filp) · line 53

Quoted 3 lines

fs/exofs/file.c +3 -0

+	struct writeback_control wbc = {
+		.sync_mode = WB_SYNC_ALL,
+		.nr_to_write = LONG_MAX,

By setting a nr_to_write you still write out data (at least in theory).

So when do I also sync the data? is that done for me at the VFS layer?

Quoted 2 lines

I'd recommend just copying the code from generic_file_fsync..

I was actually mimicking the code from nfs/write.c which has similar semantics as mine.

And if so then I'll need to not reuse the above in .flush

Quoted 8 lines · Boaz Harrosh

Quoted 6 lines

fs/exofs/file.c static int exofs_release_file(struct inode *inode, struct file *filp) · line 53

Quoted 3 lines

fs/exofs/file.c +3 -0

+	struct writeback_control wbc = {
+		.sync_mode = WB_SYNC_ALL,
+		.nr_to_write = LONG_MAX,

By setting a nr_to_write you still write out data (at least in theory).

So when do I also sync the data? is that done for me at the VFS layer?

You never have to. vfs_fsync_range does the data writeout for you.

Quoted 5 lines

Quoted 2 lines

I'd recommend just copying the code from generic_file_fsync..

I was actually mimicking the code from nfs/write.c which has similar semantics as mine.

nfs code has no good reason to do that, at least when called from ->fsync.

Quoted 1 line

And if so then I'll need to not reuse the above in .flush

I would recommend keeping the ->flush code separate.

+/* exofs_file_fsync - flush the inode to disk
+ *
+ * @datasync is not used. All metadata is written in one place regardless.
+ * the writeout is synchronous
+ */

Quoted 5 lines · Boaz Harrosh

+/* exofs_file_fsync - flush the inode to disk
+ *
+ * @datasync is not used. All metadata is written in one place regardless.
+ * the writeout is synchronous
+ */

It actually is used now :)

fs/exofs/file.c static int exofs_file_fsync(struct file *filp, int datasync) · line 75

Quoted 7 lines

static int exofs_flush(struct file *file, fl_owner_t id) {

fs/exofs/file.c +2 -2

-	exofs_file_fsync(file, 1);
+	int ret = vfs_fsync_range(file, 0, LLONG_MAX, 0);
 	/* TODO: Flush the OSD target */
-	return 0;
+	return ret;

why not just:

	return vfs_fsync(file, 0);