Re: [Ext2-devel] [PATCH 1/2] ext2/3: Support 2^32-1 blocks(Kernel)

["Takashi Sato" <sho@bsd.tnes.nec.co.jp>]

Hi,

> You changed most of the affected variables from "int" to "unsigned int",
> that seems allow block number to address 2^32. It probably a good thing
> to consider change the variables to sector_t type, so when the time we
> want to support for 64 bit block number, we don't have to re-do the
> similar work again.  Laurent did very similar work on this before.

sector_t is 8bytes on normal configuration and there are many
variables for blocks on ext2/3.  I thought extending variables may
influence on performance, so I didn't change.

> Besides these limitations, I think there is one more to limit ext3
> filesystem size to 8TB
> 
> - The superblock format currently stores the number of block groups as a
> 16-bit integer, and because (on a 4 KB blocksize filesystem) the maximum
> number of blocks in a block group is 32,768 , a combination of these
> constraints limits the maximum size of the filesystem to 8 TB

Is it s_block_group_nr in ext3_super_block?
mke2fs sets 65535 to the field if the number of block groups is greater
than 65535.  Current kernel ignores the field and re-calculate from
other fields.  findsuper command is the only user of it and it simply prints
the value.  So, it does not limit the maximum size of the filesystem to 8 TB.
I  confirmed that mke2fs with my change could make the filesysytem
which has more than 65536 groups and it could be mounted.

> I noticed that the first patch set combines changes to ext2 filesystem
> and changes to ext3 filesystem. It would be nice to split the changes to
> two different filesystems.

Ok, I'll split it later.

> But that doesn't fix all th problem. We still have places in ext3 block
> reservation code that use int for system-wide block numbers. For e.g.,
> alloc_new_reservation(), group_first_block, group_end_block, start_block
> are all filesystem wide block numbers, they need to be changed. I will
> check the code more closely tomorrow to see if the changes will break
> any assumptions.

Thank you, I missed it.  I'm looking forward to seeing your report.

> Also, I noticed that in your first patch, you changed a few variables
> for logical block number from "long" to "unsigned int". Just want to
> point out that's a seperate issue- that's for enlarge the file size, not
> for expand the max filesystem size.

Ok, I'll remove them when I update the patch next time.
They are left because I'm considering enlarging the file size max too...

>> -static int ext3_alloc_block (handle_t *handle,
>> - struct inode * inode, unsigned long goal, int *err)
>> +static unsigned int ext3_alloc_block (handle_t *handle,
>> + struct inode * inode, unsigned int goal, int *err)
>>  {
> 
> I did some changes in the same code to support ext3 multiple block
> allocation. Those patches removed this function ext3_alloc_block(). The
> patches are sitting in mm tree now.
> 
> BTW, why we change from unsigned long back to unsigned int here?

Because ext3_alloc_branch calls ext3_alloc_block with int type for the
block number and ext3_alloc_blocks returns int type.

>>  struct ext3_block_alloc_info *block_i =  EXT3_I(inode)->i_block_alloc_info;
>> @@ -505,21 +505,21 @@ static unsigned long ext3_find_goal(stru
>>  static int ext3_alloc_branch(handle_t *handle, struct inode *inode,
>>       int num,
>>       unsigned long goal,
>> -      int *offsets,
>> +      unsigned int *offsets,
>>       Indirect *branch)
> 
> offsets[] array here store the index position within a indirect block,
> where the physical block is stored. The indirect block takes a 4k block,
> holds up to 1K entry of physical block numbers, so int type for the
> index is good enough.

Ok, I'll update them too.