Reminder to update boot code after zpool upgrade

[grembo]

Motivation and Context

In FreeBSD there used to be a warning after upgrading a zpool, so users won't forget about updating the boot loader in case that pool is booted from.

As I've been told that this is also a problem on (at least some) Linux distributions, this patch aims to bring this warning back in a more generic form, that should apply to all systems. Update: Code review resulted in me changing the patch to only display the reminder on FreeBSD

Aims to address #12099.

See also:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256024

Description

When running zpool upgrade, check if the pool was actually touched/modified. If so, print a warning for the user not to forget to update the boot code. Re-purposed the existing printnl flag in the two affected functions.

Update: Only check on FreeBSD and show the warning only if the bootfs property is set on the pool.

How Has This Been Tested?

Manually on FreeBSD 13, by creating a couple of testpools of an older version in images files, "connecting" them using mdconfig and importing them into the system. Then run various combinations of zpool upgrade to make sure the warning only shows up when intended.

Before:

# zpool upgrade testpool1
This system supports ZFS pool feature flags.

Enabled the following features on 'testpool1':
      userobj_accounting
  encryption
  project_quota
  allocation_classes
  resilver_defer
  bookmark_v2
  redaction_bookmarks
  redacted_datasets
  bookmark_written
  log_spacemap
  livelist
  device_rebuild
  zstd_compress
  draid

Afterwards:

# zpool upgrade testpool2
This system supports ZFS pool feature flags.

Enabled the following features on 'testpool2':
  userobj_accounting
  encryption
  project_quota
  allocation_classes
  resilver_defer
  bookmark_v2
  redaction_bookmarks
  redacted_datasets
  bookmark_written
  log_spacemap
  livelist
  device_rebuild
  zstd_compress
  draid

Pool 'testpool2' has the bootfs property set, you might need to update
the boot code. See gptzfsboot(8) and loader.efi(8) for details.

Also works when upgrading all pools (-a):

# zpool upgrade -a
This system supports ZFS pool feature flags.

Enabled the following features on 'testpool3':
  userobj_accounting
  encryption
  project_quota
  allocation_classes
  resilver_defer
  bookmark_v2
  redaction_bookmarks
  redacted_datasets
  bookmark_written
  log_spacemap
  livelist
  device_rebuild
  zstd_compress
  draid

Pool 'testpool3' has the bootfs property set, you might need to update
the boot code. See gptzfsboot(8) and loader.efi(8) for details.
Enabled the following features on 'testpool4':
  userobj_accounting
  encryption
  project_quota
  allocation_classes
  resilver_defer
  bookmark_v2
  redaction_bookmarks
  redacted_datasets
  bookmark_written
  log_spacemap
  livelist
  device_rebuild
  zstd_compress
  draid

Pool 'testpool4' has the bootfs property set, you might need to update
the boot code. See gptzfsboot(8) and loader.efi(8) for details.

And stays silent in case nothing has been upgraded:

# ./zpool upgrade -a
This system supports ZFS pool feature flags.

All pools are already formatted using feature flags.

Every feature flags pool already has all supported features enabled.

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 have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[rincebrain]

The problem with displaying this message on Linux is that, often, there's not an update available, you just shouldn't enable those features on a pool you boot from.

[grembo]

The problem with displaying this message on Linux is that, often, there's not an update available, you just shouldn't enable those features on a pool you boot from.

In #12099 @allanjude suggested to only show the message if bootfs is set on the pool. Technically this is not a requirement, but in practice it might be good enough.

[rincebrain]

The problem with displaying this message on Linux is that, often, there's not an update available, you just shouldn't enable those features on a pool you boot from.

In #12099 @allanjude suggested to only show the message if bootfs is set on the pool. Technically this is not a requirement, but in practice it might be good enough.

You may find the discussion around #8627 relevant.

[grembo]

The problem with displaying this message on Linux is that, often, there's not an update available, you just shouldn't enable those features on a pool you boot from.

In #12099 @allanjude suggested to only show the message if bootfs is set on the pool. Technically this is not a requirement, but in practice it might be good enough.

You may find the discussion around #8627 relevant.

Technically, the FreeBSD boot loader doesn't need bootfs either. In general, it seems like this is more important to FreeBSD users, where it's quite common to have a single zroot pool and a boot loader that is updated on every release, than it is to Linux users. So in that respect, this could easily be solved by using #ifdef __FreeBSD__ (and then give more detailed instructions, e.g., "please see 'man zfsboot'"), while Linux simple outputs a newline character as before. I'm just not sure how happy the openzfs project is about having #ifdefs around for such things.

[rincebrain]

The problem with displaying this message on Linux is that, often, there's not an update available, you just shouldn't enable those features on a pool you boot from.

In #12099 @allanjude suggested to only show the message if bootfs is set on the pool. Technically this is not a requirement, but in practice it might be good enough.

You may find the discussion around #8627 relevant.

Technically, the FreeBSD boot loader doesn't need bootfs either. In general, it seems like this is more important to FreeBSD users, where it's quite common to have a single zroot pool and a boot loader that is updated on every release, than it is to Linux users. So in that respect, this could easily be solved by using #ifdef __FreeBSD__ (and then give more detailed instructions, e.g., "please see 'man zfsboot'"), while Linux simple outputs a newline character as before. I'm just not sure how happy the openzfs project is about having #ifdefs around for such things.

Honestly, I'd prefer it if there were a nice way to detect and warn people trying to run unconstrained zpool upgrade on bpools, but that seems infeasible to detect reliably.

Failing that, I'd prefer it if zpool upgrade would warn you about this in general before letting you do it (especially since zpool status suggests people run it by default in the field where it normally reports actual problems...), but I think the ship of "zpool upgrade can burn your house down" has long sailed.

Given the above, and that I doubt a patch to warn and exit (or even wait a few seconds without passing a flag) would be accepted, I think just not printing it on Linux probably makes the most sense, since "hey btw you may have just ruined your system boot irreversibly (without remaking the pool)" seems unhelpful at best and cruel at worst.

[grembo]

Given the above, and that I doubt a patch to warn and exit (or even wait a few seconds without passing a flag) would be accepted, I think just not printing it on Linux probably makes the most sense, since "hey btw you may have just ruined your system boot irreversibly (without remaking the pool)" seems unhelpful at best and cruel at worst.

@rincebrain Thanks for making me smile :)

I updated the patch, so the warning only shows up on FreeBSD. Being OS specific, it now also points the user to the two most relevant man pages on how to accomplish updating the boot loader.

[nabijaczleweli]

Isn't that the scenario the compatibility= zpool property is supposed to prevent, anyway?

[rincebrain]

Isn't that the scenario the compatibility= zpool property is supposed to prevent, anyway?

That only helps once people are running 2.1 and, for historical users, if they explicitly go set it on their bpool.

[grembo]

Note: buildbot/FreeBSD main amd64 (TEST) seems to have an unrelated problem at the moment. Checkstyle pending, since I cleaned the commit message yesterday (squashed + shortened + signed-by + force push).

[ghost]

The two places you've added this message are identical, would you mind factoring that out into one static helper function please? That way we can just ifdef freebsd the body of the function, reducing the number of ifdefs we have to add. Something like

static void
show_upgrade_warnings(zpool_handle_t *zhp)
{
#ifdef __FreeBSD__
        char bootfs[ZPOOL_MAXPROPLEN];
        if (zpool_get_prop(zhp, ZPOOL_PROP_BOOTFS, bootfs,
            sizeof (bootfs), NULL, B_FALSE) == 0 &&
            strcmp(bootfs, "-") != 0) {
                (void) printf(gettext("\nPool '%s' has the bootfs "
                    "property set, you might need to update\nthe boot "
                    "code. See gptzfsboot(8) and loader.efi(8) for "
                    "details.\n"), zpool_get_name(zhp));
        }
#else
        (void) printf("\n");
#endif
}

[ikozhukhov]

or move this print per platform because we'd like to do the same for DilOS and add defined(__dilos__) will no good one in this place

[grembo]

The two places you've added this message are identical, would you mind factoring that out into one static helper function please? That way we can just ifdef freebsd the body of the function, reducing the number of ifdefs we have to add. Something like

I thought about this too, but didn't do it yet, as it seemed like those two functions are kept completely separate on purpose, but are overlapping in functionality a lot. If you diff them, you wonder if it would make more sense to have a static helper function to perform most of what they do (and get rid of the duplicate #ifdef that way):

--- /tmp/upgrade_one	2021-05-26 16:10:12.476405000 +0200
+++ /tmp/upgrade_cb	2021-05-26 16:10:27.861114000 +0200
@@ -1,52 +1,44 @@
 static int
-upgrade_one(zpool_handle_t *zhp, void *data)
+upgrade_cb(zpool_handle_t *zhp, void *arg)
 {
+	upgrade_cbdata_t *cbp = arg;
+	nvlist_t *config;
+	uint64_t version;
 	boolean_t modified_pool = B_FALSE;
-	upgrade_cbdata_t *cbp = data;
-	uint64_t cur_version;
 	int ret;
 
-	if (strcmp("log", zpool_get_name(zhp)) == 0) {
-		(void) fprintf(stderr, gettext("'log' is now a reserved word\n"
-		    "Pool 'log' must be renamed using export and import"
-		    " to upgrade.\n"));
-		return (1);
-	}
-
-	cur_version = zpool_get_prop_int(zhp, ZPOOL_PROP_VERSION, NULL);
-	if (cur_version > cbp->cb_version) {
-		(void) printf(gettext("Pool '%s' is already formatted "
-		    "using more current version '%llu'.\n\n"),
-		    zpool_get_name(zhp), (u_longlong_t)cur_version);
-		return (0);
-	}
+	config = zpool_get_config(zhp, NULL);
+	verify(nvlist_lookup_uint64(config, ZPOOL_CONFIG_VERSION,
+	    &version) == 0);
 
-	if (cbp->cb_version != SPA_VERSION && cur_version == cbp->cb_version) {
-		(void) printf(gettext("Pool '%s' is already formatted "
-		    "using version %llu.\n\n"), zpool_get_name(zhp),
-		    (u_longlong_t)cbp->cb_version);
-		return (0);
-	}
+	assert(SPA_VERSION_IS_SUPPORTED(version));
 
-	if (cur_version != cbp->cb_version) {
-		modified_pool = B_TRUE;
+	if (version < cbp->cb_version) {
+		cbp->cb_first = B_FALSE;
 		ret = upgrade_version(zhp, cbp->cb_version);
 		if (ret != 0)
 			return (ret);
+		modified_pool = B_TRUE;
+
+		/*
+		 * If they did "zpool upgrade -a", then we could
+		 * be doing ioctls to different pools.  We need
+		 * to log this history once to each pool, and bypass
+		 * the normal history logging that happens in main().
+		 */
+		(void) zpool_log_history(g_zfs, history_str);
+		log_history = B_FALSE;
 	}
 
 	if (cbp->cb_version >= SPA_VERSION_FEATURES) {
-		int count = 0;
+		int count;
 		ret = upgrade_enable_all(zhp, &count);
 		if (ret != 0)
 			return (ret);
 
-		if (count != 0) {
+		if (count > 0) {
+			cbp->cb_first = B_FALSE;
 			modified_pool = B_TRUE;
-		} else if (cur_version == SPA_VERSION) {
-			(void) printf(gettext("Pool '%s' already has all "
-			    "supported and requested features enabled.\n"),
-			    zpool_get_name(zhp));
 		}
 	}

Another question is, if reading show_upgrade_warnings in the code might mislead readers into thinking that there will always be a warning shown, even though it won't be on linux and it's conditional on FreeBSD, but that's more of a question of naming the function clearly (post_upgrade_checks(pool) maybe).

[grembo]

or move this print per platform because we'd like to do the same for DilOS and add defined(__dilos__) will no good one in this place

@ikozhukhov @freqlabs Well, it's not just a print, but also a check. I also noticed that my code doesn't println("\n") in case the pool wasn't modified (but I have to touch it again anyway).

What about this logic in the functions in zpool_main.c and then move after_zpool_upgrade_check to some OS specific function:

	if (modified_pool) {
		after_zpool_upgrade_check(zhp);
		println("\n");
	}

This would broaden the scope of the review a lot though.
Update: Changed "post" to "after", as "post" can easily be misunderstood as a verb.

[ghost]

Sure, the name was just the first thing that popped into my head. Factoring even more out seems a bit more cumbersome, I'm not sure if that's worth it.

[ghost]

or move this print per platform because we'd like to do the same for DilOS and add defined(__dilos__) will no good one in this place

I forgot we had platform specific subdirectories in cmd/zpool/os, that's definitely the way to go.

[grembo]

or move this print per platform because we'd like to do the same for DilOS and add defined(__dilos__) will no good one in this place

I forgot we had platform specific subdirectories in cmd/zpool/os, that's definitely the way to go.

I moved this to os specific files now.

Open question:

• Better msg buffer size (MAXPATHLEN is meh)?
• Maybe create new source file (os/platform/zpool_main_os.c)? (I just popped it into zpool_vdev_os.c for now)

[grembo]

@freqlabs Thank you for taking the time to review.

Is there anything else I should do (e.g., squash and force-push), or will all of this be handled when (and if) the pull request is merged?

[ghost]

@freqlabs Thank you for taking the time to review.

Is there anything else I should do (e.g., squash and force-push), or will all of this be handled when (and if) the pull request is merged?

Yes, please do squash to a single commit. Thank you for your contribution!

[grembo]

zloop/tests failure seems unrelated:

exiting... max 1 allowed cores
Error: Process completed with exit code 1.

[grembo]

@freqlabs On the FreeBSD-current mailing list, @bsdimp writes:

To get into 13, it will need to be merged into their stable branch...

What is the best way to approach this without causing a lot of extra effort (given, that this change is *bsd only)?

[ghost]

@grembo I expect @behlendorf will pull this into the zfs-2.1-release branch, which is what we'll be pulling into FreeBSD stable/13 and eventually releng when the processes for that are ready.

[bsdimp]

I'm trying to get the final few items into place...

[grembo]

@freqlabs @bsdimp Cool, thanks for your quick responses.