[PATCH] exofs: Avoid using file_fsync()

[PATCH] exofs: Avoid using file_fsync()

[Boaz Harrosh]

Please review the below patch. I will try to push it through the
exofs tree for 2.6.31 

---
Subject: [PATCH] exofs: Avoid using file_fsync()

The use of file_fsync() in exofs_file_sync() is not necessary since it
does some extra stuff not used by exofs. Open code just the parts that
are currently needed.

TODO: Farther optimization can be done to sync the sb only on inode
update of new files, Usually the sb update is not needed in exofs.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exofs/file.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/exofs/file.c b/fs/exofs/file.c
index 6ed7fe4..9e60d68 100644
--- a/fs/exofs/file.c
+++ b/fs/exofs/file.c
@@ -47,16 +47,24 @@ static int exofs_file_fsync(struct file *filp, struct dentry *dentry,
 {
 	int ret;
 	struct address_space *mapping = filp->f_mapping;
+	struct inode * inode = dentry->d_inode;
+	struct super_block * sb;
 
 	ret = filemap_write_and_wait(mapping);
 	if (ret)
 		return ret;
 
-	/*Note: file_fsync below also calles sync_blockdev, which is a no-op
-	 *      for exofs, but other then that it does sync_inode and
-	 *      sync_superblock which is what we need here.
-	 */
-	return file_fsync(filp, dentry, datasync);
+	/* sync the inode attributes */
+	ret = write_inode_now(inode, 0);
+
+	/* This is a good place to write the sb */
+	/* TODO: Sechedule an sb-sync on create */
+	sb = inode->i_sb;
+	lock_super(sb);
+	if (sb->s_dirt && sb->s_op->write_super)
+		sb->s_op->write_super(sb);
+	unlock_super(sb);
+	return ret;
 }
 
 static int exofs_flush(struct file *file, fl_owner_t id)
-- 
1.6.2.1

Re: [PATCH] exofs: Avoid using file_fsync()

[Christoph Hellwig]

On Mon, Jun 15, 2009 at 04:31:01PM +0300, Boaz Harrosh wrote:
> +	ret = write_inode_now(inode, 0);

You shouldn't need a write_inode_now, but rather just a similar
sync_inode call as in ext2 or the new simple_fsync as data was
already written by the VFS.

> +	/* This is a good place to write the sb */
> +	/* TODO: Sechedule an sb-sync on create */
> +	sb = inode->i_sb;
> +	lock_super(sb);
> +	if (sb->s_dirt && sb->s_op->write_super)
> +		sb->s_op->write_super(sb);
> +	unlock_super(sb);

fsync is not a really good place for a sb write normally.  What metadata
in the superblock is needed related to syncing a single file in btrfs?

Re: [PATCH] exofs: Avoid using file_fsync()

[Boaz Harrosh]

On 06/15/2009 04:50 PM, Christoph Hellwig wrote:
> On Mon, Jun 15, 2009 at 04:31:01PM +0300, Boaz Harrosh wrote:
>> +	ret = write_inode_now(inode, 0);
> 
> You shouldn't need a write_inode_now, but rather just a similar
> sync_inode call as in ext2 or the new simple_fsync as data was
> already written by the VFS.
> 

Yes you are right I forgot to do this. I was sitting on this patch
for after-the-merge exactly for that reason, but ended up sending
it like this.

Thanks will fix.

>> +	/* This is a good place to write the sb */
>> +	/* TODO: Sechedule an sb-sync on create */
>> +	sb = inode->i_sb;
>> +	lock_super(sb);
>> +	if (sb->s_dirt && sb->s_op->write_super)
>> +		sb->s_op->write_super(sb);
>> +	unlock_super(sb);
> 
> fsync is not a really good place for a sb write normally.  What metadata
> in the superblock is needed related to syncing a single file in btrfs?
> 

You are absolutely right, but I will leave it like this for now.

The thing is that superblock in exofs keeps a cache of the current
number of files, and is not properly scheduled for write after update,
so this is the place to catch it for now. It was done like this until
today. When I have time to properly test it, I will drop this from here.

Thanks for the review
Boaz

Re: [PATCH] exofs: Avoid using file_fsync()

[Boaz Harrosh]

On 06/15/2009 04:50 PM, Christoph Hellwig wrote:
> On Mon, Jun 15, 2009 at 04:31:01PM +0300, Boaz Harrosh wrote:
>> +	ret = write_inode_now(inode, 0);
> 
> You shouldn't need a write_inode_now, but rather just a similar
> sync_inode call as in ext2 or the new simple_fsync as data was
> already written by the VFS.
> 

I looked into it some more and need your advise please?

It looks simple_fsync, sync_inode, and write_inode_now all do
__writeback_single_inode() inside. But write_inode_now also
waits.

I think I want to wait here, it's my close-to-open barrier.

No?

Thanks
Boaz

[PATCH version 2] exofs: Avoid using file_fsync()

[Boaz Harrosh]

I'll test with below patch and also with Christoph version which
does:

diff --git a/fs/exofs/file.c b/fs/exofs/file.c
index 0c13650..2c8abdd 100644
--- a/fs/exofs/file.c
+++ b/fs/exofs/file.c
@@ -46,16 +46,10 @@ static int exofs_file_fsync(struct file *filp, struct dentry *dentry,
 			    int datasync)
 {
 	int ret;
-	struct address_space *mapping = filp->f_mapping;
 	struct inode * inode = dentry->d_inode;
 	struct super_block * sb;
 
-	ret = filemap_write_and_wait(mapping);
-	if (ret)
-		return ret;
-
-	/* sync the inode attributes */
-	ret = write_inode_now(inode, 1);
+	ret = simple_fsync(filp, dentry, 1);
 
 	/* This is a good place to write the sb */
 	/* TODO: Sechedule an sb-sync on create */

On top of the below one. See which one feels better. What should I test for?

Thanks
Boaz

---
The use of file_fsync() in exofs_file_sync() is not necessary since it
does some extra stuff not used by exofs. Open code just the parts that
are currently needed.

TODO: Farther optimization can be done to sync the sb only on inode
update of new files, Usually the sb update is not needed in exofs.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exofs/exofs.h |    3 +++
 fs/exofs/file.c  |   17 ++++++++++++-----
 fs/exofs/super.c |    2 +-
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/exofs/exofs.h b/fs/exofs/exofs.h
index 0fd4c78..5d15512 100644
--- a/fs/exofs/exofs.h
+++ b/fs/exofs/exofs.h
@@ -156,6 +156,9 @@ ino_t exofs_parent_ino(struct dentry *child);
 int exofs_set_link(struct inode *, struct exofs_dir_entry *, struct page *,
 		    struct inode *);
 
+/* super.c               */
+int exofs_sync_fs(struct super_block *sb, int wait);
+
 /*********************
  * operation vectors *
  *********************/
diff --git a/fs/exofs/file.c b/fs/exofs/file.c
index 6ed7fe4..0c13650 100644
--- a/fs/exofs/file.c
+++ b/fs/exofs/file.c
@@ -47,16 +47,23 @@ static int exofs_file_fsync(struct file *filp, struct dentry *dentry,
 {
 	int ret;
 	struct address_space *mapping = filp->f_mapping;
+	struct inode * inode = dentry->d_inode;
+	struct super_block * sb;
 
 	ret = filemap_write_and_wait(mapping);
 	if (ret)
 		return ret;
 
-	/*Note: file_fsync below also calles sync_blockdev, which is a no-op
-	 *      for exofs, but other then that it does sync_inode and
-	 *      sync_superblock which is what we need here.
-	 */
-	return file_fsync(filp, dentry, datasync);
+	/* sync the inode attributes */
+	ret = write_inode_now(inode, 1);
+
+	/* 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);
+
+	return ret;
 }
 
 static int exofs_flush(struct file *file, fl_owner_t id)
diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index 8216c5b..e4fa0ce 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -200,7 +200,7 @@ static const struct export_operations exofs_export_ops;
 /*
  * Write the superblock to the OSD
  */
-static int exofs_sync_fs(struct super_block *sb, int wait)
+int exofs_sync_fs(struct super_block *sb, int wait)
 {
 	struct exofs_sb_info *sbi;
 	struct exofs_fscb *fscb;
-- 
1.6.2.1

Re: [PATCH] exofs: Avoid using file_fsync()

[Jeff Garzik]

Christoph Hellwig wrote:
> On Mon, Jun 15, 2009 at 04:31:01PM +0300, Boaz Harrosh wrote:
>> +	ret = write_inode_now(inode, 0);
> 
> You shouldn't need a write_inode_now, but rather just a similar
> sync_inode call as in ext2 or the new simple_fsync as data was
> already written by the VFS.

And then there is the issue of needing to flush the storage device, 
otherwise fsync(2) does not provide relevant guarantees...

	Jeff

Re: [PATCH] exofs: Avoid using file_fsync()

[Boaz Harrosh]

On 06/15/2009 08:21 PM, Jeff Garzik wrote:
> Christoph Hellwig wrote:
>> On Mon, Jun 15, 2009 at 04:31:01PM +0300, Boaz Harrosh wrote:
>>> +	ret = write_inode_now(inode, 0);
>> You shouldn't need a write_inode_now, but rather just a similar
>> sync_inode call as in ext2 or the new simple_fsync as data was
>> already written by the VFS.
> 
> And then there is the issue of needing to flush the storage device, 
> otherwise fsync(2) does not provide relevant guarantees...
> 
> 	Jeff
> 

Right thanks, I need to do this too, will do

Boaz