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.

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

Reinstate the old zpool read label logic as a fallback. #12040

Merged
merged 1 commit into from
May 27, 2021

Conversation

rincebrain
Copy link
Contributor

Motivation and Context

#12032

Description

I landed a verbatim copy of the original zpool_read_label as "zpool_read_label_slow", then reshuffled the logic in
zpool_read_label to call zpool_read_label_slow if any of the AIO operations returned EOPNOTSUPP, which is what
FBSD spits back when you try AIO on FDs where it thinks it's "unsafe".

(The reshuffling was necessary because errno after this happens isn't (always?) EOPNOTSUPP, at least some of the time it's EIO; I landed the whole old function rather than integrating the new and old logic into one function after seeing that the diff between them was almost all of the function body, so it would have just been two entirely separate copies of the logic inside anyway.)

How Has This Been Tested?

  • Tried importing and creating a pool over NFS on FBSD 13/x86_64 before and after this change with vfs.aio.enable_unsafe=0 (it fails before and works after).
  • Tried pool creation and importing on Debian bullseye/x86_64
  • Tried pool importing on CentOS 7/x86_64
  • Tried pool importing on Ubuntu 18.04/x86_64

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@behlendorf behlendorf requested review from a user and lundman May 13, 2021 16:55
@behlendorf
Copy link
Contributor

cc: @asomers

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 13, 2021
Copy link
Contributor

@asomers asomers left a comment

Choose a reason for hiding this comment

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

I think this is a good idea. It never occurred to me that somebody might try to create a pool over NFS.

lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Show resolved Hide resolved
lib/libzutil/zutil_import.c Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented May 17, 2021

Now we just need to squash all those commits and make sure the final commit is signed off on to satisfy checkstyle.

@rincebrain rincebrain force-pushed the zfs_fbsdaio branch 2 times, most recently from 59c2841 to 6a4a48b Compare May 17, 2021 16:04
@rincebrain
Copy link
Contributor Author

Hm, the merge with master plus the rebase has it reporting that I committed all the commits that were merged from master. LMK if I should have done that some other way, and I'll go rewrite history appropriately.

In case of AIO failure, we should probably fallback to the old
behavior and still work.

Closes: openzfs#12032

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@ghost ghost force-pushed the zfs_fbsdaio branch from 6a4a48b to ca52c43 Compare May 17, 2021 16:13
@ghost
Copy link

ghost commented May 17, 2021

No worries, sorted it out for you.

@rincebrain
Copy link
Contributor Author

For my future reference, how did you clean it up?

(...also, I didn't realize contributors could force push to others' branches.)

@ghost
Copy link

ghost commented May 17, 2021

For my future reference, how did you clean it up?

git rebase -i master making sure only the one commit was being picked.

(...also, I didn't realize contributors could force push to others' branches.)

There's a checkbox when you open a PR that allows contributors to make changes.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 27, 2021
@behlendorf behlendorf merged commit 0bb736c into openzfs:master May 27, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 28, 2021
In case of AIO failure, we should probably fallback to the old
behavior and still work.

Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Reviewed-by: Alan Somers <asomers@gmail.com>
Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12032
Closes openzfs#12040
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
In case of AIO failure, we should probably fallback to the old
behavior and still work.

Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Reviewed-by: Alan Somers <asomers@gmail.com>
Reviewed-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12032
Closes openzfs#12040
@rincebrain rincebrain deleted the zfs_fbsdaio branch October 23, 2021 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants