Ext4 detection #13496

travisdowns · 2023-09-18T15:48:04Z

Assume that ext filesystems are ext4

Due to scylladb/seastar#251 all of ext2, ext3 and ext4 will be
detected as ext2 (they shared the same superblock magic number).

This leads to an erroneous warning claiming that you are not running
on ext4 nor XFS. For now, we will simply that assume any ext detection
means ext4, as ext2 and 3 use is very limited as ext4 was rolled out
more than 15 years ago and almost entirely supplanted use of the
earlier variants.

A partial fix for:

#13469

A more involved fix may follow to resolve seastar#251.

Backports Required

Release Notes

Bug Fixes

ext4 is no longer incorrectly detected as ext2 (all of ext2, 3 and 4 are assumed to be ext4).

Print the detected filsystem type at info always, since this is critical one-time info. Also print the detected fs type in the warning message when the type is not xfs or ext4.

Due to scylladb/seastar#251 all of ext2, ext3 and ext4 will be detected as ext2 (they shared the same superblock magic number). This leads to an erroneous warning claiming that you are not running on ext4 nor XFS. For now, we will simply that assume any ext detection means ext4, as ext2 and 3 use is very limited as ext4 was rolled out more than 15 years ago and almost entirely supplanted use of the earlier variants. In any case, the change only changes the warning message emitted, not any other behavior. Issue redpanda-data#13469.

To make it clearer why we treat ext2 as ext4.

src/v/syschecks/syschecks.cc

StephanDollberg · 2023-09-18T18:17:40Z

src/v/syschecks/syschecks.cc

+            // all of ext2, 3 and 4 are detected as ext2, so we just assume an
+            // ext2 detection means ext4 for now, see:
+            // https://github.com/redpanda-data/redpanda/issues/13469
+            if (fs == ss::fs_type::ext2) {


Shouldn't we just also make this check for ext3 and ext4? Then it will automatically work once upstream is fixed.

+1 perhaps we should explicitly define the set of supported filesystems and the set of recommended filesystems, and log them each in these warning/error messages

@StephanDollberg - I'm not sure. If we get ext3 as a response, what does it mean? That upstream has been fixed and the fs being is used is actually ext3? Officially we don't support ext3, and so I'm not sure the warning is right in spirit and it's also wrong when it says "you are on ext4 ...". Similarly if upstream has been fixed, then it's wrong to include ext2 in this check: it should be changed to only include ext4 at that point.

However, I agree that if we ever get ext4 I guess we should print this ext4 warning. So maybe fs == ext2 || fs == ext4 is the best check? Seems weird.

BTW here's my suggested upstream fix:

scylladb/seastar#1837

It does not change the behavior of the existing method, but adds a new one. So in that case we'd need another change here to use the new method. I'm not sure if it will be accept or if it's the best approach.

+1 perhaps we should explicitly define the set of supported filesystems and the set of recommended filesystems, and log them each in these warning/error messages

As far as I know there is only one supported and recommended file system: xfs. ext4 is in a grey zone.

In any case, does the code not explicitly define these now via its "if cascade"?

Right, from what you mentioned in the meeting I had understood that ext2/3/4 are actually quite similar.

However, I agree that if we ever get ext4 I guess we should print this ext4 warning. So maybe fs == ext2 || fs == ext4 is the best check? Seems weird.

This does seem like a good middle ground even if a bit weird.

I guess it all depends a bit on what you want to do with the seastar fix. If you are going to cherry pick it into our fork now anyway then yeah it's fine lets just merge as is. Just want to avoid a scenario where we later update seastar which fixed the detection and then this breaks. Backports also an angle to consider but I guess we never really pull in new stuff there (unless done intentionally).

I guess it all depends a bit on what you want to do with the seastar fix. If you are going to cherry pick it into our fork now anyway then yeah it's fine lets just merge as is.

I don't plan to cherry-pick it immediately but see what happens upstream. I don't have any strong expectation there: it's entirely possible nothing gets accepted and so the current state of the syscheck lives forever, or maybe it will be changed to not introduce a new method but actually fix file_system_at() in which case we would suddenly start returning ext3 or ext4 here as you are concerned about.

In any case, I agree that the == ext2 | ext4 check is reasonable and I'll change it to that.

Backports also an angle to consider but I guess we never really pull in new stuff there (unless done intentionally).

Do you mean backporting this change? I plan to backport it to 23.2 only.

Do you mean backporting this change? I plan to backport it to 23.2 only.

Yeah meant this change. Sounds fine, probably no new clusters being set up on 23.1 so unlikely to run into the confusing log message.

andrwng

Not much more to add to Stephan's comments, and they're just nits. Otherwise, LGTM

andrwng · 2023-09-18T18:31:12Z

src/v/syschecks/syschecks.cc

@@ -53,6 +53,7 @@ ss::future<> disk(const ss::sstring& path) {
    return ss::check_direct_io_support(path).then([path] {
        return ss::file_system_at(path).then([path](auto fs) {
            checklog.info0("Detected file system type is {}", to_string(fs));
+            if (fs == ss::fs_type::ext2) {
                checklog.warn(
                  "Path: `{}' is on ext4, not XFS. This will probably work, "


nit: perhaps update to "ext4-like filesystem"

@andrwng - does this comment apply to line 56: if (fs == ss::fs_type::ext2) {?

Note that ss::fs_type is part of seastar.

Ah no it was meant for the logged warning ""Path: `{}' is on ext4, not XFS"

andrwng · 2023-09-18T18:32:02Z

src/v/syschecks/syschecks.cc

+            // all of ext2, 3 and 4 are detected as ext2, so we just assume an
+            // ext2 detection means ext4 for now, see:
+            // https://github.com/redpanda-data/redpanda/issues/13469
+            if (fs == ss::fs_type::ext2) {


+1 perhaps we should explicitly define the set of supported filesystems and the set of recommended filesystems, and log them each in these warning/error messages

As pointed out in review.

We semi-support ext4 but in the syscheck we will now check for both ext2 and ext4, as currently ext4 is always detected as ext2, but in the future if ext4 is returned we should also treat this as ext4. Issue redpanda-data#13469.

andrwng · 2023-09-19T16:06:04Z

src/v/syschecks/syschecks.cc

@@ -53,6 +53,7 @@ ss::future<> disk(const ss::sstring& path) {
    return ss::check_direct_io_support(path).then([path] {
        return ss::file_system_at(path).then([path](auto fs) {
            checklog.info0("Detected file system type is {}", to_string(fs));
+            if (fs == ss::fs_type::ext2) {
                checklog.warn(
                  "Path: `{}' is on ext4, not XFS. This will probably work, "


Ah no it was meant for the logged warning ""Path: `{}' is on ext4, not XFS"

vbotbuildovich · 2023-12-14T22:23:11Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/37241#018c6a26-3d0d-4200-abf5-21385852cee3

vbotbuildovich · 2023-12-20T15:52:52Z

/backport v23.2.x

Changed in redpanda-data#13496

nvartolomei · 2023-12-21T20:27:21Z

/backport v23.3.x

Changed in redpanda-data#13496 (cherry picked from commit 31ddb2d)

github-actions bot added the area/redpanda label Sep 18, 2023

travisdowns added 3 commits September 18, 2023 12:50

Log detected filesystem type

b6a869f

Print the detected filsystem type at info always, since this is critical one-time info. Also print the detected fs type in the warning message when the type is not xfs or ext4.

Add comment regarding ext detection

f99eb9e

To make it clearer why we treat ext2 as ext4.

travisdowns force-pushed the td-13469-ext4-detection branch from c0dcbf5 to f99eb9e Compare September 18, 2023 15:50

travisdowns self-assigned this Sep 18, 2023

travisdowns requested a review from StephanDollberg September 18, 2023 18:13

StephanDollberg previously approved these changes Sep 18, 2023

View reviewed changes

andrwng previously approved these changes Sep 18, 2023

View reviewed changes

Remove redundant lambda

328c245

As pointed out in review.

travisdowns dismissed stale reviews from andrwng and StephanDollberg via 328c245 September 19, 2023 04:39

Add ext4 to the syschecks FS check

051c63c

We semi-support ext4 but in the syscheck we will now check for both ext2 and ext4, as currently ext4 is always detected as ext2, but in the future if ext4 is returned we should also treat this as ext4. Issue redpanda-data#13469.

travisdowns requested review from StephanDollberg and andrwng September 19, 2023 14:27

StephanDollberg approved these changes Sep 19, 2023

View reviewed changes

andrwng approved these changes Sep 19, 2023

View reviewed changes

travisdowns merged commit 853d9ed into redpanda-data:dev Dec 20, 2023
25 checks passed

vbotbuildovich mentioned this pull request Dec 20, 2023

[v23.2.x] Ext4 detection #15812

Merged

nvartolomei added a commit to nvartolomei/redpanda that referenced this pull request Dec 21, 2023

rptest: allow the new version of xfs/ext4 fs error msg

31ddb2d

Changed in redpanda-data#13496

nvartolomei mentioned this pull request Dec 21, 2023

rptest: allow the new version of xfs/ext4 fs error msg #15853

Merged

7 tasks

vbotbuildovich mentioned this pull request Dec 21, 2023

[v23.3.x] Ext4 detection #15855

Merged

vbotbuildovich pushed a commit to vbotbuildovich/redpanda that referenced this pull request Dec 22, 2023

rptest: allow the new version of xfs/ext4 fs error msg

f7f00e9

Changed in redpanda-data#13496 (cherry picked from commit 31ddb2d)

vbotbuildovich pushed a commit to vbotbuildovich/redpanda that referenced this pull request Dec 22, 2023

rptest: allow the new version of xfs/ext4 fs error msg

34f552b

Changed in redpanda-data#13496 (cherry picked from commit 31ddb2d)

travisdowns mentioned this pull request Jan 8, 2024

When running on ext4 a warning is emitted suggesting we are not running on ext4 #13469

Closed

renovate bot mentioned this pull request May 4, 2024

feat(github-release)!: Update redpanda-operator to v24.1.6 otosky/home-ops#1232

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ext4 detection #13496

Ext4 detection #13496

travisdowns commented Sep 18, 2023 •

edited

StephanDollberg Sep 18, 2023

andrwng Sep 18, 2023

travisdowns Sep 19, 2023

travisdowns Sep 19, 2023

travisdowns Sep 19, 2023

travisdowns Sep 19, 2023

StephanDollberg Sep 19, 2023

travisdowns Sep 19, 2023

StephanDollberg Sep 19, 2023

andrwng left a comment

andrwng Sep 18, 2023

travisdowns Sep 19, 2023

andrwng Sep 19, 2023

andrwng Sep 18, 2023

andrwng Sep 19, 2023

vbotbuildovich commented Dec 14, 2023

vbotbuildovich commented Dec 20, 2023

nvartolomei commented Dec 21, 2023

Ext4 detection #13496

Ext4 detection #13496

Conversation

travisdowns commented Sep 18, 2023 • edited

Backports Required

Release Notes

Bug Fixes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Dec 14, 2023

vbotbuildovich commented Dec 20, 2023

nvartolomei commented Dec 21, 2023

travisdowns commented Sep 18, 2023 •

edited