zpool import progress indicator

[ofaaland]

Motivation and Context

When an import requires a long MMP activity check, or when the user
requests pool recovery, the import make take a long time. The user may
not know why, or be able to tell whether the import is progressing or is
hung.

Description

A previous commit added a kstat which lists all imports currently
being processed by the
kernel (currently only one at a time is possible, but the kstat allows
for more than one). The kstat is at
/proc/spl/kstat/zfs/import_progress.

In the zpool utility, create an additional thread which periodically
checks the contents of this kstat. If the import does not finish
quickly, it reports the following on stderr:

• Name and guid of pool being imported
• User-friendly version of spa_load_state
• spa_load_max_txg

If one or more of these values change, the thread prints a new record.

A new record will be printed to stderr with a maximum frequency of one
per second so as not to spam the user. As a result the printed output
may reflect only some of the import states transitioned through.

Sample output:

Pool tank1 (guid 4591991398949870326):
Checking for a remote import.
Check will complete by Tue Apr 16 08:43:58 2019

Pool tank1 (guid 4591991398949870326):
Checking for a remote import.
Check will complete by Tue Apr 16 08:44:43 2019
Recovering Pool max txg 745

How Has This Been Tested?

Hand testing with and without multihost check triggered, and with and without zpool -FX -T arguments.

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)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.

[richardelling]

The idea is good. But I’m not sure about stderr. It seems more consistent to have a zpool import —verbose option and print to stdout.

Perhaps some of the other automation and appliance builders can comment on how this might affect the automation and reporting?

[ofaaland]

@richardelling thanks for the feedback.

[codecov]

Codecov Report

Merging #8646 (6555737) into master (161ed82) will increase coverage by 4.49%.
The diff coverage is 9.09%.

❗ Current head 6555737 differs from pull request most recent head c7b9aa8. Consider uploading reports for the commit c7b9aa8 to get more accurate results

@@            Coverage Diff             @@
##           master    #8646      +/-   ##
==========================================
+ Coverage   75.17%   79.67%   +4.49%     
==========================================
  Files         402      394       -8     
  Lines      128071   124719    -3352     
==========================================
+ Hits        96283    99372    +3089     
+ Misses      31788    25347    -6441     

Flag	Coverage Δ
kernel	80.39% <ø> (+1.63%)	⬆️
user	65.68% <9.09%> (+18.25%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/zpool/zpool_main.c	80.12% <9.09%> (-0.70%)	⬇️
include/os/linux/zfs/sys/trace_acl.h	33.33% <0.00%> (-33.34%)	⬇️
cmd/mount_zfs/mount_zfs.c	42.85% <0.00%> (-20.91%)	⬇️
cmd/zed/zed_file.c	33.80% <0.00%> (-8.42%)	⬇️
cmd/zed/zed_exec.c	82.92% <0.00%> (-7.49%)	⬇️
cmd/zvol_id/zvol_id_main.c	76.31% <0.00%> (-5.27%)	⬇️
lib/libzfs/os/linux/libzfs_mount_os.c	68.86% <0.00%> (-4.97%)	⬇️
module/zfs/vdev_missing.c	60.00% <0.00%> (-3.64%)	⬇️
module/zfs/abd.c	91.61% <0.00%> (-3.36%)	⬇️
cmd/raidz_test/raidz_test.c	79.47% <0.00%> (-3.15%)	⬇️
... and 259 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec64fdb...c7b9aa8. Read the comment docs.

[ahrens]

@ofaaland This seems like a nice enhancement to zpool import. I see that it's marked "work in progress". What additional work needs to be done to get it ready to review and integrate?

[ofaaland]

I think I might just need to rebase it. It's been a while as you see. The original version implemented both the kstat and the utility changes, but then IIRC I broke that into two parts, and Brian approved the kstat.

…

-Olaf

________________________________________ From: Matthew Ahrens ***@***.***> Sent: Friday, June 4, 2021 4:46 PM To: openzfs/zfs Cc: Faaland, Olaf P.; Mention Subject: Re: [openzfs/zfs] zpool import progress indicator [WIP] (#8646) @ofaaland<https://urldefense.us/v3/__https://github.com/ofaaland__;!!G2kpM7uM-TzIFchu!kp599p1GtNr5Xd_iAWTXvS5dQnR3XZMRlEYcRuvg0aP7F5cbjggKmDelpt1TyVaF7w$> This seems like a nice enhancement to zpool import. I see that it's marked "work in progress". What additional work needs to be done to get it ready to review and integrate? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.us/v3/__https://github.com/openzfs/zfs/pull/8646*issuecomment-855129636__;Iw!!G2kpM7uM-TzIFchu!kp599p1GtNr5Xd_iAWTXvS5dQnR3XZMRlEYcRuvg0aP7F5cbjggKmDelpt2cMTH5EQ$>, or unsubscribe<https://urldefense.us/v3/__https://github.com/notifications/unsubscribe-auth/AB73C4ZSUK2B2ETWSBDR3NLTRFQTXANCNFSM4HHI7AWA__;!!G2kpM7uM-TzIFchu!kp599p1GtNr5Xd_iAWTXvS5dQnR3XZMRlEYcRuvg0aP7F5cbjggKmDelpt3LMB_q8Q$>.

[ofaaland]

@ofaaland This seems like a nice enhancement to zpool import. I see that it's marked "work in progress". What additional work needs to be done to get it ready to review and integrate?

I confirmed this PR is just for the utility changes. The rebase was fairly clean so it might still work fine (haven't tested yet). I haven't written tests for the utility changes yet. I'll work on that - it would be nice to get this done.

[ofaaland]

Hi, before reviewing this for detail stuff, I have a question.

I've altered the existing MMP tests to use the output of zpool import -v so that the the multihost related output has some test coverage. I don't currently have any test coverage for the TXG rewind portion of the output because I don't see an existing way to make the import slow other than forcing a MMP activity check. As a consequence, the import would complete (or fail) before my TXG rewind related output would appear.

Options I've come up with are:

1. Create a new test that sets up an import which requires an MMP check and does a TXG rewind. Then at least I can confirm the rewind-related text appears.
2. Create a mechanism to artificially slow down the import, with a sleep() somewhere in zutil_import.c triggered by the presence of a special environment variable
3. Create a mechanism to artificially slow down the import, in the kernel, triggered by an undocumented module option or a new zinject mechanism.

I don't really think options 2 or 3 really add much, although the 3 would allow the test to check which TXG numbers reported are reasonable.

Do you have any thoughts? Thanks

[ofaaland]

@freqlabs @nabijaczleweli @asomers I have a linux/freebsd compat question and see you've contributed several freebsd-related patches.

My patch needs to read a kstat. On linux that's done with something like fd = open("/proc/spl/kstat/<kstat-name>", O_RDONLY); read(fd, buf, bufsize); .

Googling around it looks like kstats are accessible on freebsd systems under /dev/kstat/ and there's a family of functions kstat_open(), kstat_read(), etc. for that purpose. I looked quite a bit and find no place in the existing ZFS code where a C function reads a kstat to use as a model. What's the right way? Is there already a mechanism for abstracting this in the ZFS code that I overlooked? Thank you.

[asomers]

@freqlabs @nabijaczleweli @asomers I have a linux/freebsd compat question and see you've contributed several freebsd-related patches.

My patch needs to read a kstat. On linux that's done with something like fd = open("/proc/spl/kstat/<kstat-name>", O_RDONLY); read(fd, buf, bufsize); .

Googling around it looks like kstats are accessible on freebsd systems under /dev/kstat/ and there's a family of functions kstat_open(), kstat_read(), etc. for that purpose. I looked quite a bit and find no place in the existing ZFS code where a C function reads a kstat to use as a model. What's the right way? Is there already a mechanism for abstracting this in the ZFS code that I overlooked? Thank you.

There is no /dev/kstat, kstat_open, kstat_read on FreeBSD. Instead, use the sysctl interface. From sh, do something like sysctl kstat.zfs.zroot.dataset.objset-0x7923.reads. Or from C, use sysctlbyname(3).

[ofaaland]

There is no /dev/kstat, kstat_open, kstat_read on FreeBSD. Instead, use the sysctl interface. From sh, do something like sysctl kstat.zfs.zroot.dataset.objset-0x7923.reads. Or from C, use sysctlbyname(3).

Awesome, thank you

[ghost]

The sysctls on FreeBSD are typed, they're not all text like the kstat files on Linux. That throws a bit of a monkey wrench into the kstat_read interface. It will work for the import_progress kstat because we happened to reuse most of what Linux was doing for that instead of having separate properly typed fields, but more generally we don't expose kstats as text. Briefly looking over kstat_read(3) and kstat(3kstat) manuals for illumos, it seems to be a similar situation there. Linux is the oddball in exposing kstats as strings of text.
The kstats are also organized differently on FreeBSD, so import_progress is in kstat.zfs.misc.import_progress rather than kstat.zfs.import_progress, among other things. Here's the structure and types of kstat.zfs sysctl nodes on a system with two pools "storage" and "system-nvme" imported:
https://gist.github.com/freqlabs/0039bd94c498b17c39031a9311e25928

[ofaaland]

Thanks, @freqlabs

[ghost]

To move this forward I think it makes sense to abandon the idea of a general kstat_read library function and make it something like read_import_progress so it's clear this function isn't a general purpose kstat reader. A real implementation of kstat_read is tricky enough to be outside the scope of this work IMO.

[ofaaland]

@freqlabs,

it makes sense to abandon the idea of a general kstat_read library function and make it something like read_import_progress so it's clear this function isn't a general purpose kstat reader

I agree, I'll do that. Thanks for the information.

[amotin]

This is mostly replaced by more detailed #15539 & Co.