[PATCH 0/7] affs: stop using write_supers and s_dirt

[PATCH 0/7] affs: stop using write_supers and s_dirt

[Artem Bityutskiy]

This patch-set makes AFFS file-system stop using the VFS '->write_supers()'
call-back and the '->s_dirt' superblock field because I plan to remove them
once all users are gone.

Like the other similar patch-sets, we switch to a delayed job for writing out
the superblock instead of using the 's_dirt' flag. Additionally, this patch-set
includes many clean-ups.

Reminder:

The goal is to get rid of the 'sync_supers()' kernel thread. This kernel thread
wakes up every 5 seconds (by default) and calls '->write_super()' for all
mounted file-systems. And the bad thing is that this is done even if all the
superblocks are clean. Moreover, many file-systems do not even need this end
they do not register the '->write_super()' method at all (e.g., btrfs).

So 'sync_supers()' most often just generates useless wake-ups and wastes power.
I am trying to make all file-systems independent of '->write_super()' and plan
to remove 'sync_supers()' and '->write_super()' completely once there are no
more users.

Al, in the past I was trying to upstream patches which optimized 'sync_super()',
but you wanted me to kill it completely instead, which I am trying to do
now, see http://lkml.org/lkml/2010/7/22/96

Tested using the fsstress test from the LTP project. AFFS has some issues
without my patches - I see it reporting about corrupted inodes. These patch-set
does not introduce new regressions. I did not try to resolve the AFFS problems
which are already there.

======
Overall status:

1. ext4: patches submitted, waiting for review from Ted Ts'o:
   https://lkml.org/lkml/2012/4/2/111
2. udf: patch submitted, should be in Jan Kara's tree:
   https://lkml.org/lkml/2012/6/4/233
3  exofs: patch submitted,
   https://lkml.org/lkml/2012/6/4/211
4. ext2:     done, see commit f72cf5e223a28d3b3ea7dc9e40464fd534e359e8
5. vfat:     done, see commit 78491189ddb6d84d4a4abae992ed891a236d0263
6. jffs2:    done, see commit 208b14e507c00ff7f108e1a388dd3d8cc805a443
7. reiserfs: done, see commit 033369d1af1264abc23bea2e174aa47cdd212f6f

TODO: hfs, hfsplus, sysv, ufs
======

 fs/affs/affs.h   |    8 ++++++
 fs/affs/bitmap.c |    4 +-
 fs/affs/super.c  |   69 +++++++++++++++++++++++++++++++++++------------------
 3 files changed, 55 insertions(+), 26 deletions(-)

Thanks,
Artem.

[PATCH 1/7] affs: remove strange argument of affs_commit_super

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

The 'affs_commit_super()' function takes a 'clean' argument which it stores in
the 'bm_flags' field of the AFFS root block. This patch removes it to make the
code simpler and cleaner and enable few other clean-ups.

Currently AFFS stores values '1' and '2' in 'bm_flags', and I fail to see any
logic when it prefers one or another. AFFS writes '1' only from
'->put_super()', while '->sync_fs()' and '->write_super()' store value '2'.
So on the first glance, it looks like we want to have '1' if we unmount.
However, this does not really happen in these cases:
  1. superblock is written via 'write_super()' then we unmount;
  2. we re-mount R/O, then unmount.
which are quite typical.

I could not find good documentation describing this field, except of one random
piece of documentation in the internet which says that -1 means that the root
block is valid, which is not consistent with what we have in the Linux AFFS
driver.

Thus, my conclusion is that value of '1' is as good as value of '2' and we can
just always use '2'. This is exactly what this patch does.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/affs/super.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index 0782653..698282a 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -25,13 +25,13 @@ 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 wait, int clean)
+affs_commit_super(struct super_block *sb, int wait)
 {
 	struct affs_sb_info *sbi = AFFS_SB(sb);
 	struct buffer_head *bh = sbi->s_root_bh;
 	struct affs_root_tail *tail = AFFS_ROOT_TAIL(sb, bh);
 
-	tail->bm_flag = cpu_to_be32(clean);
+	tail->bm_flag = cpu_to_be32(2);
 	secs_to_datestamp(get_seconds(), &tail->disk_change);
 	affs_fix_checksum(sb, bh);
 	mark_buffer_dirty(bh);
@@ -46,7 +46,7 @@ affs_put_super(struct super_block *sb)
 	pr_debug("AFFS: put_super()\n");
 
 	if (!(sb->s_flags & MS_RDONLY) && sb->s_dirt)
-		affs_commit_super(sb, 1, 1);
+		affs_commit_super(sb, 1);
 
 	kfree(sbi->s_prefix);
 	affs_free_bitmap(sb);
@@ -60,7 +60,7 @@ affs_write_super(struct super_block *sb)
 {
 	lock_super(sb);
 	if (!(sb->s_flags & MS_RDONLY))
-		affs_commit_super(sb, 1, 2);
+		affs_commit_super(sb, 1);
 	sb->s_dirt = 0;
 	unlock_super(sb);
 
@@ -71,7 +71,7 @@ static int
 affs_sync_fs(struct super_block *sb, int wait)
 {
 	lock_super(sb);
-	affs_commit_super(sb, wait, 2);
+	affs_commit_super(sb, wait);
 	sb->s_dirt = 0;
 	unlock_super(sb);
 	return 0;
-- 
1.7.7.6

[PATCH 2/7] affs: remove useless superblock writeout on unmount

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

We do not need to write out the superblock from '->put_super()' because VFS has
already called '->sync_fs()' by this time and the superblock has already been
written out. Thus, remove the 'affs_commit_super()' infocation from
'affs_put_super()'.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/affs/super.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index 698282a..db184c0 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -45,9 +45,6 @@ affs_put_super(struct super_block *sb)
 	struct affs_sb_info *sbi = AFFS_SB(sb);
 	pr_debug("AFFS: put_super()\n");
 
-	if (!(sb->s_flags & MS_RDONLY) && sb->s_dirt)
-		affs_commit_super(sb, 1);
-
 	kfree(sbi->s_prefix);
 	affs_free_bitmap(sb);
 	affs_brelse(sbi->s_root_bh);
-- 
1.7.7.6

[PATCH 3/7] affs: remove useless superblock writeout on remount

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

We do not need to write out the superblock from '->remount_fs()' because
VFS has already called '->sync_fs()' by this time and the superblock has
already been written out. Thus, remove the 'affs_write_super()'
infocation from 'affs_remount()'.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/affs/super.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index db184c0..3fc50b1 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -546,10 +546,9 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 	if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY))
 		return 0;
 
-	if (*flags & MS_RDONLY) {
-		affs_write_super(sb);
+	if (*flags & MS_RDONLY)
 		affs_free_bitmap(sb);
-	} else
+	else
 		res = affs_init_bitmap(sb, flags);
 
 	return res;
-- 
1.7.7.6

[PATCH 4/7] affs: re-structure superblock locking a bit

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

AFFS wants to serialize the superblock (the root block in AFFS terms) updates
and uses 'lock_super()/unlock_super()' for these purposes. This patch pushes the
locking down to the 'affs_commit_super()' from the callers.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/affs/super.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/affs/super.c b/fs/affs/super.c
index 3fc50b1..8293cb9 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -31,12 +31,14 @@ affs_commit_super(struct super_block *sb, int wait)
 	struct buffer_head *bh = sbi->s_root_bh;
 	struct affs_root_tail *tail = AFFS_ROOT_TAIL(sb, bh);
 
+	lock_super(sb);
 	tail->bm_flag = cpu_to_be32(2);
 	secs_to_datestamp(get_seconds(), &tail->disk_change);
 	affs_fix_checksum(sb, bh);
 	mark_buffer_dirty(bh);
 	if (wait)
 		sync_dirty_buffer(bh);
+	unlock_super(sb);
 }
 
 static void
@@ -55,22 +57,17 @@ affs_put_super(struct super_block *sb)
 static void
 affs_write_super(struct super_block *sb)
 {
-	lock_super(sb);
 	if (!(sb->s_flags & MS_RDONLY))
 		affs_commit_super(sb, 1);
 	sb->s_dirt = 0;
-	unlock_super(sb);
-
 	pr_debug("AFFS: write_super() at %lu, clean=2\n", get_seconds());
 }
 
 static int
 affs_sync_fs(struct super_block *sb, int wait)
 {
-	lock_super(sb);
 	affs_commit_super(sb, wait);
 	sb->s_dirt = 0;
-	unlock_super(sb);
 	return 0;
 }
 
-- 
1.7.7.6

[PATCH 5/7] affs: stop using lock_super

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

The VFS's 'lock_super()' and 'unlock_super()' calls are deprecated and unwanted
and just wait for a brave knight who'd kill them. This patch makes AFFS stop
using them and introduces own AFFS superblock mutex which we use for
serializeing the root block changes.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/affs/affs.h  |    1 +
 fs/affs/super.c |    5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/affs/affs.h b/fs/affs/affs.h
index 45a0ce4..b8af745 100644
--- a/fs/affs/affs.h
+++ b/fs/affs/affs.h
@@ -108,6 +108,7 @@ struct affs_sb_info {
 	char *s_prefix;			/* Prefix for volumes and assigns. */
 	char s_volume[32];		/* Volume prefix for absolute symlinks. */
 	spinlock_t symlink_lock;	/* protects the previous two */
+	struct mutex s_lock;		/* protects the SB's buffer-head */
 };
 
 #define SF_INTL		0x0001		/* International filesystem. */
diff --git a/fs/affs/super.c b/fs/affs/super.c
index 8293cb9..8674915 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -31,14 +31,14 @@ affs_commit_super(struct super_block *sb, int wait)
 	struct buffer_head *bh = sbi->s_root_bh;
 	struct affs_root_tail *tail = AFFS_ROOT_TAIL(sb, bh);
 
-	lock_super(sb);
+	mutex_lock(&sbi->s_lock);
 	tail->bm_flag = cpu_to_be32(2);
 	secs_to_datestamp(get_seconds(), &tail->disk_change);
 	affs_fix_checksum(sb, bh);
 	mark_buffer_dirty(bh);
 	if (wait)
 		sync_dirty_buffer(bh);
-	unlock_super(sb);
+	mutex_unlock(&sbi->s_lock);
 }
 
 static void
@@ -299,6 +299,7 @@ static int affs_fill_super(struct super_block *sb, void *data, int silent)
 		return -ENOMEM;
 
 	sb->s_fs_info = sbi;
+	mutex_init(&sbi->s_lock);
 	mutex_init(&sbi->s_bmlock);
 	spin_lock_init(&sbi->symlink_lock);
 
-- 
1.7.7.6

[PATCH 6/7] affs: introduce VFS superblock object back-reference

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Add an 'sb' VFS superblock back-reference to the 'struct affs_sb_info' data
structure - we will need to find the VFS superblock from a 'struct
affs_sb_info' object in the next patch, so this change is jut a preparation.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/affs/affs.h  |    1 +
 fs/affs/super.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/affs/affs.h b/fs/affs/affs.h
index b8af745..178d3a3 100644
--- a/fs/affs/affs.h
+++ b/fs/affs/affs.h
@@ -109,6 +109,7 @@ struct affs_sb_info {
 	char s_volume[32];		/* Volume prefix for absolute symlinks. */
 	spinlock_t symlink_lock;	/* protects the previous two */
 	struct mutex s_lock;		/* protects the SB's buffer-head */
+	struct super_block *sb;		/* the VFS superblock object */
 };
 
 #define SF_INTL		0x0001		/* International filesystem. */
diff --git a/fs/affs/super.c b/fs/affs/super.c
index 8674915..9acab34 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -299,6 +299,7 @@ static int affs_fill_super(struct super_block *sb, void *data, int silent)
 		return -ENOMEM;
 
 	sb->s_fs_info = sbi;
+	sbi->sb = sb;
 	mutex_init(&sbi->s_lock);
 	mutex_init(&sbi->s_bmlock);
 	spin_lock_init(&sbi->symlink_lock);
-- 
1.7.7.6

[PATCH 7/7] affs: get rid of affs_sync_super

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This patch makes affs stop using the VFS '->write_super()' method along with
the 's_dirt' superblock flag, because they are on their way out.

The whole "superblock write-out" VFS infrastructure is served by the
'sync_supers()' kernel thread, which wakes up every 5 (by default) seconds and
writes out all dirty superblocks using the '->write_super()' call-back.  But the
problem with this thread is that it wastes power by waking up the system every
5 seconds, even if there are no diry superblocks, or there are no client
file-systems which would need this (e.g., btrfs does not use
'->write_super()'). So we want to kill it completely and thus, we need to make
file-systems to stop using the '->write_super()' VFS service, and then remove
it together with the kernel thread.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/affs/affs.h   |    6 ++++++
 fs/affs/bitmap.c |    4 ++--
 fs/affs/super.c  |   48 +++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/fs/affs/affs.h b/fs/affs/affs.h
index 178d3a3..563ba3f 100644
--- a/fs/affs/affs.h
+++ b/fs/affs/affs.h
@@ -3,6 +3,7 @@
 #include <linux/buffer_head.h>
 #include <linux/amigaffs.h>
 #include <linux/mutex.h>
+#include <linux/workqueue.h>
 
 /* AmigaOS allows file names with up to 30 characters length.
  * Names longer than that will be silently truncated. If you
@@ -110,6 +111,9 @@ struct affs_sb_info {
 	spinlock_t symlink_lock;	/* protects the previous two */
 	struct mutex s_lock;		/* protects the SB's buffer-head */
 	struct super_block *sb;		/* the VFS superblock object */
+	int work_queued;		/* non-zero delayed work is queued */
+	struct delayed_work sb_work;	/* superblock flush delayed work */
+	spinlock_t work_lock;		/* protects sb_work and work_queued */
 };
 
 #define SF_INTL		0x0001		/* International filesystem. */
@@ -130,6 +134,8 @@ static inline struct affs_sb_info *AFFS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
+void affs_mark_sb_dirty(struct super_block *sb);
+
 /* amigaffs.c */
 
 extern int	affs_insert_hash(struct inode *inode, struct buffer_head *bh);
diff --git a/fs/affs/bitmap.c b/fs/affs/bitmap.c
index 3e26271..6e0be43 100644
--- a/fs/affs/bitmap.c
+++ b/fs/affs/bitmap.c
@@ -103,7 +103,7 @@ affs_free_block(struct super_block *sb, u32 block)
 	*(__be32 *)bh->b_data = cpu_to_be32(tmp - mask);
 
 	mark_buffer_dirty(bh);
-	sb->s_dirt = 1;
+	affs_mark_sb_dirty(sb);
 	bm->bm_free++;
 
 	mutex_unlock(&sbi->s_bmlock);
@@ -248,7 +248,7 @@ find_bit:
 	*(__be32 *)bh->b_data = cpu_to_be32(tmp + mask);
 
 	mark_buffer_dirty(bh);
-	sb->s_dirt = 1;
+	affs_mark_sb_dirty(sb);
 
 	mutex_unlock(&sbi->s_bmlock);
 
diff --git a/fs/affs/super.c b/fs/affs/super.c
index 9acab34..c79de88 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -17,6 +17,7 @@
 #include <linux/magic.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/writeback.h>
 #include "affs.h"
 
 extern struct timezone sys_tz;
@@ -47,6 +48,7 @@ affs_put_super(struct super_block *sb)
 	struct affs_sb_info *sbi = AFFS_SB(sb);
 	pr_debug("AFFS: put_super()\n");
 
+	cancel_delayed_work_sync(&sbi->sb_work);
 	kfree(sbi->s_prefix);
 	affs_free_bitmap(sb);
 	affs_brelse(sbi->s_root_bh);
@@ -54,23 +56,45 @@ affs_put_super(struct super_block *sb)
 	sb->s_fs_info = NULL;
 }
 
-static void
-affs_write_super(struct super_block *sb)
-{
-	if (!(sb->s_flags & MS_RDONLY))
-		affs_commit_super(sb, 1);
-	sb->s_dirt = 0;
-	pr_debug("AFFS: write_super() at %lu, clean=2\n", get_seconds());
-}
-
 static int
 affs_sync_fs(struct super_block *sb, int wait)
 {
 	affs_commit_super(sb, wait);
-	sb->s_dirt = 0;
 	return 0;
 }
 
+static void flush_superblock(struct work_struct *work)
+{
+	struct affs_sb_info *sbi;
+	struct super_block *sb;
+
+	sbi = container_of(work, struct affs_sb_info, sb_work.work);
+	sb = sbi->sb;
+
+	spin_lock(&sbi->work_lock);
+	sbi->work_queued = 0;
+	spin_unlock(&sbi->work_lock);
+
+	affs_commit_super(sb, 1);
+}
+
+void affs_mark_sb_dirty(struct super_block *sb)
+{
+	struct affs_sb_info *sbi = AFFS_SB(sb);
+	unsigned long delay;
+
+	if (sb->s_flags & MS_RDONLY)
+	       return;
+
+	spin_lock(&sbi->work_lock);
+	if (!sbi->work_queued) {
+	       delay = msecs_to_jiffies(dirty_writeback_interval * 10);
+	       queue_delayed_work(system_long_wq, &sbi->sb_work, delay);
+	       sbi->work_queued = 1;
+	}
+	spin_unlock(&sbi->work_lock);
+}
+
 static struct kmem_cache * affs_inode_cachep;
 
 static struct inode *affs_alloc_inode(struct super_block *sb)
@@ -132,7 +156,6 @@ static const struct super_operations affs_sops = {
 	.write_inode	= affs_write_inode,
 	.evict_inode	= affs_evict_inode,
 	.put_super	= affs_put_super,
-	.write_super	= affs_write_super,
 	.sync_fs	= affs_sync_fs,
 	.statfs		= affs_statfs,
 	.remount_fs	= affs_remount,
@@ -303,6 +326,8 @@ static int affs_fill_super(struct super_block *sb, void *data, int silent)
 	mutex_init(&sbi->s_lock);
 	mutex_init(&sbi->s_bmlock);
 	spin_lock_init(&sbi->symlink_lock);
+	spin_lock_init(&sbi->work_lock);
+	INIT_DELAYED_WORK(&sbi->sb_work, flush_superblock);
 
 	if (!parse_options(data,&uid,&gid,&i,&reserved,&root_block,
 				&blocksize,&sbi->s_prefix,
@@ -527,6 +552,7 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 		return -EINVAL;
 	}
 
+	flush_delayed_work_sync(&sbi->sb_work);
 	replace_mount_options(sb, new_opts);
 
 	sbi->s_flags = mount_flags;
-- 
1.7.7.6

Re: [PATCH 5/7] affs: stop using lock_super

[Jan Kara]

On Tue 05-06-12 14:28:13, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> The VFS's 'lock_super()' and 'unlock_super()' calls are deprecated and unwanted
> and just wait for a brave knight who'd kill them. This patch makes AFFS stop
> using them and introduces own AFFS superblock mutex which we use for
> serializeing the root block changes.
  Hum, why not just use the buffer lock for this?

								Honza
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  fs/affs/affs.h  |    1 +
>  fs/affs/super.c |    5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/affs/affs.h b/fs/affs/affs.h
> index 45a0ce4..b8af745 100644
> --- a/fs/affs/affs.h
> +++ b/fs/affs/affs.h
> @@ -108,6 +108,7 @@ struct affs_sb_info {
>  	char *s_prefix;			/* Prefix for volumes and assigns. */
>  	char s_volume[32];		/* Volume prefix for absolute symlinks. */
>  	spinlock_t symlink_lock;	/* protects the previous two */
> +	struct mutex s_lock;		/* protects the SB's buffer-head */
>  };
>  
>  #define SF_INTL		0x0001		/* International filesystem. */
> diff --git a/fs/affs/super.c b/fs/affs/super.c
> index 8293cb9..8674915 100644
> --- a/fs/affs/super.c
> +++ b/fs/affs/super.c
> @@ -31,14 +31,14 @@ affs_commit_super(struct super_block *sb, int wait)
>  	struct buffer_head *bh = sbi->s_root_bh;
>  	struct affs_root_tail *tail = AFFS_ROOT_TAIL(sb, bh);
>  
> -	lock_super(sb);
> +	mutex_lock(&sbi->s_lock);
>  	tail->bm_flag = cpu_to_be32(2);
>  	secs_to_datestamp(get_seconds(), &tail->disk_change);
>  	affs_fix_checksum(sb, bh);
>  	mark_buffer_dirty(bh);
>  	if (wait)
>  		sync_dirty_buffer(bh);
> -	unlock_super(sb);
> +	mutex_unlock(&sbi->s_lock);
>  }
>  
>  static void
> @@ -299,6 +299,7 @@ static int affs_fill_super(struct super_block *sb, void *data, int silent)
>  		return -ENOMEM;
>  
>  	sb->s_fs_info = sbi;
> +	mutex_init(&sbi->s_lock);
>  	mutex_init(&sbi->s_bmlock);
>  	spin_lock_init(&sbi->symlink_lock);
>  
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/7] affs: remove strange argument of affs_commit_super

[Jan Kara]

On Tue 05-06-12 14:28:09, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> The 'affs_commit_super()' function takes a 'clean' argument which it stores in
> the 'bm_flags' field of the AFFS root block. This patch removes it to make the
> code simpler and cleaner and enable few other clean-ups.
> 
> Currently AFFS stores values '1' and '2' in 'bm_flags', and I fail to see any
> logic when it prefers one or another. AFFS writes '1' only from
> '->put_super()', while '->sync_fs()' and '->write_super()' store value '2'.
> So on the first glance, it looks like we want to have '1' if we unmount.
> However, this does not really happen in these cases:
>   1. superblock is written via 'write_super()' then we unmount;
>   2. we re-mount R/O, then unmount.
> which are quite typical.
> 
> I could not find good documentation describing this field, except of one random
> piece of documentation in the internet which says that -1 means that the root
> block is valid, which is not consistent with what we have in the Linux AFFS
> driver.
  I have some vague recollection that on Amiga boolean was usually encoded
as: 0 == false, ~0 == -1 == true. But it has been ages...

> Thus, my conclusion is that value of '1' is as good as value of '2' and we can
> just always use '2'. This is exactly what this patch does.
  Yeah, I believe so as well. But generally bm_flags handling looks
strange. If they are 0, we mount fs read only and thus cannot change them.
If they are != 0, we write 2 there. So IMHO if you just removed bm_flags
setting, nothing will really happen.

								Honza

> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  fs/affs/super.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/affs/super.c b/fs/affs/super.c
> index 0782653..698282a 100644
> --- a/fs/affs/super.c
> +++ b/fs/affs/super.c
> @@ -25,13 +25,13 @@ 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 wait, int clean)
> +affs_commit_super(struct super_block *sb, int wait)
>  {
>  	struct affs_sb_info *sbi = AFFS_SB(sb);
>  	struct buffer_head *bh = sbi->s_root_bh;
>  	struct affs_root_tail *tail = AFFS_ROOT_TAIL(sb, bh);
>  
> -	tail->bm_flag = cpu_to_be32(clean);
> +	tail->bm_flag = cpu_to_be32(2);
>  	secs_to_datestamp(get_seconds(), &tail->disk_change);
>  	affs_fix_checksum(sb, bh);
>  	mark_buffer_dirty(bh);
> @@ -46,7 +46,7 @@ affs_put_super(struct super_block *sb)
>  	pr_debug("AFFS: put_super()\n");
>  
>  	if (!(sb->s_flags & MS_RDONLY) && sb->s_dirt)
> -		affs_commit_super(sb, 1, 1);
> +		affs_commit_super(sb, 1);
>  
>  	kfree(sbi->s_prefix);
>  	affs_free_bitmap(sb);
> @@ -60,7 +60,7 @@ affs_write_super(struct super_block *sb)
>  {
>  	lock_super(sb);
>  	if (!(sb->s_flags & MS_RDONLY))
> -		affs_commit_super(sb, 1, 2);
> +		affs_commit_super(sb, 1);
>  	sb->s_dirt = 0;
>  	unlock_super(sb);
>  
> @@ -71,7 +71,7 @@ static int
>  affs_sync_fs(struct super_block *sb, int wait)
>  {
>  	lock_super(sb);
> -	affs_commit_super(sb, wait, 2);
> +	affs_commit_super(sb, wait);
>  	sb->s_dirt = 0;
>  	unlock_super(sb);
>  	return 0;
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 5/7] affs: stop using lock_super

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On Wed, 2012-06-06 at 12:07 +0200, Jan Kara wrote:
> > The VFS's 'lock_super()' and 'unlock_super()' calls are deprecated
> and unwanted
> > and just wait for a brave knight who'd kill them. This patch makes
> AFFS stop
> > using them and introduces own AFFS superblock mutex which we use for
> > serializeing the root block changes.
>   Hum, why not just use the buffer lock for this? 

Indeed. Addressed in v2, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]