Skip to content

Do not perform xfs_repair on xfs filesystem #150

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

Merged
merged 1 commit into from
Mar 24, 2020
Merged

Do not perform xfs_repair on xfs filesystem #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 1 addition & 35 deletions mount/mount_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,33 +314,6 @@ func (mounter *SafeFormatAndMount) checkAndRepairFilesystem(source string) error
return nil
}

// checkAndRepairXfsFilesystem checks and repairs xfs filesystem using command xfs_repair.
func (mounter *SafeFormatAndMount) checkAndRepairXfsFilesystem(source string) error {
klog.V(4).Infof("Checking for issues with xfs_repair on disk: %s", source)

args := []string{source}
checkArgs := []string{"-n", source}

// check-only using "xfs_repair -n", if the exit status is not 0, perform a "xfs_repair"
_, err := mounter.Exec.Command("xfs_repair", checkArgs...).CombinedOutput()
if err != nil {
if err == utilexec.ErrExecutableNotFound {
klog.Warningf("'xfs_repair' not found on system; continuing mount without running 'xfs_repair'.")
return nil
} else {
klog.Warningf("Filesystem corruption was detected for %s, running xfs_repair to repair", source)
out, err := mounter.Exec.Command("xfs_repair", args...).CombinedOutput()
if err != nil {
return NewMountError(HasFilesystemErrors, "'xfs_repair' found errors on device %s but could not correct them: %s\n", source, out)
} else {
klog.Infof("Device %s has errors which were corrected by xfs_repair.", source)
return nil
}
}
}
return nil
}

// formatAndMount uses unix utils to format and mount the given disk
func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
readOnly := false
Expand Down Expand Up @@ -410,14 +383,7 @@ func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target

if !readOnly {
// Run check tools on the disk to fix repairable issues, only do this for formatted volumes requested as rw.
var err error
switch existingFormat {
case "xfs":
err = mounter.checkAndRepairXfsFilesystem(source)
default:
err = mounter.checkAndRepairFilesystem(source)
}

err := mounter.checkAndRepairFilesystem(source)
Copy link
Member Author

Choose a reason for hiding this comment

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

code before change:

	if !readOnly {
		// Run fsck on the disk to fix repairable issues, only do this for volumes requested as rw.
		klog.V(4).Infof("Checking for issues with fsck on disk: %s", source)
		args := []string{"-a", source}
		out, err := mounter.Exec.Command("fsck", args...).CombinedOutput()
		if err != nil {
			ee, isExitError := err.(utilexec.ExitError)
			switch {
			case err == utilexec.ErrExecutableNotFound:
				klog.Warningf("'fsck' not found on system; continuing mount without running 'fsck'.")
			case isExitError && ee.ExitStatus() == fsckErrorsCorrected:
				klog.Infof("Device %s has errors which were corrected by fsck.", source)
			case isExitError && ee.ExitStatus() == fsckErrorsUncorrected:
				return NewMountError(HasFilesystemErrors, "'fsck' found errors on device %s but could not correct them: %s", source, string(out))
			case isExitError && ee.ExitStatus() > fsckErrorsUncorrected:
				klog.Infof("`fsck` error %s", string(out))
			}
		}
	}

and hence should be same as before.

Copy link
Member

Choose a reason for hiding this comment

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

The code that is part of checkAndRepairFilesystem(...) used to be called before // Check if the disk is already formatted. As part of #126 it was moved down here. Should we revert this change as well or is it necessary for some reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could revert that change too but I figured it is harmless and helps to keep diff of this PR small. We will have to fix all the breaking tests because order in which fsck gets called will change.

Also it is perfectly fine to not run fsck on a unformatted disk.

Copy link
Member

Choose a reason for hiding this comment

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

Ack. Sounds good to me. @msau42 @xing-yang @jsafrane any objections?

Copy link

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

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

lgtm. The bug report had the error past the fs format check, so it's not the fs format check that's failing.

Copy link
Member

Choose a reason for hiding this comment

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

The bug report had the error past the fs format check, so it's not the fs format check that's failing.

@gnufied can you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

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

which bug report? #141 or #125 ? I assume #141 - yes the problem isn't format checking but problem was that repairing the disk actually prevented volume from being mounted and recovery (although in some cases XFS volume can't recover itself and requires xfs_repair).

So, the only valid reason I did not do a full revert is because I liked not having to do a fsck if disk is unformatted.

Copy link
Member

Choose a reason for hiding this comment

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

Ack. SGTM

if err != nil {
return err
}
Expand Down
38 changes: 0 additions & 38 deletions mount/safe_format_and_mount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,44 +202,6 @@ func TestSafeFormatAndMount(t *testing.T) {
},
expErrorType: GetDiskFormatFailed,
},
{
description: "Test that 'xfs_repair' is called only once, no need to repair the filesystem",
fstype: "xfs",
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", nil},
},
},
{
description: "Test that 'xfs_repair' is called twice and repair the filesystem",
fstype: "xfs",
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
{"xfs_repair", []string{"/dev/foo"}, "\ndone\n", nil},
},
},
{
description: "Test that 'xfs_repair' is called twice and repair the filesystem, but mount failed",
fstype: "xfs",
mountErrs: []error{fmt.Errorf("unknown filesystem type '(null)'")},
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
{"xfs_repair", []string{"/dev/foo"}, "\ndone\n", nil},
},
expErrorType: UnknownMountError,
},
{
description: "Test that 'xfs_repair' is called twice but could not repair the filesystem",
fstype: "xfs",
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "DEVNAME=/dev/foo\nTYPE=xfs\n", nil},
{"xfs_repair", []string{"-n", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 1}},
{"xfs_repair", []string{"/dev/foo"}, "\nAn error occurred\n", &testingexec.FakeExitError{Status: 1}},
},
expErrorType: HasFilesystemErrors,
},
{
description: "Test that 'blkid' is called and confirms unformatted disk, format fails with sensitive options",
fstype: "ext4",
Expand Down