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

zfs-import-{cache,scan}: change condition to FileNotEmpty #11568

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

nabijaczleweli
Copy link
Contributor

Motivation and Context

Sometimes(?) zfs generates an empty cache file for me, and the import fails, which is suboptimal, since this means that dracut fails, and I have to type in zpool import -a to boot, delete the file, and regenerate+reinstall the initrd

Description

This works around this by ignoring a clearly bogus cache, while still supporting systems that do utilise one

How Has This Been Tested?

Applied equivalent change to the services on my laptop, regenerated and rebooted for each of the following: no cachefile => -scan, empty cachefile => -scan, non-empty cachefile => -cache. This is superiour to the previous behaviour of empty cachefile => -cache (failed).

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:

  • My code follows the OpenZFS code style requirements.
  • I have updated the documentation accordingly. – I didn't, but I don't think there is any, so
  • I have read the contributing document.
  • I have added tests to cover my changes. – N/A
  • I have run the ZFS Test Suite with this change applied. – dunno how to, but I don't consider it relevant here
  • All commit messages are properly formatted and contain Signed-off-by.

Sometimes(?) zfs generates an empty cache file for me,
and the import fails, which is suboptimal, since this means that dracut
fails, and I have to type in zpool import -a to boot, delete the file,
and regenerate+reinstall the initrd

This works around this by ignoring a clearly bogus cache,
while still supporting systems that do utilise one

Signed-off-by: Ahelenia Ziemiańska <[email protected]>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 4, 2021
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

The reason for this is that while the kernel provides interfaces to read/write existing files (from within the kernel), it does not provide one to remove a file. So in the case where we'd want to remove the cache file because all pools have been exported it's instead truncated to zero length. The zpool utility already treats a zero-length cache file and a missing cache file the same, so it makes good sense to apply this same reasoning to these service files.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 5, 2021
@behlendorf behlendorf requested a review from rlaager February 5, 2021 17:14
Copy link
Member

@rlaager rlaager left a comment

Choose a reason for hiding this comment

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

This looks good to me. My only concern was whether ConditionFileNotEmpty was new (in systemd). I checked that and it has been available for a long time.

@behlendorf behlendorf merged commit 7c64ee9 into openzfs:master Feb 5, 2021
behlendorf pushed a commit that referenced this pull request Feb 5, 2021
When all pools are exported ZFS will generate an empty cache file.
This will cause the import service to fail, which is sub-optimal, 
since this means that dracut fails, and it necessary to run 
`zpool import -a` to boot, delete the file, and regenerate+reinstall 
the initrd.

This resolves the issue by treating an zero-length cache files the
same as a missing cache file.  This aligns the behavior with that
of the `zpool` command itself.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #11568
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
When all pools are exported ZFS will generate an empty cache file.
This will cause the import service to fail, which is sub-optimal, 
since this means that dracut fails, and it necessary to run 
`zpool import -a` to boot, delete the file, and regenerate+reinstall 
the initrd.

This resolves the issue by treating an zero-length cache files the
same as a missing cache file.  This aligns the behavior with that
of the `zpool` command itself.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11568
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
When all pools are exported ZFS will generate an empty cache file.
This will cause the import service to fail, which is sub-optimal, 
since this means that dracut fails, and it necessary to run 
`zpool import -a` to boot, delete the file, and regenerate+reinstall 
the initrd.

This resolves the issue by treating an zero-length cache files the
same as a missing cache file.  This aligns the behavior with that
of the `zpool` command itself.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Richard Laager <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11568
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.

3 participants