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

[Jan Kara <jack@suse.cz>]

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