e2fsck: make sure orphan files are cleaned-up
cover e2fsck: make sure orphan files are cleaned-up
Hi!
I'm sending a fix to e2fsck that forces the filesystem checks to happen when the orphan file is present in the filesystem. This patch resulted from a bug reported in openSUSE Tumbleweed[1] where e2fsck doesn't clean-up this file and later the filesystem fails to be mounted read-only (because it still requires recovery).
I'm also sending a new test to validate this scenario.
[1] https://bugzilla.suse.com/show_bug.cgi?id=1226043
Luis Henriques (SUSE) (2):
e2fsck: don'k skip checks if the orphan file is present in the
filesystem
tests: new test to check that the orphan file is cleaned up
e2fsck/unix.c | 4 ++++ tests/f_clear_orphan_file/expect.1 | 35 +++++++++++++++++++++++++++++ tests/f_clear_orphan_file/expect.2 | 7 ++++++ tests/f_clear_orphan_file/image.gz | Bin 0 -> 12449 bytes tests/f_clear_orphan_file/name | 1 + tests/f_clear_orphan_file/script | 2 ++ 6 files changed, 49 insertions(+) create mode 100644 tests/f_clear_orphan_file/expect.1 create mode 100644 tests/f_clear_orphan_file/expect.2 create mode 100644 tests/f_clear_orphan_file/image.gz create mode 100644 tests/f_clear_orphan_file/name create mode 100644 tests/f_clear_orphan_file/script
PATCH 1/2 e2fsck: don't skip checks if the orphan file is present in the filesystem
If the filesystem supports the orphan file feature and that file is present then don't skip the filesystem checks as that file may need to be cleaned up.
--- e2fsck/unix.c | 4 ++++ 1 file changed, 4 insertions(+)
e2fsck/unix.c +4 -0
@@ -371,6 +371,10 @@ static void check_if_skip(e2fsck_t ctx)if (ctx->options &E2F_OPT_JOURNAL_ONLY ) goto skip; +if (ext2fs_has_feature_orphan_file (fs->super) && +ext2fs_has_feature_orphan_present (fs->super)) +return ; + lastcheck =ext2fs_get_tstamp (sb, s_lastcheck);if (lastcheck > ctx->now) lastcheck -= ctx->time_fudge;
PATCH 2/2 tests: new test to check that the orphan file is cleaned up
In reply to Luis Henriques (SUSE)This test verifies that e2fsck clears the orphan file if it is present. The filesystem was created by simply creating a bunch of empty files and, while those files were open by an application, delete some of them in order to add them to the orphan file. After this, the filesystem is simply powered off. An e2fsck will need to clear the orphaned inodes but also to clean the orphan file.
--- tests/f_clear_orphan_file/expect.1 | 35 +++++++++++++++++++++++++++++ tests/f_clear_orphan_file/expect.2 | 7 ++++++ tests/f_clear_orphan_file/image.gz | Bin 0 -> 12449 bytes tests/f_clear_orphan_file/name | 1 + tests/f_clear_orphan_file/script | 2 ++ 5 files changed, 45 insertions(+) create mode 100644 tests/f_clear_orphan_file/expect.1 create mode 100644 tests/f_clear_orphan_file/expect.2 create mode 100644 tests/f_clear_orphan_file/image.gz create mode 100644 tests/f_clear_orphan_file/name create mode 100644 tests/f_clear_orphan_file/script
tests/f_clear_orphan_file/expect.1 new file +35 -0
@@ -0,0 +1,35 @@ +test_filesys: recovering journal +Clearing orphaned inode 13 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 14 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 15 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 16 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 17 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 18 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 19 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 20 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 21 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 22 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 23 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 24 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 25 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 26 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 27 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 28 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 29 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 30 (uid=0, gid=0, mode=0100644, size=0) +Clearing orphaned inode 31 (uid=0, gid=0, mode=0100644, size=0) +Pass 1: Checking inodes, blocks, and sizes +Pass 2: Checking directory structure +Pass 3: Checking directory connectivity +Pass 4: Checking reference counts +Pass 5: Checking group summary information +Free inodes count wrong (2055, counted=2005). +Fix? yes + +Orphan file (inode 12) block 0 is not clean. +Clear? yes + + +test_filesys: ***** FILE SYSTEM WAS MODIFIED ***** +test_filesys: 43/2048 files (2.3% non-contiguous), 1650/8192 blocks +Exit status is 1
tests/f_clear_orphan_file/expect.2 new file +7 -0
@@ -0,0 +1,7 @@ +Pass 1: Checking inodes, blocks, and sizes +Pass 2: Checking directory structure +Pass 3: Checking directory connectivity +Pass 4: Checking reference counts +Pass 5: Checking group summary information +test_filesys: 43/2048 files (2.3% non-contiguous), 1650/8192 blocks +Exit status is 0
tests/f_clear_orphan_file/image.gz new file binary +0 -0
tests/f_clear_orphan_file/name new file +1 -0
@@ -0,0 +1 @@ +clear orphan file
tests/f_clear_orphan_file/script new file +2 -0
@@ -0,0 +1,2 @@ +FSCK_OPT=-y +. $cmd_dir/run_e2fsck
Quoted 9 lines · Luis Henriques (SUSE)
Hi!
I'm sending a fix to e2fsck that forces the filesystem checks to happen when the orphan file is present in the filesystem. This patch resulted from a bug reported in openSUSE Tumbleweed[1] where e2fsck doesn't clean-up this file and later the filesystem fails to be mounted read-only (because it still requires recovery).
I'm also sending a new test to validate this scenario.
I know it's holidays season, but since I've sent this a while ago I believe it's time for a ping.
Cheers,
Signature · 1 line
Luís
Quoted 20 lines
[1] https://bugzilla.suse.com/show_bug.cgi?id=1226043
Luis Henriques (SUSE) (2):
e2fsck: don'k skip checks if the orphan file is present in the
filesystem
tests: new test to check that the orphan file is cleaned upe2fsck/unix.c | 4 ++++ tests/f_clear_orphan_file/expect.1 | 35 +++++++++++++++++++++++++++++ tests/f_clear_orphan_file/expect.2 | 7 ++++++ tests/f_clear_orphan_file/image.gz | Bin 0 -> 12449 bytes tests/f_clear_orphan_file/name | 1 + tests/f_clear_orphan_file/script | 2 ++ 6 files changed, 49 insertions(+) create mode 100644 tests/f_clear_orphan_file/expect.1 create mode 100644 tests/f_clear_orphan_file/expect.2 create mode 100644 tests/f_clear_orphan_file/image.gz create mode 100644 tests/f_clear_orphan_file/name create mode 100644 tests/f_clear_orphan_file/script
Quoted 7 lines · Luis Henriques (SUSE)
Hi!
I'm sending a fix to e2fsck that forces the filesystem checks to happen when the orphan file is present in the filesystem. This patch resulted from a bug reported in openSUSE Tumbleweed[1] where e2fsck doesn't clean-up this file and later the filesystem fails to be mounted read-only (because it still requires recovery).
Looks like Fedora is hitting this bug now:
https://bugzilla.redhat.com/show_bug.cgi?id=2318710
(unclear why fedora upgrade is leaving an unclean root fs on reboot, but that's a separate issue.)
With this patch in place, bare e2fsck asks for confirmation, not sure if that's expected. But with "yes" answers, the filesystem is cleaned properly and mounts just fine.
Also - shouldn't we go ahead and deal with the orphan inode file even on a readonly mount, as long as the bdev itself is not readonly?
ext4_mark_recovery_complete():
if (sb_rdonly(sb) && (ext4_has_feature_journal_needs_recovery(sb) ||
ext4_has_feature_orphan_present(sb))) {
if (!ext4_orphan_file_empty(sb)) {
ext4_error(sb, "Orphan file not empty on read-only fs.");
err = -EFSCORRUPTED;
goto out;
}
ext4_clear_feature_journal_needs_recovery(sb);
ext4_clear_feature_orphan_present(sb);
ext4_commit_super(sb);
}# losetup /dev/loop0 2318710-e2image.raw ## from above bz attachment
# e2fsck /dev/loop0 (without this patch)
...
# mount -o ro /dev/loop0 mnt
mount: /root/e2fsprogs/mnt: fsconfig system call failed: Structure needs cleaning.
dmesg(1) may have more information after failed mount system call.
# dmesg | tail -n 2
[ 3083.343622] EXT4-fs error (device loop0): ext4_mark_recovery_complete:6229: comm mount: Orphan file not empty on read-only fs.
[ 3083.345339] EXT4-fs (loop0): mount failed
# mount -o rw /dev/loop0 mnt
# echo $?
0-Eric
Quoted 23 lines
I'm also sending a new test to validate this scenario.
[1] https://bugzilla.suse.com/show_bug.cgi?id=1226043
Luis Henriques (SUSE) (2):
e2fsck: don'k skip checks if the orphan file is present in the
filesystem
tests: new test to check that the orphan file is cleaned upe2fsck/unix.c | 4 ++++ tests/f_clear_orphan_file/expect.1 | 35 +++++++++++++++++++++++++++++ tests/f_clear_orphan_file/expect.2 | 7 ++++++ tests/f_clear_orphan_file/image.gz | Bin 0 -> 12449 bytes tests/f_clear_orphan_file/name | 1 + tests/f_clear_orphan_file/script | 2 ++ 6 files changed, 49 insertions(+) create mode 100644 tests/f_clear_orphan_file/expect.1 create mode 100644 tests/f_clear_orphan_file/expect.2 create mode 100644 tests/f_clear_orphan_file/image.gz create mode 100644 tests/f_clear_orphan_file/name create mode 100644 tests/f_clear_orphan_file/script
Quoted 16 lines · Eric Sandeen
Quoted 7 lines · Luis Henriques (SUSE)
Hi!
I'm sending a fix to e2fsck that forces the filesystem checks to happen when the orphan file is present in the filesystem. This patch resulted from a bug reported in openSUSE Tumbleweed[1] where e2fsck doesn't clean-up this file and later the filesystem fails to be mounted read-only (because it still requires recovery).
Looks like Fedora is hitting this bug now:
https://bugzilla.redhat.com/show_bug.cgi?id=2318710
(unclear why fedora upgrade is leaving an unclean root fs on reboot, but that's a separate issue.)
Quoted 6 lines
With this patch in place, bare e2fsck asks for confirmation, not sure if that's expected. But with "yes" answers, the filesystem is cleaned properly and mounts just fine.
Also - shouldn't we go ahead and deal with the orphan inode file even on a readonly mount, as long as the bdev itself is not readonly?
Since that would be a filesystem-level change, my opinion is that we should not do that in a read-only mount. But that's just my opinion and maybe there are other similar cases (I didn't check) where changes are written on read-only mounts.
Cheers,
Signature · 1 line
Luís
Quoted 54 lines
ext4_mark_recovery_complete():if (sb_rdonly(sb) && (ext4_has_feature_journal_needs_recovery(sb) || ext4_has_feature_orphan_present(sb))) { if (!ext4_orphan_file_empty(sb)) { ext4_error(sb, "Orphan file not empty on read-only fs."); err = -EFSCORRUPTED; goto out; } ext4_clear_feature_journal_needs_recovery(sb); ext4_clear_feature_orphan_present(sb); ext4_commit_super(sb); }# losetup /dev/loop0 2318710-e2image.raw ## from above bz attachment # e2fsck /dev/loop0 (without this patch) ... # mount -o ro /dev/loop0 mnt mount: /root/e2fsprogs/mnt: fsconfig system call failed: Structure needs cleaning. dmesg(1) may have more information after failed mount system call. # dmesg | tail -n 2 [ 3083.343622] EXT4-fs error (device loop0): ext4_mark_recovery_complete:6229: comm mount: Orphan file not empty on read-only fs. [ 3083.345339] EXT4-fs (loop0): mount failed # mount -o rw /dev/loop0 mnt # echo $? 0-Eric
Quoted 23 lines
I'm also sending a new test to validate this scenario.
[1] https://bugzilla.suse.com/show_bug.cgi?id=1226043
Luis Henriques (SUSE) (2):
e2fsck: don'k skip checks if the orphan file is present in the
filesystem
tests: new test to check that the orphan file is cleaned upe2fsck/unix.c | 4 ++++ tests/f_clear_orphan_file/expect.1 | 35 +++++++++++++++++++++++++++++ tests/f_clear_orphan_file/expect.2 | 7 ++++++ tests/f_clear_orphan_file/image.gz | Bin 0 -> 12449 bytes tests/f_clear_orphan_file/name | 1 + tests/f_clear_orphan_file/script | 2 ++ 6 files changed, 49 insertions(+) create mode 100644 tests/f_clear_orphan_file/expect.1 create mode 100644 tests/f_clear_orphan_file/expect.2 create mode 100644 tests/f_clear_orphan_file/image.gz create mode 100644 tests/f_clear_orphan_file/name create mode 100644 tests/f_clear_orphan_file/script
Quoted 22 lines · Luis Henriques
Quoted 20 lines · Eric Sandeen
Quoted 7 lines · Luis Henriques (SUSE)
Hi!
I'm sending a fix to e2fsck that forces the filesystem checks to happen when the orphan file is present in the filesystem. This patch resulted from a bug reported in openSUSE Tumbleweed[1] where e2fsck doesn't clean-up this file and later the filesystem fails to be mounted read-only (because it still requires recovery).
Looks like Fedora is hitting this bug now:
https://bugzilla.redhat.com/show_bug.cgi?id=2318710
(unclear why fedora upgrade is leaving an unclean root fs on reboot, but that's a separate issue.)
With this patch in place, bare e2fsck asks for confirmation, not sure if that's expected. But with "yes" answers, the filesystem is cleaned properly and mounts just fine.
Quoted 7 lines
Quoted 2 lines
Also - shouldn't we go ahead and deal with the orphan inode file even on a readonly mount, as long as the bdev itself is not readonly?
Since that would be a filesystem-level change, my opinion is that we should not do that in a read-only mount. But that's just my opinion and maybe there are other similar cases (I didn't check) where changes are written on read-only mounts.
Well, we do the whole log replay on readonly mounts, as long as the device itself is not RO. But I guess I don't know if we fully process orphan inodes during an RO replay. *shrug*
Anyway, thank you for this patch, it's in Fedora now, would be great to get it upstream.
-Eric
Theodore Ts'o
AppliedQuoted 9 lines · Luis Henriques (SUSE)
I'm sending a fix to e2fsck that forces the filesystem checks to happen when the orphan file is present in the filesystem. This patch resulted from a bug reported in openSUSE Tumbleweed[1] where e2fsck doesn't clean-up this file and later the filesystem fails to be mounted read-only (because it still requires recovery).
I'm also sending a new test to validate this scenario.
[...]
Applied, thanks!
[1/2] e2fsck: don't skip checks if the orphan file is present in the filesystem
commit: a8df015009e7cd71b411f21e7d6f0797a28cba5c
[2/2] tests: new test to check that the orphan file is cleaned up
commit: 9448aedd93521deb038f64ba16eff574628f0632Best regards,
Signature · 1 line
Theodore Ts'o <[email protected]>