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.

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

Jump to bottom

Set spa_ccw_fail_time=0 when expanding a vdev. #15405

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Set spa_ccw_fail_time=0 when expanding a vdev. #15405

merged 1 commit into from
Oct 20, 2023

Conversation

cperciva
Copy link
Contributor

When a vdev is to be expanded -- either via zpool online -e or via the autoexpand option -- a SPA_ASYNC_CONFIG_UPDATE request is queued to be handled via an asynchronous worker thread (spa_async_thread). This normally happens almost immediately; but will be delayed up to zfs_ccw_retry_interval seconds (default 5 minutes) if an attempt to write the zpool configuration cache failed.

When FreeBSD boots ZFS-root VM images generated using makefs -t zfs, the zpoolupgrade rc.d script runs zpool upgrade, which modifies the pool configuration and triggers an attempt to write to the cache file. This attempted write fails because the filesystem is still mounted read-only at this point in the boot process, triggering a 5-minute cooldown before SPA_ASYNC_CONFIG_UPDATE requests will be handled by the asynchronous worker thread.

When expanding a vdev, reset the "when did a configuration cache write last fail" value so that the SPA_ASYNC_CONFIG_UPDATE request will be handled promptly. A cleaner but more intrusive option would be to use separate SPA_ASYNC_ flags for "configuration changed" and "try writing the configuration cache again", but with FreeBSD 14.0 coming very soon I'd prefer to leave such refactoring for a later date.

Motivation and Context

FreeBSD ZFS EC2 AMIs take 5 minutes before expanding their filesystems when they first boot. This fixes that.

Description

Resets spa_ccw_fail_time to zero when we want to expand the filesystem, in order to bypass the normal 5 minute SPA_ASYNC_CONFIG_UPDATE cooldown.

How Has This Been Tested?

Tested in FreeBSD/EC2. Filesystem resize is now more or less immediate when zpool online -e runs.

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:

@cperciva
Set spa_ccw_fail_time=0 when expanding a vdev.
d9088aa 
When a vdev is to be expanded -- either via `zpool online -e` or via
the autoexpand option -- a SPA_ASYNC_CONFIG_UPDATE request is queued
to be handled via an asynchronous worker thread (spa_async_thread).
This normally happens almost immediately; but will be delayed up to
zfs_ccw_retry_interval seconds (default 5 minutes) if an attempt to
write the zpool configuration cache failed.

When FreeBSD boots ZFS-root VM images generated using `makefs -t zfs`,
the zpoolupgrade rc.d script runs `zpool upgrade`, which modifies the
pool configuration and triggers an attempt to write to the cache file.
This attempted write fails because the filesystem is still mounted
read-only at this point in the boot process, triggering a 5-minute
cooldown before SPA_ASYNC_CONFIG_UPDATE requests will be handled by
the asynchronous worker thread.

When expanding a vdev, reset the "when did a configuration cache
write last fail" value so that the SPA_ASYNC_CONFIG_UPDATE request
will be handled promptly.  A cleaner but more intrusive option would
be to use separate SPA_ASYNC_ flags for "configuration changed" and
"try writing the configuration cache again", but with FreeBSD 14.0
coming very soon I'd prefer to leave such refactoring for a later
date.

Signed-off-by: Colin Percival <cperciva@FreeBSD.org>
@amotin
Copy link
Member

amotin commented Oct 14, 2023

I have no objections, but wonder if we should do the same for all other spa_async_request(spa, SPA_ASYNC_CONFIG_UPDATE) calls beside the one in spa_write_cachefile().

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Oct 14, 2023
@cperciva
Copy link
Contributor Author

I have no objections, but wonder if we should do the same for all other spa_async_request(spa, SPA_ASYNC_CONFIG_UPDATE) calls beside the one in spa_write_cachefile().

Maybe? I'm not saying this is necessarily the best fix, just that it fixes a serious problem which 14.0 is tripping over and I want to get it into FreeBSD before the release. ;-)

@amotin
Copy link
Member

amotin commented Oct 15, 2023

I have no objections, but wonder if we should do the same for all other spa_async_request(spa, SPA_ASYNC_CONFIG_UPDATE) calls beside the one in spa_write_cachefile().

Maybe?

As I see it, the time is set when cache file write fails to not retry it constantly. But I see no problem to retry it on any explicit user activity, not only expanding. Though I may be wrong about some of the delay motivations.

@cperciva
Copy link
Contributor Author

As I see it, the time is set when cache file write fails to not retry it constantly. But I see no problem to retry it on any explicit user activity, not only expanding. Though I may be wrong about some of the delay motivations.

I think you're probably right, but I don't know enough about this code to be certain. So I wanted to be conservative since there's only a few weeks left before FreeBSD 14.0-RELEASE.

@cperciva
Copy link
Contributor Author

Ping?

amotin
amotin approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I would still go further and changed all the places except the error handling.

@cperciva
Copy link
Contributor Author

Can someone please merge this so we can get it into FreeBSD 14.0?

@behlendorf behlendorf merged commit ea30b5a into openzfs:master Oct 20, 2023
25 of 26 checks passed
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 20, 2023
@cperciva
Copy link
Contributor Author

Thank you!

ixhamza pushed a commit to truenas/zfs that referenced this pull request Nov 20, 2023
@cperciva @tonyhutter
Set spa_ccw_fail_time=0 when expanding a vdev.
1cc1bf4 
When a vdev is to be expanded -- either via `zpool online -e` or via
the autoexpand option -- a SPA_ASYNC_CONFIG_UPDATE request is queued
to be handled via an asynchronous worker thread (spa_async_thread).
This normally happens almost immediately; but will be delayed up to
zfs_ccw_retry_interval seconds (default 5 minutes) if an attempt to
write the zpool configuration cache failed.

When FreeBSD boots ZFS-root VM images generated using `makefs -t zfs`,
the zpoolupgrade rc.d script runs `zpool upgrade`, which modifies the
pool configuration and triggers an attempt to write to the cache file.
This attempted write fails because the filesystem is still mounted
read-only at this point in the boot process, triggering a 5-minute
cooldown before SPA_ASYNC_CONFIG_UPDATE requests will be handled by
the asynchronous worker thread.

When expanding a vdev, reset the "when did a configuration cache
write last fail" value so that the SPA_ASYNC_CONFIG_UPDATE request
will be handled promptly.  A cleaner but more intrusive option would
be to use separate SPA_ASYNC_ flags for "configuration changed" and
"try writing the configuration cache again", but with FreeBSD 14.0
coming very soon I'd prefer to leave such refactoring for a later
date.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Colin Percival <cperciva@FreeBSD.org>
Closes openzfs#15405
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@cperciva @lundman
Set spa_ccw_fail_time=0 when expanding a vdev.
cb708d1 
When a vdev is to be expanded -- either via `zpool online -e` or via
the autoexpand option -- a SPA_ASYNC_CONFIG_UPDATE request is queued
to be handled via an asynchronous worker thread (spa_async_thread).
This normally happens almost immediately; but will be delayed up to
zfs_ccw_retry_interval seconds (default 5 minutes) if an attempt to
write the zpool configuration cache failed.

When FreeBSD boots ZFS-root VM images generated using `makefs -t zfs`,
the zpoolupgrade rc.d script runs `zpool upgrade`, which modifies the
pool configuration and triggers an attempt to write to the cache file.
This attempted write fails because the filesystem is still mounted
read-only at this point in the boot process, triggering a 5-minute
cooldown before SPA_ASYNC_CONFIG_UPDATE requests will be handled by
the asynchronous worker thread.

When expanding a vdev, reset the "when did a configuration cache
write last fail" value so that the SPA_ASYNC_CONFIG_UPDATE request
will be handled promptly.  A cleaner but more intrusive option would
be to use separate SPA_ASYNC_ flags for "configuration changed" and
"try writing the configuration cache again", but with FreeBSD 14.0
coming very soon I'd prefer to leave such refactoring for a later
date.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Colin Percival <cperciva@FreeBSD.org>
Closes openzfs#15405
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@amotin amotin amotin approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cperciva @amotin @behlendorf