Issue 2094

[ryao]

This builds on previous work by @dweeezil in #2097 and should be ready to be merged.

[dweeezil]

Other than the note I just sent, looks good to me. These instructions should be in the release notes for 0.6.3 (we do have release notes, don't we :)

[behlendorf]

These instructions should be in the release notes for 0.6.3

I'm conflicted about this point. This will never be an issue for people who have only run the tagged releases and I've only written up release notes for the tags.

(we do have release notes, don't we :)

I usually write up something when I tag. Although I/we could do a lot better and commit them to the tree.

[dweeezil]

What about adding a flag to the spa struct that would get scooped up here:

--- a/module/zfs/spa_misc.c
+++ b/module/zfs/spa_misc.c
@@ -1796,6 +1796,9 @@ spa_scan_get_stats(spa_t *spa, pool_scan_stat_t *ps)
        ps->pss_pass_start = spa->spa_scan_pass_start;
        ps->pss_pass_exam = spa->spa_scan_pass_exam;

+       /* zol scan errata flag */
+       ps->pss_zol_errata = spa->spa_scan_zol_errata;
+
        return (0);
 }

and then generate a message here:

--- a/cmd/zpool/zpool_main.c
+++ b/cmd/zpool/zpool_main.c
@@ -3909,6 +3909,13 @@ print_scan_status(pool_scan_stat_t *ps)

        assert(ps->pss_func == POOL_SCAN_SCRUB ||
            ps->pss_func == POOL_SCAN_RESILVER);
+
+       /*
+        * zol errata
+        */
+       if (ps->ps_zol_errata)
+               printf("... something about zol errata...
+
        /*
         * Scan is finished or canceled.
         */

[behlendorf]

@dweeezil I think that's a nice solution. It alleviates my concerns about an end user never knowing they need to do a scrub, and hopefully @ryao's concerns about doing it automatically. Perhaps something like this (untested):

diff --git a/cmd/zpool/zpool_main.c b/cmd/zpool/zpool_main.c
index a856fd4..9b4b904 100644
--- a/cmd/zpool/zpool_main.c
+++ b/cmd/zpool/zpool_main.c
@@ -3903,6 +3903,19 @@ print_scan_status(pool_scan_stat_t *ps)
                return;
        }

+       /*
+        * Scan required, https://github.com/zfsonlinux/zfs/issues/2094.
+        */
+       if ((ps->pss_errata & POOL_ERRATA_2094) &&
+           (ps->pss_state == DSS_FINISHED || ps->pss_state == DSS_CANCELED)) {
+               (void) printf(gettext("detected pool compatibility issue\n"));
+               (void) printf(gettext(
+                   "   see: https://github.com/zfsonlinux/zfs/issues/2094\n"));
+               (void) printf(gettext
+                   "action: To correct the issue run 'zpool scrub'.\n");
+               return;
+       }
+
        start = ps->pss_start_time;
        end = ps->pss_end_time;
        zfs_nicenum(ps->pss_processed, processed_buf, sizeof (processed_buf));

Which should show the end user a message like this when they run zpool status. This is the same format as all the other see/action messages end users may get.

  pool: rpool
 state: ONLINE
  scan: detected pool compatibility issue
   see: https://github.com/zfsonlinux/zfs/issues/2094
action: To correct the issue run 'zpool scrub'.
config:
   ...

[dweeezil]

@behlendorf Yes, that's exactly what I had in mind.

What are your thoughts on the need to handle the hopefully very rare situation where an async destroy is in progress? It seems we'd have to fail the import in that case. We could look at the new spa flag following the dsl_scan_init() call at the bottom of dsl_pool_open and bail if the earlier async destroy stuff had been triggered. Presumably just about any error we return will be propagated back as some sort of un-importable pool failure.

[behlendorf]

@dweeezil I think we shouldn't try to handle the async destroy case which I expect will be very rare.
I think we just want to ensure the import cleanly fails in that case.

For those few (if any) users who are be impacted they can just continue to run the version of the code they had until the async destroy completes. Once done they should be able to update their code run then run a scrub.

I'd prefer to avoid inventing more machinery for this than we absolutely need too.

[dweeezil]

@behlendorf I agree.

[ryao]

@behlendorf @dweeezil We can partially handle the async_destroy case by checking if the feature is activated when we detect the corrupt object in dsl_scan_init(). There is already a check there, so we simply need to hook into it. This works because the feature is deactivated whenever an async_destroy finishes.

[behlendorf]

@ryao Do you have an updated for this this? I'd like to push this over the finish line this week, I can propose the patch if your swamped.

[behlendorf]

Closed in favor of #2112