Skip to content

zvol: Enable zvol threading functionality on FreeBSD #17169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 8, 2025

Conversation

fuporovvStack
Copy link
Contributor

@fuporovvStack fuporovvStack commented Mar 22, 2025

Motivation and Context

Add zvol IO taskqs support on FreeBSD side. Make zvol threadpool creation/destruction logic and zvol module parameters shared for both Linux and FreeBSD.

Description

Make zvol I/O requests processing asynchronous on FreeBSD side in some cases. Clone zvol threading logic and required module parameters from Linux side. Make zvol threadpool creation/destruction logic shared for both Linux and FreeBSD.
The IO requests are processed asynchronously in next cases:

  • volmode=geom: if IO request passed thru zvol_geom_worker thread.
  • volmode=cdev: if IO request passed thru struct cdevsw .d_strategy routine, mean is AIO request.

In all other cases the IO requests are processed synchronously.

How Has This Been Tested?

The zvol_request_sync module parameter manipulation was added to tests/functional/zvol/zvol_stress/zvol_stress.ksh testcase for both Linux and FreeBSD.

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:

@fuporovvStack
Copy link
Contributor Author

@amotin, @markjdb

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to see somebody working on unifying this code. But see some comments inline:

Copy link
Contributor Author

@fuporovvStack fuporovvStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found 4 ways to call zvol, and decided to use next sync/async switching logic dependent of volmode parameter:
geom:
- (1) zvol_geom_bio_start() - sync
- (2) zvol_geom_worker() - async
cdev:
- (3) zvol_cdev_read()/_write() - sync
- (4) zvol_geom_bio_strategy() - async

(1) - the way dependent from geom thread stack usage, case when direct=1.
(2) - case when direct variable value become zero. The direct value is not depend from sync or aio usage.
(3) - normal usage in cdev mode, example: fio with engine=sync.
(4) - used in case of AIO, example: fio with engine=posixaio

@amotin
Copy link
Member

amotin commented Mar 31, 2025

(1) - the way dependent from geom thread [stack usage](https://github.com/freebsd/freebsd-src/blob/main/sys/geom/geom_io.c#L566), case when direct=1.

Aside of stack usage, some GEOM classes may be unable to use direct I/O due to locking constraints and have to switch into GEOM up/down threads.

@fuporovvStack fuporovvStack force-pushed the zvol-taskq branch 2 times, most recently from 7d474c6 to 0d12476 Compare April 1, 2025 12:22
@tonyhutter
Copy link
Contributor

@fuporovvStack can you rebase on the latest master so you pull in d947b9a?

@fuporovvStack fuporovvStack force-pushed the zvol-taskq branch 2 times, most recently from 2b799e1 to ec70a51 Compare May 3, 2025 07:09
@amotin amotin added Status: Code Review Needed Ready for review and testing Status: Revision Needed Changes are required for the PR to be accepted labels May 5, 2025
@github-actions github-actions bot removed the Status: Revision Needed Changes are required for the PR to be accepted label May 6, 2025
@fuporovvStack fuporovvStack force-pushed the zvol-taskq branch 4 times, most recently from 550f95f to b620d61 Compare May 6, 2025 16:53
Make zvol I/O requests processing asynchronous on FreeBSD side in some
cases. Clone zvol threading logic and required module parameters from
Linux side. Make zvol threadpool creation/destruction logic shared for
both Linux and FreeBSD.
The IO requests are processed asynchronously in next cases:
- volmode=geom: if IO request thread is geom thread or cannot sleep.
- volmode=cdev: if IO request passed thru struct cdevsw .d_strategy
routine, mean is AIO request.
In all other cases the IO requests are processed synchronously. The
volthreading zvol property is ignored on FreeBSD side.

Sponsored-by: vStack, Inc.
Signed-off-by: Fedor Uporov <[email protected]>
@tonyhutter tonyhutter added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 8, 2025
@amotin amotin merged commit 1a8f5ad into openzfs:master May 8, 2025
20 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants