mke2fs: avoid inode number error with large FS

From: Alexey Lyashkov <alexey.lyashkov@gmail.com>

Sometimes during system deployment customers are faced with system
formating problem for given inodes/bytes rate. User need to recalucate
this rate and start formating again.

This patch adds code that limit inodes count instead of error return,
to use all inodes in the filesystem.

Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9501
Cray-bug-id: LUS-5250
Signed-off-by: Artem Blagodarenko <artem.blagodarenko@gmail.com>
Signed-off-by: Alexey Lyashkov <alexey.lyashkov@gmail.com>
---
 misc/mke2fs.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

On Mon, Feb 12, 2018 at 02:14:19PM +0300, Artem Blagodarenko wrote:
> From: Alexey Lyashkov <alexey.lyashkov@gmail.com>
> 
> Sometimes during system deployment customers are faced with system
> formating problem for given inodes/bytes rate. User need to recalucate
> this rate and start formating again.
> 
> This patch adds code that limit inodes count instead of error return,
> to use all inodes in the filesystem.

Hi,

in this case then you do not have byte-per-inode ratio you've
specified. So why to specify it in the first place ?

Maybe I am missing something but I would think that if you specify -i
then you know what you want and if it's not possible then I would not
expect the mke2fs to just succeed regardless. I guess it's confusing.

Also the man page says:

"This value generally shouldn't be smaller than the blocksize of the
filesystem, since in that case more inodes would be made than can ever
be used."

But in your case you're using "-i 1024" on what I assume is a 4k bs file
system ?

-Lukas

> 
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9501
> Cray-bug-id: LUS-5250
> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@gmail.com>
> Signed-off-by: Alexey Lyashkov <alexey.lyashkov@gmail.com>
> ---
>  misc/mke2fs.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index cfb10bc4..6fb0a717 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -2457,14 +2457,11 @@ profile_error:
>  		unsigned long long n;
>  		n = ext2fs_blocks_count(&fs_param) * blocksize / inode_ratio;
>  		if (n > MAX_32_NUM) {
> -			if (ext2fs_has_feature_64bit(&fs_param))
> -				num_inodes = MAX_32_NUM;
> -			else {
> +			num_inodes = MAX_32_NUM;
> +			if (!ext2fs_has_feature_64bit(&fs_param))
>  				com_err(program_name, 0,
> -					_("too many inodes (%llu), raise "
> -					  "inode ratio?"), n);
> -				exit(1);
> -			}
> +					_("too many inodes (%llu), reduced to "
> +					  "%llu"), n, MAX_32_NUM);
>  		}
>  	} else if (num_inodes > MAX_32_NUM) {
>  		com_err(program_name, 0,
> -- 
> 2.14.3 (Apple Git-98)
>

On 2/12/18 9:45 AM, Lukas Czerner wrote:
> On Mon, Feb 12, 2018 at 02:14:19PM +0300, Artem Blagodarenko wrote:
>> From: Alexey Lyashkov <alexey.lyashkov@gmail.com>
>>
>> Sometimes during system deployment customers are faced with system
>> formating problem for given inodes/bytes rate. User need to recalucate
>> this rate and start formating again.
>>
>> This patch adds code that limit inodes count instead of error return,
>> to use all inodes in the filesystem.
> 
> Hi,
> 
> in this case then you do not have byte-per-inode ratio you've
> specified. So why to specify it in the first place ?
> 
> Maybe I am missing something but I would think that if you specify -i
> then you know what you want and if it's not possible then I would not
> expect the mke2fs to just succeed regardless. I guess it's confusing.

I agree that fixing up incorrect/impossible format specifications and
continuing is not preferable; it really makes the behavior matrix complex
when some incorrect options are fixed on the fly, while others fail.

And worse, this creates a new "default" behavior which comes into play
only when specific incorrect mkfs options are explicitly provided.

When an admin stops using mkfs defaults and starts manually specifying
geometry, the onus is on /them/ to specify options which are valid.

> Also the man page says:
> 
> "This value generally shouldn't be smaller than the blocksize of the
> filesystem, since in that case more inodes would be made than can ever
> be used."
> 
> But in your case you're using "-i 1024" on what I assume is a 4k bs file
> system ?

Right, can you offer a concrete example of the commandline you're trying
to fix?

If it's "-i 1024" on a 4k filesystem, that's simply broken and /should/
be rejected.  If the error message is not clear, perhaps that's the best
place to focus these efforts.

Thanks,
-Eric

On 2/12/18 10:06 AM, Eric Sandeen wrote:
> 
> 
> On 2/12/18 9:45 AM, Lukas Czerner wrote:
>> On Mon, Feb 12, 2018 at 02:14:19PM +0300, Artem Blagodarenko wrote:
>>> From: Alexey Lyashkov <alexey.lyashkov@gmail.com>
>>>
>>> Sometimes during system deployment customers are faced with system
>>> formating problem for given inodes/bytes rate. User need to recalucate
>>> this rate and start formating again.
>>>
>>> This patch adds code that limit inodes count instead of error return,
>>> to use all inodes in the filesystem.
>>
>> Hi,
>>
>> in this case then you do not have byte-per-inode ratio you've
>> specified. So why to specify it in the first place ?
>>
>> Maybe I am missing something but I would think that if you specify -i
>> then you know what you want and if it's not possible then I would not
>> expect the mke2fs to just succeed regardless. I guess it's confusing.
> 
> I agree that fixing up incorrect/impossible format specifications and
> continuing is not preferable; it really makes the behavior matrix complex
> when some incorrect options are fixed on the fly, while others fail.
> 
> And worse, this creates a new "default" behavior which comes into play
> only when specific incorrect mkfs options are explicitly provided.
> 
> When an admin stops using mkfs defaults and starts manually specifying
> geometry, the onus is on /them/ to specify options which are valid.

Ok, I have read the patch more carefully now ;)

So prior to this patch, on a filesystem /with/ the 64bit feature, we
already silently limit inode count to MAX_32_NUM.  So if an inode ratio
is specified which would be > MAX_32_NUM we silently cap it, but only
if 64bit is turned on.

With your patch, you change the behavior so that the MAX_32_NUM
cap/adjustment is always in place both with and without the 64bit
feature, but it only warns the user when the 64bit feature is not present?

So I guess there is precedent for the behavior.  I'd have preferred that
mkfs simply fail if an invalid inode ratio was specified, but I guess
that is already not the case.  So perhaps fixing it on the fly for
both 64bit and !64bit is reasonable, though I'm not sure why we should
warn with !64bit and be silent with 64bit...

I guess my preference would be to always fail the mkfs if an invalid
inode ratio was specified on the commandline, regardless of 64bit
feature presence.  Silent fix-ups of incorrect specifications tend
to violate the principle of least surprise.

-Eric

On Mon, Feb 12, 2018 at 10:06:57AM -0600, Eric Sandeen wrote:
> 
> 
> On 2/12/18 9:45 AM, Lukas Czerner wrote:
> > On Mon, Feb 12, 2018 at 02:14:19PM +0300, Artem Blagodarenko wrote:
> >> From: Alexey Lyashkov <alexey.lyashkov@gmail.com>
> >>
> >> Sometimes during system deployment customers are faced with system
> >> formating problem for given inodes/bytes rate. User need to recalucate
> >> this rate and start formating again.
> >>
> >> This patch adds code that limit inodes count instead of error return,
> >> to use all inodes in the filesystem.
> > 
> > Hi,
> > 
> > in this case then you do not have byte-per-inode ratio you've
> > specified. So why to specify it in the first place ?
> > 
> > Maybe I am missing something but I would think that if you specify -i
> > then you know what you want and if it's not possible then I would not
> > expect the mke2fs to just succeed regardless. I guess it's confusing.
> 
> I agree that fixing up incorrect/impossible format specifications and
> continuing is not preferable; it really makes the behavior matrix complex
> when some incorrect options are fixed on the fly, while others fail.
> 
> And worse, this creates a new "default" behavior which comes into play
> only when specific incorrect mkfs options are explicitly provided.
> 
> When an admin stops using mkfs defaults and starts manually specifying
> geometry, the onus is on /them/ to specify options which are valid.
> 
> > Also the man page says:
> > 
> > "This value generally shouldn't be smaller than the blocksize of the
> > filesystem, since in that case more inodes would be made than can ever
> > be used."
> > 
> > But in your case you're using "-i 1024" on what I assume is a 4k bs file
> > system ?
> 
> Right, can you offer a concrete example of the commandline you're trying
> to fix?
> 
> If it's "-i 1024" on a 4k filesystem, that's simply broken and /should/
> be rejected.  If the error message is not clear, perhaps that's the best
> place to focus these efforts.

I think that inline data actually changes that ? However if that's the
case then we need to change the documentation. But it still does not
mean we want to "autocorrect" spcified values.

-Lukas

> 
> Thanks,
> -Eric