Add a diagnostic kstat for obtaining pool status

[ihoro]

Motivation and Context

A hung pool process can be left holding the spa config lock or the spa namespace lock. If an admin wants to observe the status of a pool using the traditional zpool status, it could hang waiting for one of the locks held by the stuck process. It would be nice to observe pool status in this scenario without the risk of the inquiry hanging.

This PR is an aggregated and updated version of #16026 and #16484.

Description

This change adds /proc/spl/kstat/zfs/<poolname>/status_json.

This kstat output does not require taking the spa_namespace lock, as in the case for 'zpool status'. It can be used for investigations when pools are in a hung state while holding global locks required for a traditional 'zpool status' to proceed.

The newly introduced zfs_lockless_read_enabled module parameter enables traversal of related kernel structures in cases where the required config read locks cannot be taken.

When zfs_lockess_read_enabled is set, this kstat is not safe to use in conditions where pools are in the process of configuration changes (i.e., adding/removing devices).

The idea is to follow zpool status -jp output as much as possible:

# sysctl -n kstat.zfs.p2.misc.status_json | jq
{
  "output_version": {
    "command": "kstat zpool status",
    "vers_major": 0,
    "vers_minor": 1
  },
  "pools": {
    "p2": {
      "name": "p2",
      "state": "ONLINE",
      "pool_guid": "16174423138529437443",
      "txg": "4",
      "spa_version": "5000",
      "zpl_version": "5",
      "vdevs": {
        "p2": {
          "name": "p2",
          "vdev_type": "root",
          "guid": "16174423138529437443",
          "class": "normal",
          "state": "ONLINE",
          "alloc_space": "116736",
          "total_space": "134217728",
          "def_space": "134217728",
          "read_errors": "0",
          "write_errors": "0",
          "checksum_errors": "0",
          "vdevs": {
            "/root/tmp/p2/file1": {
              "name": "/root/tmp/p2/file1",
              "vdev_type": "file",
              "guid": "542797111474461141",
              "path": "/root/tmp/p2/file1",
              "class": "normal",
              "state": "ONLINE",
              "alloc_space": "59392",
              "total_space": "67108864",
              "def_space": "67108864",
              "rep_dev_size": "71827456",
              "phys_space": "83886080",
              "read_errors": "0",
              "write_errors": "0",
              "checksum_errors": "0",
              "slow_ios": "0"
            },
            "/root/tmp/p2/file2📌": {
              "name": "/root/tmp/p2/file2📌",
              "vdev_type": "file",
              "guid": "13616452074869266915",
              "path": "/root/tmp/p2/file2📌",
              "class": "normal",
              "state": "ONLINE",
              "alloc_space": "57344",
              "total_space": "67108864",
              "def_space": "67108864",
              "rep_dev_size": "71827456",
              "phys_space": "83886080",
              "read_errors": "0",
              "write_errors": "0",
              "checksum_errors": "0",
              "slow_ios": "0"
            }
          }
        }
      },
      "error_count": "0"
    }
  }
}

How Has This Been Tested?

Added new pool_status_json.ksh atuomated test which compares the output with zpool status -jp.

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.

[gmelikov]

Don't mean to oppose this PR somehow, but just wanted to highlight for anybody passing by that there's already a method to get just a pool's status without locks via cat /proc/spl/kstat/zfs/POOLNAME/state.

[allanjude]

Don't mean to oppose this PR somehow, but just wanted to highlight for anybody passing by that there's already a method to get just a pool's status without locks via cat /proc/spl/kstat/zfs/POOLNAME/state.

for clarity, that returns: ONLINE or DEGRADED

Not something analogous to zpool status, that tells you about EACH vdev

[tonyhutter]

I think having a module param to disable locks for these rare debug situations is a valid use case. If we go down that path though, I think there's potentially more benefit in disabling locks for the userspace commands rather than a using a kstat. That would let us use the familiar zpool status with all its nice options and formatting (and other commands). It would also be less code churn than the kstat route.

I have a rough prototype here that seems to work (lightly tested only!):
tonyhutter@a729b84

# Disable locks on 'tank'
echo -n tank > /sys/module/zfs/parameters/zfs_debug_skip_locks_on_pool

# Disable locks on all pools
echo -n '*' > /sys/module/zfs/parameters/zfs_debug_skip_locks_on_pool

zpool status
zpool status -j
...

The prototype is slightly more dangerous than the kstat version since it does traverse the spa_namespace avl tree (locklessly). But the odds that the tree changes out from under you while running zpool is very extremely low, and when it does, it would be doing so during a time that would risk using the kstat as well (like an export).

Thoughts?

[allanjude]

I think having a module param to disable locks for these rare debug situations is a valid use case. If we go down that path though, I think there's potentially more benefit in disabling locks for the userspace commands rather than a using a kstat. That would let us use the familiar zpool status with all its nice options and formatting (and other commands). It would also be less code churn than the kstat route.

I have a rough prototype here that seems to work (lightly tested only!): tonyhutter@a729b84

# Disable locks on 'tank'
echo -n tank > /sys/module/zfs/parameters/zfs_debug_skip_locks_on_pool

# Disable locks on all pools
echo -n '*' > /sys/module/zfs/parameters/zfs_debug_skip_locks_on_pool

zpool status
zpool status -j
...

The prototype is slightly more dangerous than the kstat version since it does traverse the spa_namespace avl tree (locklessly). But the odds that the tree changes out from under you while running zpool is very extremely low, and when it does, it would be doing so during a time that would risk using the kstat as well (like an export).

Thoughts?

So the lock we are overriding with a tunable here is the pool specific config lock.

The lock we avoid using the kstat is the all-pools spa namespace lock.

I am not sure it is very practical or safe to do zpool status without locks

[tonyhutter]

So the lock we are overriding with a tunable here is the pool specific config lock.

The lock we avoid using the kstat is the all-pools spa namespace lock.

The zpool status prototype also skips the spa_namespace lock (see spa_open_common() changes).

I am not sure it is very practical or safe to do zpool status without locks

For normal operation, zpool status would still use the locks.

With locks disabled, the safety would pretty much the same for both lockless kstat and zpool status. For both, you could crash or have undefined results if you modified/freed the target spa_t while running the query. For the zpool status path, there's a tiny bit more risk, as you could also panic if ZFS is updating the spa_namespace_avl tree, but those windows of time are going to be extremely tiny. This tiny risk could even be removed by placing an additional RW lock bracketing all add/remove/query calls on spa_namespace_avl (I ran out of time to put this in my prototype).