Re: [PATCH 0/12] make ->sync_fs mandatory

[Boaz Harrosh <bharrosh@panasas.com>]

On 06/08/2009 05:45 PM, Al Viro wrote:
> On Mon, Jun 08, 2009 at 10:02:52AM +0200, Christoph Hellwig wrote:
>> Currently the superblock methods are a bit confusing.  There is the
>> newer ->sync_fs method which is unconditionally called from
>> __sync_filesystem for data integrity writebacks.  And there is the older
>> ->write_super which is called from pdflush for the periodic superblock
>> writeback, but also from __sync_filesystem just before calling
>> ->sync_fs. (And still from file_fsync, but that is about to go away).
>>
>> This series makes sure all filesystems that need superblock writeouts
>> define a ->sync_fs so we can stop calling ->write_super for the data
>> integrity writeback.  This means the presence of ->write_super indicates
>> a filesystem does want periodic superblock writeback and can implement
>> these independently from the data integrity writeback. It also means
>> we don't need to bother with possible s_dirt races for the data
>> integrity syncs.
>>
>> This patch series first adds ->sync_fs methods to all filesystems having
>> ->write_super, often refactoring the code in that are (and also
>> preparing the ->write_super instances for moving MS_RDONLY checking into
>> the caller), then it makes sure the existing ->sync_fs instaces cover
>> the work previously done in ->write_super for the two cases where they
>> differed widely, and lastly removes the call to ->write_super from
>> __sync_filesystem.
> 
> Applied.  FWIW, file_fsync() is nearly gone; the last remnants are exofs,
> hfs and hfsplus.  Is there any sane way to keep track of dirty metadata
> blocks there?  Or is "just flush all dirty buffer_head for the device"
> really the best we can do?

Hi Al

Below is just an RFC for review concerning the file_fsync() use. I've
open coded the parts of file_fsync() that are actually needed. Note
the TODO as the sb update is only needed for new files, I'll farther
look into it later.

The below patch is no good because it's based on an old tree. The lock_super
was moved to inside write_super right? and also I should call the new
exofs_sync_fs() directly. Please just give me the OK that this is the
idea here and I'll prepare the right patch.

What is the most uptodate tree that has all these changes? so I can cut
the patches over? (Sorry for the novice-ness)
---
[RFC] 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.

Boaz
---
git diff --stat -p fs/exofs/file.c
 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: Schedule 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)