Bug 1099237 - rhel7 ext4 defaults to 64 bit, which extlinux can't reliably read
Summary: rhel7 ext4 defaults to 64 bit, which extlinux can't reliably read
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: e2fsprogs
Version: 7.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Eric Sandeen
QA Contact: Filesystem QE
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-05-19 21:14 UTC by Colin Walters
Modified: 2017-03-25 02:16 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-10-22 21:27:51 UTC


Attachments (Terms of Use)
debug hacks (3.70 KB, patch)
2014-05-19 21:15 UTC, Colin Walters
no flags Details | Diff
changes to spec (1.75 KB, patch)
2014-05-19 21:15 UTC, Colin Walters
no flags Details | Diff
patch for 64 bit (2.86 KB, patch)
2014-10-01 18:51 UTC, Colin Walters
no flags Details | Diff

Description Colin Walters 2014-05-19 21:14:23 UTC
Sometimes we're seeing issues where syslinux says "ERROR: No configuration found" after an upgrade or rollback.

The physical FS appears fine; this is some interaction issue between extlinux and the FS.

I'm going to attach debugging patches for this.

What I'm doing is applying the patches to the RHEL syslinux package, then my test flow is:

$ rhpkg local
$ sudo yum localinstall x86_64/*.rpm
$ rm /var/tmp/.guestfs* -rf
$ echo 'extlinux /boot' | guestfish -a /var/lib/libvirt/images/rh-atomic-host-test-rollback.qcow2 -m /dev/sda3:/ -m /dev/sda1:/boot

That'll install an updated bootloader in the guest.

Comment 1 Colin Walters 2014-05-19 21:15:07 UTC
Created attachment 897323 [details]
debug hacks

Comment 2 Colin Walters 2014-05-19 21:15:30 UTC
Created attachment 897324 [details]
changes to spec

Comment 3 Colin Walters 2014-05-19 23:31:13 UTC
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.

Comment 6 Colin Walters 2014-05-20 12:52:39 UTC
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?

Comment 7 Colin Walters 2014-05-20 13:41:15 UTC
A question for the ext4 people: if I do turn off the 64bit flag for /boot, is that a supported configuration?

Comment 8 Colin Walters 2014-05-20 13:56:33 UTC
It looks like grub2 supports the 64 bit feature, so that's an argument for syslinux doing the same.

Comment 11 Lukáš Czerner 2014-05-20 14:27:07 UTC
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

Comment 12 Lukáš Czerner 2014-05-20 14:33:45 UTC
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

Comment 13 Colin Walters 2014-05-20 14:48:51 UTC
(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.

Comment 19 Colin Walters 2014-05-20 20:10:36 UTC
For reference, required downstream workaround: https://github.com/cgwalters/rpm-ostree/commit/1df4f37249f0746a0aab0ef786de92744140684b

Comment 20 Eric Sandeen 2014-05-21 05:16:53 UTC
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 .

Comment 21 Lukáš Czerner 2014-05-21 13:36:01 UTC
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

Comment 22 Eric Sandeen 2014-05-21 15:45:21 UTC
Ah, that's right - I'm sorry, I forgot since I didn't take that bug.  :)

Comment 23 H. Peter Anvin 2014-08-11 22:49:06 UTC
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.

Comment 24 Colin Walters 2014-10-01 15:59:19 UTC
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)

Comment 25 Peter Jones 2014-10-01 16:31:28 UTC
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
}

Comment 26 Eric Sandeen 2014-10-01 16:37:55 UTC
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

Comment 27 Lukáš Czerner 2014-10-01 16:50:15 UTC
(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

Comment 28 David Cantrell 2014-10-01 17:38:27 UTC
(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.

Comment 29 Colin Walters 2014-10-01 18:51:48 UTC
Created attachment 943183 [details]
patch for 64 bit

Comment 30 Eric Sandeen 2014-10-01 19:02:43 UTC
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?

Comment 31 Colin Walters 2014-10-02 03:22:15 UTC
(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)

Comment 32 Eric Sandeen 2014-10-02 05:36:10 UTC
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

Comment 33 Lukáš Czerner 2014-10-02 10:22:47 UTC
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

Comment 34 Lukáš Czerner 2014-10-02 12:48:04 UTC
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

Comment 39 H. Peter Anvin 2014-10-03 18:49:39 UTC
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.

Comment 40 H. Peter Anvin 2014-10-03 18:49:53 UTC
Make that:

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

Comment 41 Eric Sandeen 2014-10-03 18:52:56 UTC
Awesome, thanks!

Comment 43 Colin Walters 2014-10-22 20:47:02 UTC
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

Comment 44 Eric Sandeen 2014-10-22 21:27:51 UTC
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.

Comment 45 Lukáš Czerner 2014-10-23 10:06:58 UTC
(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

Comment 46 Dusty Mabe 2016-05-02 18:03:24 UTC
(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?


Note You need to log in before you can comment on or make changes to this bug.