[PATCH 0/2] AFFS clean-ups

[PATCH 0/2] AFFS clean-ups

[Artem Bityutskiy]

Al,

you requested me to clean up 'sb->s_dirt' users, but did not respond
to my last "sync_supers" optimizations patch series:
http://marc.info/?l=linux-fsdevel&m=127583599911620&w=2

Probably the patch series was too large, so I decided to submit you
few patches at a time. Here are AFFS clean-up patches. They lessen
the amount of places where it refers 'sb->s_dirt', and fix few issues.

The patches were tested - I mounted AFFS, did some simple operations,
added prints to write super and saw that it is still synchronized.

Please, take a look and add to your tree if they are fine. I'll then
send you some more patches.

Artem.

[PATCH 1/2] AFFS: clean up dirty flag usage

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

In 'affs_write_super()': remove ancient and wrong commented code,
remove unneeded 'clean' variable, so the function becomes a bit
cleaner and simpler.

In 'affs_remount(): remove unnecessary SB dirty flag changes.

Tested-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/affs/super.c |   19 +++++--------------
 1 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index 16a3e47..8447451 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -61,20 +61,13 @@ affs_put_super(struct super_block *sb)
 static void
 affs_write_super(struct super_block *sb)
 {
-	int clean = 2;
-
 	lock_super(sb);
-	if (!(sb->s_flags & MS_RDONLY)) {
-		//	if (sbi->s_bitmap[i].bm_bh) {
-		//		if (buffer_dirty(sbi->s_bitmap[i].bm_bh)) {
-		//			clean = 0;
-		affs_commit_super(sb, clean);
-		sb->s_dirt = !clean;	/* redo until bitmap synced */
-	} else
-		sb->s_dirt = 0;
+	if (!(sb->s_flags & MS_RDONLY))
+		affs_commit_super(sb, 2);
+	sb->s_dirt = 0;
 	unlock_super(sb);
 
-	pr_debug("AFFS: write_super() at %lu, clean=%d\n", get_seconds(), clean);
+	pr_debug("AFFS: write_super() at %lu, clean=2\n", get_seconds());
 }
 
 static int
@@ -554,9 +547,7 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 		return 0;
 	}
 	if (*flags & MS_RDONLY) {
-		sb->s_dirt = 1;
-		while (sb->s_dirt)
-			affs_write_super(sb);
+		affs_write_super(sb);
 		affs_free_bitmap(sb);
 	} else
 		res = affs_init_bitmap(sb, flags);
-- 
1.7.0.1

[PATCH 2/2] AFFS: wait for sb synchronization when needed

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

AFFS does not ever wait for superblock synchronization in
->put_super(), ->write_super, and ->sync_fs().

However, it should wait for synchronization in ->put_super() because
it is about to be unmounted, in ->write_super() because this is
periodic SB synchronization performed from a separate kernel thread,
and in ->sync_fs() it should respect the 'wait' flag. This patch fixes
the situation.

Also, in ->put_super(), do not write the SB if it is not dirty.

Tested-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/affs/super.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index 8447451..9f6fbee 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -26,7 +26,7 @@ static int affs_statfs(struct dentry *dentry, struct kstatfs *buf);
 static int affs_remount (struct super_block *sb, int *flags, char *data);
 
 static void
-affs_commit_super(struct super_block *sb, int clean)
+affs_commit_super(struct super_block *sb, int wait, int clean)
 {
 	struct affs_sb_info *sbi = AFFS_SB(sb);
 	struct buffer_head *bh = sbi->s_root_bh;
@@ -36,6 +36,8 @@ affs_commit_super(struct super_block *sb, int clean)
 	secs_to_datestamp(get_seconds(), &tail->disk_change);
 	affs_fix_checksum(sb, bh);
 	mark_buffer_dirty(bh);
+	if (wait)
+		sync_dirty_buffer(bh);
 }
 
 static void
@@ -46,8 +48,8 @@ affs_put_super(struct super_block *sb)
 
 	lock_kernel();
 
-	if (!(sb->s_flags & MS_RDONLY))
-		affs_commit_super(sb, 1);
+	if (!(sb->s_flags & MS_RDONLY) && sb->s_dirt)
+		affs_commit_super(sb, 1, 1);
 
 	kfree(sbi->s_prefix);
 	affs_free_bitmap(sb);
@@ -63,7 +65,7 @@ affs_write_super(struct super_block *sb)
 {
 	lock_super(sb);
 	if (!(sb->s_flags & MS_RDONLY))
-		affs_commit_super(sb, 2);
+		affs_commit_super(sb, 1, 2);
 	sb->s_dirt = 0;
 	unlock_super(sb);
 
@@ -74,7 +76,7 @@ static int
 affs_sync_fs(struct super_block *sb, int wait)
 {
 	lock_super(sb);
-	affs_commit_super(sb, 2);
+	affs_commit_super(sb, wait, 2);
 	sb->s_dirt = 0;
 	unlock_super(sb);
 	return 0;
-- 
1.7.0.1

Re: [PATCH 2/2] AFFS: wait for sb synchronization when needed

[Christoph Hellwig]

On Fri, Jun 11, 2010 at 06:05:20PM +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> AFFS does not ever wait for superblock synchronization in
> ->put_super(), ->write_super, and ->sync_fs().
> 
> However, it should wait for synchronization in ->put_super() because
> it is about to be unmounted, in ->write_super() because this is
> periodic SB synchronization performed from a separate kernel thread,
> and in ->sync_fs() it should respect the 'wait' flag. This patch fixes
> the situation.
> 
> Also, in ->put_super(), do not write the SB if it is not dirty.

->sync_fs should also wait for the superblock I/O to be completed.

Re: [PATCH 2/2] AFFS: wait for sb synchronization when needed

[Artem Bityutskiy]

On Fri, 2010-06-11 at 12:31 -0400, Christoph Hellwig wrote:
> On Fri, Jun 11, 2010 at 06:05:20PM +0300, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > 
> > AFFS does not ever wait for superblock synchronization in
> > ->put_super(), ->write_super, and ->sync_fs().
> > 
> > However, it should wait for synchronization in ->put_super() because
> > it is about to be unmounted, in ->write_super() because this is
> > periodic SB synchronization performed from a separate kernel thread,
> > and in ->sync_fs() it should respect the 'wait' flag. This patch fixes
> > the situation.
> > 
> > Also, in ->put_super(), do not write the SB if it is not dirty.
> 
> ->sync_fs should also wait for the superblock I/O to be completed.

Err, depending on the 'wait' flag? I did:

@@ -74,7 +76,7 @@ static int
 affs_sync_fs(struct super_block *sb, int wait)
 {
        lock_super(sb);
-       affs_commit_super(sb, 2);
+       affs_commit_super(sb, wait, 2);
        sb->s_dirt = 0;
        unlock_super(sb);
        return 0;

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)