Bug 1099237

Created attachment 897323 [details]
debug hacks

Created attachment 897324 [details]
changes to spec

On the physical FS we have:

/syslinux/syslinux.cfg -> ../loader/syslinux.cfg
/loader -> loader.1
/loader.1/syslinux.cfg

what I see is this:

iget(syslinux, 2) => 16 (mode: 4, flags: 524288, mtime: 1399141282)
iget(syslinux.cfg, 16) => 17 (mode: 10, flags: 0, mtime: 1399141282)
iget(loader, 2) => 26 (mode: 10, flags: 0, mtime: 1400528392)
iget(loader.1, 2) => 2049 (mode: 5, flags: 409600, mtime: 0)

Note the corrupted mode on loader.1 - there is no mode 5, and the mtime is obviously wrong too.

The inode number of 2049 is correct though, looking at the filesystem in debugfs.

So something is going wrong with extlinux locating this inode.

Ok, I think I see what's going wrong here.

extlinux:

struct ext2_group_desc {
...
    uint32_t bg_inode_table;	/* Inodes table block */
...
}

ext4:

struct ext4_group_desc {
    ...
    uint32_t bg_inode_table_lo;	/* Inodes table block */
    ...
    uint32_t bg_inode_table_hi;	/* Inodes table block MSB */
}

And in fact, the MSB word is set for loader.1, the LSB word is 0.

Possibly we should turn off the 64 bit flag for /boot?

A question for the ext4 people: if I do turn off the 64bit flag for /boot, is that a supported configuration?

It looks like grub2 supports the 64 bit feature, so that's an argument for syslinux doing the same.

If bg_inode_table_hi really is set then that means that the block it is pointing to is past 16TB is that really the case ?

Having a file system without 64bit is supported, but it also means that this particular file system can not grow past 16TB.

However the problem really is that extlinux is obviously doing something wrong when defining it's own version of the extN structures. That's going to break very often. They should be using a libraries provided by e2fsprogs (ext2fs and e2p).

-Lukas

Also, what Eric said is very much true. extlinux *must* act accordingly when it gets the file system which features it does not understand (either bail our, or not change it).

-Lukas

(In reply to Lukáš Czerner from comment #11)
> If bg_inode_table_hi really is set then that means that the block it is
> pointing to is past 16TB is that really the case ?

That's a really good point; maybe my diagnosis is wrong here, and what's actually wrong is we're reading the wrong block group.

> Having a file system without 64bit is supported, but it also means that this
> particular file system can not grow past 16TB.

Right, not concerned about a 16TB /boot =)

> However the problem really is that extlinux is obviously doing something
> wrong when defining it's own version of the extN structures. That's going to
> break very often. They should be using a libraries provided by e2fsprogs
> (ext2fs and e2p).

extlinux is a bootloader, it can't make use of (at least unmodified) userspace libraries.  Same situation with GRUB.

For reference, required downstream workaround: https://github.com/cgwalters/rpm-ostree/commit/1df4f37249f0746a0aab0ef786de92744140684b

Lukas, is rhel7 default behavior w.r.t. 64 bit really different from upstream ?  I don't remember any patches to change that.... Will look tomorrow I guess .

Yes it is. This is what we have in RHEL7:

Patch6: e2fsprogs-1.42.9-enable-64bit-feature-by-default.patch

it fixed this bz: https://bugzilla.redhat.com/show_bug.cgi?id=982871

and this is upstream proposal:

http://www.spinics.net/lists/linux-ext4/msg42294.html

-Lukas

Ah, that's right - I'm sorry, I forgot since I didn't take that bug.  :)

This bug was just pointed to me...

It isn't just the data structure definitions, Syslinux needs to be updated to understand the new data structures themselves.  As Colin Walters points out, we can't really use libext2fs.

It is probably not a huge amount of work, other than the fact that ino_t inside Syslinux needs to be expanded to 64 bits.

Anaconda is going to need to create ext4 /boot as ^64bit.  I think we can just update autopart for that by default.  If we wanted a more targeted fix, only do it if extlinux is in use.  (But I can't think of a reason anyone needs a 64 bit filesystem for /boot)

I think you're right that the best thing to do now is just not create /boot with "64bit" set.  The simplest way to do that is to change mke2fs.conf from something like:

small = {
      blocksize = 1024
      inode_size = 128
      inode_ratio = 4096
}

to:

small = {
      blocksize = 1024
      inode_size = 128
      inode_ratio = 4096
      features = ^64bit
}

Is that mke2fs.conf only used in the installer root?

I don't remember if "small" gets chosen automatically used based on size; probably?  So then if someone happens to provision a larger /boot you'll be in trouble again.

I *think* you can just mkfs.ext4 -O ^64bit /dev/blah and explicitly do it when you are formatting the boot filesystem, regardless of size.  That seems safest to me.

Lukas, did that 64bit default patch ever make it upstream?

-Eric

(In reply to Eric Sandeen from comment #26)
> 
> Lukas, did that 64bit default patch ever make it upstream?
> 
> -Eric

Unfortunately not yet. I am not sure where is the problem, I did not even got any comments rejecting the idea.

-Lukas

(In reply to Eric Sandeen from comment #26)
> Is that mke2fs.conf only used in the installer root?

The installation environment uses the default mke2fs.conf provided by the e2fsprogs package.

> I don't remember if "small" gets chosen automatically used based on size;
> probably?  So then if someone happens to provision a larger /boot you'll be
> in trouble again.

Automatic partition layouts create a small /boot.  While it is technically possible for a user to create a very large /boot, the far more common case right now is to not.

> I *think* you can just mkfs.ext4 -O ^64bit /dev/blah and explicitly do it
> when you are formatting the boot filesystem, regardless of size.  That seems
> safest to me.

We do not carry special options like that in blivet.  mke2fs is just invoked specifying the filesystem type.  All other options are set by the mke2fs.conf file provided by e2fsprogs.  The primary reason for this is determining what options used at fs creation time during an install.  In the past we used to force a handful of options, which changed periodically based on the wants and desires of Fedora or RHEL or whatever.  It's better to centralize the option setting in mke2fs.conf.

Created attachment 943183 [details]
patch for 64 bit

So, mke2fs does indeed switch on fs size; the cutoff for "small" is 512mb.

        meg = (1024 * 1024) / EXT2_BLOCK_SIZE(sb);
        if (fs_blocks_count < 3 * meg)
                size_type = "floppy";
        else if (fs_blocks_count < 512 * meg)
                size_type = "small";

What's the default?  A box here reports a 497M /boot; that's pretty close.  Going over 512M for an admin who likes to keep a few kernels around doesn't sound terribly unlikely.

I'm sympathetic to blivet not wanting to care special sauce; I know that we ran into problems there before.

But in this case you have very specific knowledge of what you need: we are installing, we are creating boot, and we want extlinux to be able to read it.

But you'd like upstream defaults to know all that, and just make it work w/o any options.  That's tricky.

Disabling 64bit on small filesystems is probably fine as far as it goes; growing from 512m to > 16T would be pretty insane (actually in this case, it'd be > 4T; 2^32 1k blocks).  I'm just not sure it'll be robust enough, it's a heuristic with pretty significant holes, it seems to me.

The bulletproof way to fix this is to fix extlinux.  Is there a bug open for that?

(In reply to Eric Sandeen from comment #30)
> 
> The bulletproof way to fix this is to fix extlinux.  Is there a bug open for
> that?

Not sure, the upstream author is here anyways and is aware.  However, what makes this problematic is extlinux only breaks on RHEL7 (and it's only used there for Atomic)

I really think the only reliable way to fix this is in extlinux.  Unfortunately I don't think we yet have a real root cause on why the 64bit feature breaks it; I guess it'll take some investigation to get there.

HPA, is there an easy way to test extlinux outside of an actual booting environment, once we get to the fs image which breaks it?

Thanks,
-eric

I agree we need to fix extlinux. Changing mke2fs.conf is not an option. We have 64bit set by default exactly so that people can resize the file system past 64bit and this wold break for some use cases and they will be plenty :

 - it will break if the /boot partition is bigger then 512MB
 - it will break if someone tries to resize <512MB file system past 64bit (4TB for 1k bs, 16TB for 4k bs).

Let's see what we can do to get extlinux working.

-Lukas

Please provide me with an easy to use reproducer of this issue so I can fix extlinux since noone seems to be eager to get the thing done. Please keep in mind that I do not regularly work with atomic nor docker.

Thanks!
-Lukas

I have a very simple test framework for Syslinux at:

http://www.zytor.com/~hpa/syslinux/exttest-20120627.tar.xz

For this particular use it should be sufficient.

64-bit support shouldn't be all that hard to add in, but it requires some more conditionals in the parsing of ext[234] data structures.

Make that:

http://www.zytor.com/~hpa/syslinux/exttest-20140521.tar.xz

Awesome, thanks!

Sorry I forgot to update this bug; Atomic no longer needs this as we're switching to GRUB2 across all targets.

https://www.redhat.com/archives/anaconda-devel-list/2014-October/msg00008.html

Ok, I'm going to close this one then; I'll still try to take a look to be nice to extlinux, but if it's not a RHEL7 issue, I'm taking it off that radar.

(In reply to Eric Sandeen from comment #44)
> Ok, I'm going to close this one then; I'll still try to take a look to be
> nice to extlinux, but if it's not a RHEL7 issue, I'm taking it off that
> radar.

In that case we should probably file a bug with extlinux since we do ship it.

-Lukas

(In reply to Lukáš Czerner from comment #45)

> 
> In that case we should probably file a bug with extlinux since we do ship it.
> 
> -Lukas

Did anyone ever open that bug? I don't see an issue tracker for syslinux. Does one exist?