Re: [PATCH] fs/minix: bugfix, number of indirect block ptrs per block depends on block size

[Jan Kara <jack@suse.cz>]

On Wed 26-05-10 12:03:23, Erik van der Kouwe wrote:
> From: Erik van der Kouwe <vdkouwe@cs.vu.nl>
> 
> The MINIX filesystem driver used a constant number of indirect block
> pointers in an indirect block. This worked only for filesystems with
> 1kb block, while the MINIX default block size is now 4kb. As a
> consequence, large files were read incorrectly on such filesystems
> and writing a large file would cause the filesystem to become
> corrupted. This patch computes the number of indirect block pointers
> based on the block size, making the driver work for each block size.
  Out of curiosity: Are you really using MINIX? The code is dead for a
long time...

> I would like to thank Feiran Zheng ('Fam') for pointing out the
> cause of the corruption.
> 
> Signed-off-by: Erik van der Kouwe <vdkouwe@cs.vu.nl>
> ---
> 
> --- fs/minix/itree_v2.c.orig	2010-05-26 14:10:15.000000000 +0200
> +++ fs/minix/itree_v2.c	2010-05-26 13:44:53.000000000 +0200
> @@ -20,6 +20,9 @@ static inline block_t *i_data(struct ino
> 	return (block_t *)minix_i(inode)->u.i2_data;
> }
> 
> +#define DIRCOUNT 7
> +#define INDIRCOUNT(sb) ((sb)->s_blocksize / 4)
> +
> static int block_to_path(struct inode * inode, long block, int offsets[DEPTH])
> {
> 	int n = 0;
> @@ -34,21 +37,21 @@ static int block_to_path(struct inode *
> 			printk("MINIX-fs: block_to_path: "
> 			       "block %ld too big on dev %s\n",
> 				block, bdevname(sb->s_bdev, b));
> -	} else if (block < 7) {
> +	} else if (block < DIRCOUNT) {
> 		offsets[n++] = block;
> -	} else if ((block -= 7) < 256) {
> -		offsets[n++] = 7;
> +	} else if ((block -= DIRCOUNT) < INDIRCOUNT(sb)) {
  This modification of 'block' in the if condition is a blatant violation
of the kernel coding style. If you feel like it, you could fix it when changing
the conditions anyway. Something like:

  if (block < DIRCOUNT) {
	offsets[n++] = block;
	return n;
  }
  block -= DIRCOUNT;
  if (block < INDIRCOUNT(sb)) {
  ...

> +		offsets[n++] = DIRCOUNT;
> 		offsets[n++] = block;
> -	} else if ((block -= 256) < 256*256) {
> -		offsets[n++] = 8;
> -		offsets[n++] = block>>8;
> -		offsets[n++] = block & 255;
> +	} else if ((block -= INDIRCOUNT(sb)) < INDIRCOUNT(sb) * INDIRCOUNT(sb)) {
> +		offsets[n++] = DIRCOUNT + 1;
> +		offsets[n++] = block / INDIRCOUNT(sb);
> +		offsets[n++] = block % INDIRCOUNT(sb);
  This division and modulo aren't really necessary. You can define
INDIRCOUNT_BITS(sb) as (sb->s_blocksize_bits - 2) and use it instead
of the division and use & (INDIRCOUNT(sb)-1) instead of the modulo. Not
that it would seriously matter but it's an improvement.

> 	} else {
> -		block -= 256*256;
> -		offsets[n++] = 9;
> -		offsets[n++] = block>>16;
> -		offsets[n++] = (block>>8) & 255;
> -		offsets[n++] = block & 255;
> +		block -= INDIRCOUNT(sb) * INDIRCOUNT(sb);
> +		offsets[n++] = DIRCOUNT + 2;
> +		offsets[n++] = (block / INDIRCOUNT(sb)) / INDIRCOUNT(sb);
> +		offsets[n++] = (block / INDIRCOUNT(sb)) % INDIRCOUNT(sb);
> +		offsets[n++] = block % INDIRCOUNT(sb);
> 	}
> 	return n;
> }

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR