Re: [Ext2-devel] [RFC][10/21]ext3 enlarge blocksize

[Andreas Dilger <adilger@clusterfs.com>]

On Apr 13, 2006  16:07 +0900, sho@tnes.nec.co.jp wrote:
>           - Add an incompat flag "EXT3_FEATURE_INCOMPAT_LARGE_BLOCK"
>             which indicates that the filesystem is extended.
>           - Allow block size till pagesize in ext3.
> 
> -	if (blocksize < EXT3_MIN_BLOCK_SIZE ||
> -	    blocksize > EXT3_MAX_BLOCK_SIZE) {
> -		printk(KERN_ERR 
> -		       "EXT3-fs: Unsupported filesystem blocksize %d on %s.\n",
> -		       blocksize, sb->s_id);
> -		goto failed_mount;
> +	if (EXT3_HAS_INCOMPAT_FEATURE(sb,
> +	    EXT3_FEATURE_INCOMPAT_LARGE_BLOCK)) {
> +		if (blocksize < EXT3_MIN_BLOCK_SIZE ||
> +		    blocksize > PAGE_SIZE ||
> +		    blocksize > EXT3_EXTENDED_MAX_BLOCK_SIZE) {

This in itself doesn't need an incompatible flag, since old kernels will
prevent the mount for filesystems with blocksize > EXT3_MAX_BLOCK_SIZE
anyways.  It is already incompatible with older kernels, and isn't really
related to the other use of this flag - changing the i_blocks field to
handle files > 2TB.

> +			printk(KERN_ERR "EXT3-fs: Unsupported extended filesystem blocksize %d on %s.\n",
> +				blocksize, sb->s_id);

May be better to have:

+			printk(KERN_ERR "EXT3-fs: cannot mount filesystem with "
+			       "blocksize %u larger than PAGE_SIZE %u on %s\n",
+			       blocksize, PAGE_SIZE, sb->s_id);

On Apr 13, 2006  16:08 +0900, sho@tnes.nec.co.jp wrote:
>   [11/21] enlarge file size(ext3)
>           - If the flag is set to super block, i_blocks of disk inode
>             (ext3_inode) is filesystem-block unit, and i_blocks of VFS
>             inode is sector unit.
> 
>           - If the flag is set to super block, max file size is set to
>             (FS blocksize) * (2^32 -1).

I like this patch, but prefer if we maintain as much compatibility as
possible.  There is not really a reason to make a filesystem incompatible
unless there are actually files > 2TB stored in it (just like we didn't
make filesystems incompatible for large_file unless there were files over
2GB in the filesystem).

> @@ -2627,7 +2628,13 @@ void ext3_read_inode(struct inode * inod
> +	if (EXT3_HAS_INCOMPAT_FEATURE(sb,
> +	    EXT3_FEATURE_INCOMPAT_LARGE_BLOCK)) {
> +		inode->i_blocks = (blkcnt_t)le32_to_cpu(raw_inode->i_blocks)
> +			<< (inode->i_blkbits - EXT3_SECTOR_BITS);
> +	} else {
> +		inode->i_blocks = le32_to_cpu(raw_inode->i_blocks);
> +	} 

In particular, if INCOMPAT_LARGE_BLOCK is set on the filesystem, we could
check a flag (a new EXT3_LARGE_BLOCK_FL = 0x40000, or whatever Ted would
want there) in the inode to determine if the i_blocks is in fs blocksize
units, or the default sector units.

> @@ -2760,7 +2768,13 @@ static int ext3_do_update_inode(handle_t
> +	if (EXT3_HAS_INCOMPAT_FEATURE(sb,
> +	    EXT3_FEATURE_INCOMPAT_LARGE_BLOCK)) {
> +		raw_inode->i_blocks = cpu_to_le32((inode->i_blocks)
> +			>> (inode->i_blkbits - EXT3_SECTOR_BITS));
> +	} else {
> +		raw_inode->i_blocks = cpu_to_le32(inode->i_blocks);
> +	}

In this case, we would only set EXT3_LARGE_BLOCK_FL if the i_blocks count
was over 2^32 sectors, and set the EXT3_FEATURE_INCOMPAT_LARGE_BLOCK flag
in the superblock.  It would probably be best if this was implemented in
a small helper function like ext3_set_feature(sb, compat, rocompat, incompat)
that takes the code to check/set EXT3_FEATURE_RO_COMPAT_LARGE_FILE and
update dynamic_rev.

That way you get maximum compatibility, and only break compatibility for
the very few cases where such large files are acutally needed.  It would
also allow ext3 to handle > 2TB sparse files without being incompatible,
since we don't need to know a-priori whether the file will have a lot of
blocks allocated to it.

I guess the only other question is whether this should be an INCOMPAT flag,
or an ROCOMPAT one, since there is very little harm in getting the i_blocks
count wrong on a read-only filesystem, and having it ROCOMPAT at least
allows some form of system recovery if this flag is set by error.

The flag for large files was also ROCOMPAT, and one could argue that
the file size is 1000x more important than the number of blocks, since
the size affects actually reading the file, while the blocks count is
used by almost nothing.

> +static loff_t ext3_max_size(int bits, struct super_block *sb)
>  {
> +	loff_t upper_limit;
> +	if(EXT3_HAS_INCOMPAT_FEATURE(sb, 
> +	   EXT3_FEATURE_INCOMPAT_LARGE_BLOCK)) {
> +		upper_limit = (1LL << (bits + 32)) - 1;

With the above changes, the upper limit could always be the higher value.

> @@ -1703,6 +1709,15 @@ static int ext3_fill_super (struct super
>  	 */
>  
>  	root = iget(sb, EXT3_ROOT_INO);
> +	/* To appoint -O large block option, LSF needs to be enabled */
> +	if (EXT3_HAS_INCOMPAT_FEATURE(sb,
> +	    EXT3_FEATURE_INCOMPAT_LARGE_BLOCK)) {
> +		if (sizeof(root->i_blocks) < sizeof(u64)) {
> +			printk(KERN_ERR "EXT3-fs: %s: Unsupported large block option"\
> +				"with LSF disabled.\n", sb->s_id);
> +			goto failed_mount;
> +		}

This could be considered a failure only for write mounts, and not readonly.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

-
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/