zvol_disk_open() may spin on CPU

[ahrens]

Motivation and Context

zvol_disk_open() waits for up to zfs_vdev_open_timeout_ms (1 second by default) (e.g. if the block device does not exist). While in this loop, it calls schedule_timeout().

Description

The problem is that schedule_timeout() may not actually cause the thread to go off-CPU. Per the "documentation" (comment in the source code):

 * The function behavior depends on the current task state:
 * %TASK_RUNNING - the scheduler is called, but the task does not sleep
 * at all. That happens because sched_submit_work() does nothing for
 * tasks in %TASK_RUNNING state.

In my experience, schedule_timeout() never sleeps from this code path. This is especially noticeable if zfs_vdev_open_timeout_ms has been increased from its default.

This commit uses msleep() to actually sleep. Note that this is how it was before #7629.

How Has This Been Tested?

increasing zfs_vdev_open_timeout_ms and then importing a pool whose disks are no longer present on the system (/dev/... links no longer exist)

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.

[ahrens]

@behlendorf do you know why it was changed from msleep() to schedule_timeout() in #7629? Will changing it back cause any problems?

[ahrens]

Should this use delay() instead (which uses schedule_timeout_uninterruptible())?

[behlendorf]

I don't recall why this was changed. My hunch would be to reduce the number of kernel interfaces we depend on. I see msleep() isn't used anywhere else in the Linux code. It's unlikely msleep() would change, but you never know.

If we do want to use msleep() here we'll want to add a ./configure test for it. My preference would be to use the native schedule_timeout_uninterruptible() interface here. This is in the Linux specific code so that's not an issue, it avoids adding a new dependency, and it makes it crystal clear how we intended to sleep.