Re: [RFC PATCH 1/1] minix: unify the v1 and v2/v3 itree code paths

[Christoph Hellwig]

I think this is a good idea, and should make the code more maintainable
forward, as well as simply your iomap conversion.

A few comments:

> +	/* Set the direct block and indirect block depth now that the rest of
> +	 * the version-specific settings have been set.
> +	 */

Please use the normal kernel comment style:

	/*
	 * Set the direct block and indirect block depth now that the rest of
	 * the version-specific settings have been set.
	 */

> +	sbi->s_direct = MINIX_DIRECT; /* Always the same. */

No need for the comment I think.

>  	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
>  	if (INODE_VERSION(inode) == MINIX_V1)
> -		stat->blocks = (BLOCK_SIZE / 512) * V1_minix_blocks(stat->size, sb);
> +		stat->blocks = (BLOCK_SIZE / 512) * minix_blocks(stat->size, sb);
>  	else
> -		stat->blocks = (sb->s_blocksize / 512) * V2_minix_blocks(stat->size, sb);
> +		stat->blocks = (sb->s_blocksize / 512) * minix_blocks(stat->size, sb);

v1 always sets s_blocksize to BLOCK_SIZE, so this can simply become and
unconditional:

	stat->blocks = (sb->s_blocksize / 512) * minix_blocks(stat->size, sb);

> +++ b/fs/minix/itree.c
> @@ -0,0 +1,744 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + *  linux/fs/minix/inode.c

Please do not add (or move) file names in top of file comments,
they are a bit pointless.  Also we usually don't have an empty line
between the SPDX tag and the top of file comment.

> +#include <linux/slab.h>
> +#include "minix.h"
> +
> +#define DIRCOUNT 7
> +#define INDIRCOUNT(sb) (1 << ((sb)->s_blocksize_bits - 2))
> +#define MINIX_V1_BLK 512
> +#define MINIX_V1_BLK_SHIFT 9
> +
> +/* The different versions of the Minix filesystem also have different block
> + * sizes. In order to unify the itree functions and not have the split that's
> + * been in place for decades, we're going to have two separate types for v1 and
> + * v2/v3 block sizes. This does require having explicit version checks and
> + * casting block pointers to v1_block_t and a few cases where there's a special
> + * v1 version of a function that gets called where it's necessary to do it that
> + * way.

Comment usually should describe the current code and not the history.
There's a few exception where the history really matters like for bad
on-disk or on-wire formats set in stone.

> + * files, the last master commit before they were merged and altered was
> + * 87320be9f0d24fce67631b7eef919f0b79c3e45c.

Also not needed, git log/blame will tell us easily.

> +static inline int v2_block_to_path(struct inode *inode, long block, int *offsets)

Overly long line.  (a few more below)

> +/* same business with chain as before */
> +static inline int splice_branch(struct inode *inode,
> +				     Indirect *chain,
> +				     Indirect *where,
> +				     int num)

static inline int splice_branch(struct inode *inode, Indirect *chain,
		Indirect *where, int num)

Similar for a few other functions.

> +extern int minix_get_block(struct inode *inode, sector_t block,
> +		struct buffer_head *bh, int create);
> +extern unsigned int minix_blocks(loff_t size, struct super_block *sb);

Please drop the extern for all function declarations that you touch.