Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: changed the default value of nrext64 to 0 to avoid unstable feature of nrext64 #295

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

fix: changed the default value of nrext64 to 0 to avoid unstable feature of nrext64 #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a85369
update: modified kernel 5.10 consume config files
Sparksssj Dec 6, 2024
6c55d99
update: modified kernel 5.15 consume config files
Sparksssj Dec 6, 2024
e00a34e
update: modified kernel 6.1 consume config files
Sparksssj Dec 6, 2024
abbc6cf
patch: patching xfsprogs to source default config files
Sparksssj Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/kernel-5.10/kernel-5.10.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ Source100: config-bottlerocket
Source220: neuron-sysinit.target.drop-in.conf
Source221: [email protected]

# XFS configuration
Source300: mkfs.xfs.conf

# Help out-of-tree module builds run `make prepare` automatically.
Patch1001: 1001-Makefile-add-prepare-target-for-external-modules.patch
# Enable INITRAMFS_FORCE config option for our use case.
Expand Down Expand Up @@ -83,6 +86,12 @@ Summary: Modules for the Linux kernel
%description modules
%{summary}.

%package mkfs-confs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions to improve the dependencies:

  1. Require the parent package so that the installed configs match the installed kernel.
  2. Provide a generic capability (using parens, by convention) that other packages can key off of
  3. Conflict with any other package providing the same capability

Goal is to make it so that only one mkfs-confs package can be installed.

Suggested change
%package mkfs-confs
%package mkfs-confs
Requires: %{name}
Provides: %{_cross_os}kernel(mkfs-confs)
Conflicts: %{_cross_os}kernel(mkfs-confs)

However, that said - I would not make this a separate subpackage, but instead add that Provides and Conflicts to the parent kernel package.

Summary: mkfs configurations for different file systems

%description mkfs-confs
%{summary}.

%if "%{_cross_arch}" == "x86_64"
%package modules-neuron
Summary: Modules for the Linux kernel with Neuron hardware
Expand Down Expand Up @@ -291,6 +300,9 @@ mkdir -p %{buildroot}%{_cross_unitdir}/[email protected]
install -p -m 0644 %{S:221} %{buildroot}%{_cross_unitdir}/[email protected]/neuron.conf
%endif

mkdir -p %{buildroot}%{_cross_datadir}/xfs
install -p -m 0644 %{S:300} %{buildroot}%{_cross_datadir}/xfs/mkfs.xfs.conf

%files
%license COPYING LICENSES/preferred/GPL-2.0 LICENSES/exceptions/Linux-syscall-note
%{_cross_attribution_file}
Expand Down Expand Up @@ -340,4 +352,7 @@ install -p -m 0644 %{S:221} %{buildroot}%{_cross_unitdir}/[email protected]
%files archive
%{_cross_datadir}/bottlerocket/kernel-devel.tar.xz

%files mkfs-confs
%{_cross_datadir}/xfs/mkfs.xfs.conf

%changelog
5 changes: 5 additions & 0 deletions packages/kernel-5.10/mkfs.xfs.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# V5 features that were the mkfs defaults when the upstream Linux 5.10 LTS
# kernel was released at the end of 2020.

[inode]
nrext64=0
Comment on lines +1 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the only feature of interest that's worth pinning? It seems a bit sparse.

Copy link
Contributor Author

@Sparksssj Sparksssj Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this is indeed the only feature we care about to fix the original issue. Unless you have more thoughts and we can sure add more in other PRs -- I still think we should only keep this one here because this is the one related to the issue.

15 changes: 15 additions & 0 deletions packages/kernel-5.15/kernel-5.15.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ Source100: config-bottlerocket
Source220: neuron-sysinit.target.drop-in.conf
Source221: [email protected]

# XFS configuration
Source300: mkfs.xfs.conf

# Help out-of-tree module builds run `make prepare` automatically.
Patch1001: 1001-Makefile-add-prepare-target-for-external-modules.patch
# Expose tools/* targets for out-of-tree module builds.
Expand Down Expand Up @@ -81,6 +84,12 @@ Summary: Modules for the Linux kernel
%description modules
%{summary}.

%package mkfs-confs
Summary: mkfs configurations for different file systems

%description mkfs-confs
%{summary}.

%if "%{_cross_arch}" == "x86_64"
%package modules-neuron
Summary: Modules for the Linux kernel with Neuron hardware
Expand Down Expand Up @@ -285,6 +294,9 @@ mkdir -p %{buildroot}%{_cross_unitdir}/[email protected]
install -p -m 0644 %{S:221} %{buildroot}%{_cross_unitdir}/[email protected]/neuron.conf
%endif

mkdir -p %{buildroot}%{_cross_datadir}/xfs
install -p -m 0644 %{S:300} %{buildroot}%{_cross_datadir}/xfs/mkfs.xfs.conf

%files
%license COPYING LICENSES/preferred/GPL-2.0 LICENSES/exceptions/Linux-syscall-note
%{_cross_attribution_file}
Expand Down Expand Up @@ -334,4 +346,7 @@ install -p -m 0644 %{S:221} %{buildroot}%{_cross_unitdir}/[email protected]
%files archive
%{_cross_datadir}/bottlerocket/kernel-devel.tar.xz

%files mkfs-confs
%{_cross_datadir}/xfs/mkfs.xfs.conf

%changelog
5 changes: 5 additions & 0 deletions packages/kernel-5.15/mkfs.xfs.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# V5 features that were the mkfs defaults when the upstream Linux 5.15 LTS
# kernel was released at the end of 2021.

[inode]
nrext64=0
15 changes: 15 additions & 0 deletions packages/kernel-6.1/kernel-6.1.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ Source300: bootconfig-aws.conf
Source301: bootconfig-vmware.conf
Source302: bootconfig-metal.conf

# XFS configuration
Source400: mkfs.xfs.conf

# Help out-of-tree module builds run `make prepare` automatically.
Patch1001: 1001-Makefile-add-prepare-target-for-external-modules.patch
# Expose tools/* targets for out-of-tree module builds.
Expand Down Expand Up @@ -138,6 +141,12 @@ Summary: Modules for the Linux kernel on bare metal
%description modules-metal
%{summary}.

%package mkfs-confs
Summary: mkfs configurations for different file systems

%description mkfs-confs
%{summary}.

%if "%{_cross_arch}" == "x86_64"
%package modules-neuron
Summary: Modules for the Linux kernel with Neuron hardware
Expand Down Expand Up @@ -377,6 +386,9 @@ mkdir -p %{buildroot}%{_cross_unitdir}/[email protected]
install -p -m 0644 %{S:221} %{buildroot}%{_cross_unitdir}/[email protected]/neuron.conf
%endif

mkdir -p %{buildroot}%{_cross_datadir}/xfs
install -p -m 0644 %{S:400} %{buildroot}%{_cross_datadir}/xfs/mkfs.xfs.conf

# Install platform-specific bootconfig snippets.
install -d %{buildroot}%{_cross_bootconfigdir}
install -p -m 0644 %{S:300} %{buildroot}%{_cross_bootconfigdir}/05-aws.conf
Expand Down Expand Up @@ -1431,4 +1443,7 @@ install -p -m 0644 %{S:302} %{buildroot}%{_cross_bootconfigdir}/05-metal.conf
%{_cross_unitdir}/[email protected]/neuron.conf
%endif

%files mkfs-confs
%{_cross_datadir}/xfs/mkfs.xfs.conf

%changelog
5 changes: 5 additions & 0 deletions packages/kernel-6.1/mkfs.xfs.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# V5 features that were the mkfs defaults when the upstream Linux 6.1 LTS
# kernel was released at the end of 2022.

[inode]
nrext64=0
Comment on lines +3 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep this as the default of 1 on 6.1? If I recall correctly, isn't that when the default of 1 was introduced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. But it will show EXPERIMENTAL Large extent counts feature in use. Use at your own risk!. Since we are not very confident about that so we just set it 0 for now. The purpose for this change is to conveniently change it to 1 when needed.

89 changes: 89 additions & 0 deletions packages/xfsprogs/0001-mkfs-source-defaults-from-config-file.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
From b20a9a213e1f4901072f559e3aef4797b5955c59 Mon Sep 17 00:00:00 2001
From: Sparks Song <[email protected]>
Date: Wed, 4 Dec 2024 23:47:28 +0000
Subject: [PATCH] mkfs: source defaults from config file to make nrext64
default off on multiple kernel versions

---
mkfs/xfs_mkfs.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 6d2469c..1f6f643 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -39,6 +39,9 @@
*/
#define WHACK_SIZE (128 * 1024)

+/* Default path for the mkfs.xfs configuration file */
+#define DEFAULT_CONFIG_PATH "/usr/share/xfs/mkfs.xfs.conf"
+
/*
* XXX: The configured block and sector sizes are defined as global variables so
* that they don't need to be passed to getnum/cvtnum().
@@ -4271,6 +4274,44 @@ cfgfile_parse(
cli->cfgfile);
}

+
+/* This function is similar to cfgfile_parse() and specifically parses
+ * default config files. It doesn’t exit on failure.
+ */
Comment on lines +29 to +32
Copy link
Contributor

@bcressey bcressey Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems almost identical to cfgfile_parse except the exit(1) has been removed and the message is changed in a trivial way.

Rather than duplicate the function, I would just leave the message unmodified (it will obviously point to the default config file) and add a check around exit(1) like this:

if (strcmp(cli->cfgfile, DEFAULT_CONFIG_PATH) != 0)
    exit(1);

The problem with the copy-paste-tweak approach to patching is that if the upstream version of the function ever changes, your copy-paste version won't pick up those changes, and the patch is very likely to still apply cleanly.

By modifying the existing function in a careful way, you are much more likely to see the patch fail to apply on newer versions if there's been a significant change, which is what you want as it will signal the need to revisit the downstream alteration.

+static void
+cfgfile_parse_default(
+ struct cli_params *cli)
+{
+ int error;
+
+ if (!cli->cfgfile)
+ return;
+
+ error = ini_parse(cli->cfgfile, cfgfile_parse_ini, cli);
+ if (error) {
+ if (error > 0) {
+ fprintf(stderr,
+ _("%s: Unrecognised input on line %d. Aborting.\n"),
+ cli->cfgfile, error);
+ } else if (error == -1) {
+ fprintf(stderr,
+ _("Unable to open default config file %s. Aborting.\n"),
+ cli->cfgfile);
+ } else if (error == -2) {
+ fprintf(stderr,
+ _("Memory allocation failure parsing %s. Aborting.\n"),
+ cli->cfgfile);
+ } else {
+ fprintf(stderr,
+ _("Unknown error %d opening default config file %s. Aborting.\n"),
+ error, cli->cfgfile);
+ }
+ } else {
+ printf(_("Parameters parsed from default config file %s successfully\n"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you see this output in the journal when it ran successfully? Do we expect this to be seen on every boot or are the mkfs.xfs calls for the XFS backed data partitions not emitting logs?

Copy link
Contributor Author

@Sparksssj Sparksssj Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment 1: I did make confirmation on aws-k8s-1.25 with size i3en.2xlarge and corresponding userdata which is exactly what user mentioned in original issue.
Comment 2: Yes. We do see this output when this's succeed. And these logs will not be seen directly in EC2 console. And can be seen if you run

umount /dev/nvme2n1
mkfs.xfs /dev/nvme2n1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so it sounds like you did validate that this code fixes the issue in bottlerocket-os/bottlerocket#4281 but just on aws-k8s-1.24 instead of aws-k8s-1.25 which should work the same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I did both 1.24 and 1.25 tests, both works.

+ cli->cfgfile);
+ }
+}
+
int
main(
int argc,
@@ -4428,7 +4469,15 @@ main(
* the options from this file parsed, we can then proceed with parameter
* and bounds checking and making the filesystem.
*/
- cfgfile_parse(&cli);
+
+ if (cli.cfgfile == NULL) {
+ /* No user config file specified, try default config */
+ cli.cfgfile = DEFAULT_CONFIG_PATH;
+ cfgfile_parse_default(&cli);
+ } else {
+ /* User specified their own config file, use that */
+ cfgfile_parse(&cli);
+ }
Comment on lines +74 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should all just be:

if (cli.cfgfile == NULL) {
    cli.cfgfile = DEFAULT_CONFIG_PATH;
}

cfgfile_parse(&cli);

That minimizes the changes to this function while still providing the fallback behavior you're trying to add.


protostring = setup_proto(cli.protofile);

--
2.47.0

7 changes: 6 additions & 1 deletion packages/xfsprogs/xfsprogs.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ License: GPL-2.0-only AND LGPL-2.1-only
URL: https://xfs.wiki.kernel.org
Source0: http://kernel.org/pub/linux/utils/fs/xfs/xfsprogs/xfsprogs-%{version}.tar.xz

Patch1000: 0001-mkfs-source-defaults-from-config-file.patch

BuildRequires: %{_cross_os}glibc-devel
BuildRequires: %{_cross_os}libuuid-devel
BuildRequires: %{_cross_os}libinih-devel
Expand All @@ -14,6 +16,9 @@ BuildRequires: %{_cross_os}libblkid-devel

Requires: %{_cross_os}liburcu
Requires: %{_cross_os}libinih
Requires: (%{_cross_os}kernel-5.10-mkfs-confs if %{_cross_os}kernel-5.10)
Requires: (%{_cross_os}kernel-5.15-mkfs-confs if %{_cross_os}kernel-5.15)
Requires: (%{_cross_os}kernel-6.1-mkfs-confs if %{_cross_os}kernel-6.1)
Comment on lines +19 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just require a generic capability that any kernel package can implement. Otherwise it will be challenging to bring a custom kernel of a different version.

Suggested change
Requires: (%{_cross_os}kernel-5.10-mkfs-confs if %{_cross_os}kernel-5.10)
Requires: (%{_cross_os}kernel-5.15-mkfs-confs if %{_cross_os}kernel-5.15)
Requires: (%{_cross_os}kernel-6.1-mkfs-confs if %{_cross_os}kernel-6.1)
Requires: (%{_cross_os}kernel(mkfs-confs)


%description
%{summary}.
Expand All @@ -26,7 +31,7 @@ Requires: %{name}
%{summary}.

%prep
%autosetup -n xfsprogs-%{version}
%autosetup -n xfsprogs-%{version} -p1

%build
%cross_configure \
Expand Down