Re: [RFC][PATCH] 64 bit blocks support for e2fsprogs

[Andreas Dilger <adilger@sun.com>]

On Dec 07, 2007  11:14 -0500, Josef Bacik wrote:
> At this point I've only added the helper functions and converted mke2fs
> (sloppily for now) to use the helper functions.  I know we want to make this
> as painless as possible to go between ext2/3/4 so what I plan on doing is
> leave everything as it is and only allow large disks to be formatted if
> EXT4_FEATURE_INCOMPAT_64BIT is set.  Any feedback would be awesome, and if
> somebody is already working on this please let me know so I'm not duplicating
> work.  Thanks much,

> +_INLINE_ void ext2fs_blocks_count_set(struct ext2_super_block *super,
> +				      blk64_t blk)
> +{
> +	super->s_blocks_count = ext2fs_cpu_to_le32((__u32)blk);
> +	super->s_blocks_count_hi = ext2fs_cpu_to_le32(blk >> 32);
> +}

I'm undecided of whether there should be checking of whether INCOMPAT_64BIT
set in the superblock before using the s_blocks*_hi fields.  In one sense
these fields are only valid when the INCOMPAT_64BIT flag is set, but
there is also a concern that the 64BIT flag might be incorrectly unset...

Given that these are the ext2fs_* interfaces, and e2fsck should probably
be doing its own validation of the 64BIT flag, along with the size of the
physical device and the s_blocks*_hi fields I think the ext2fs_* functions
SHOULD check for INCOMPAT_64BIT being set before using the _hi fields.

> @@ -191,9 +192,9 @@ static void test_disk(ext2_filsys fs, badblocks_list *bb_list)
> -	sprintf(buf, "badblocks -b %d -X %s%s%s %u", fs->blocksize,
> +	sprintf(buf, "badblocks -b %d -X %s%s%s %lu", fs->blocksize,
>  		quiet ? "" : "-s ", (cflag > 1) ? "-w " : "",
> -		fs->device_name, fs->super->s_blocks_count-1);
> +		fs->device_name, ext2fs_blocks64_count(fs)-1);

"%lu" is not correct for a 64-bit field, which will be long long on 32-bit
systems.  Instead I'd suggest using %llu and casting the return of
ext2fs_blocks64_count() to (unsigned long long).

> -	retval = zero_blocks(fs, 0, fs->super->s_blocks_count,
> +	retval = zero_blocks(fs, 0, ext2fs_blocks64_count(fs),
>  			     &progress, &blk, &count);

Since zero_blocks() only takes a blk_t as a parameter I think this is
incorrect.  However, given it is a static internal function we can also
change the prototype to fix it.

> @@ -686,9 +690,10 @@ static void show_stats(ext2_filsys fs)
> +	if (ext2fs_blocks_count(fs_param.super) != ext2fs_blocks_count(s))
>  		fprintf(stderr, _("warning: %u blocks unused.\n\n"),
> -		       fs_param.s_blocks_count - s->s_blocks_count);
> +		       ext2fs_blocks_count(fs_param.super) - 
> +		       ext2fs_blocks_count(s));

Need to cast the difference to (unsigned).  The difference will never need
to be a 64-bit value, but the returned type will be blk64_t.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.