Re: [PATCH v2 1/7] affs: stop setting bm_flags

[Geert Uytterhoeven]

On Wed, Jun 6, 2012 at 5:56 PM, Artem Bityutskiy <dedekind1@xxxxxxxxx> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
>
> 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.

The book "Amiga Intern" (German version on the 'net, which is faster to access
than the Dutch version somewhere in my attic ;-) agrees with this ("Dieses Flag
enthÃlt -1 (TRUE), wenn die Bit-Map der Diskette gÃltig ist").

I checked some file systems I had lying around, and they all have 0xffffffff in
bm_flag (byte offset 0x138 in the block at the middle of the file system).
At bit to my surprise, as I expected two of them (very old backups of file
systems) to have been written to from Linux at least once, but of course I can
be mistaken, and never have trusted Linux's affs with them ;-).
Mounting (copies of) them on Ubuntu and writing to them changes this flag to 1.

After digging in the affs sources, it seems we had this behavior since at least
2.0.29 (which is 1997 or so), so I think it's safe to assume it's been like that
forever.

> Jan Kara commented on this: "I have some vague recollection that on Amiga
> boolean was usually encoded as: 0 == false, ~0 == -1 == true. But it has been
> ages..."

include/exec/types.h:

#define TRUE            1
#define FALSE           0

However, include/dos/dos.h:

#define DOSTRUE (-1L)
#define DOSFALSE (0L)

So AmigaOS will probably always use 0xffffffff.

> Thus, my conclusion is that value of '1' is as good as value of '2' and we can
> just always use '2'. An Jan Kara suggested to go further: "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."

As no one ever complained, 1 or 2 or whatever non-zero value probably doesn't
matter at all to AmigaOS.

> So this patch removes the bm_flags setting completely. This makes the "clean"
> argument of the 'affs_commit_super()' function unneeded, so it is also removed.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
> ---
> Âfs/affs/super.c | Â Â9 ++++-----
> Â1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/affs/super.c b/fs/affs/super.c
> index 0782653..1d42e46 100644
> --- a/fs/affs/super.c
> +++ b/fs/affs/super.c
> @@ -25,13 +25,12 @@ 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);
> Â Â Â Âsecs_to_datestamp(get_seconds(), &tail->disk_change);
> Â Â Â Âaffs_fix_checksum(sb, bh);
> Â Â Â Âmark_buffer_dirty(bh);
> @@ -46,7 +45,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 +59,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 +70,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

Gr{oetje,eeting}s,

            Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
             Â Â -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/