-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
OpenZFS 7578 - Fix/improve some aspects of ZIL writing. #6191
Conversation
@dinatale2, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bprotopopov, @behlendorf and @tuxoko to be potential reviewers. |
80764f9
to
d2c3a91
Compare
@dinatale2 nice job reconciling the differences with upstream OpenZFS. I only added two small additional commits for fairly minor issues. |
@ryao it would be great if you could review this OpenZFS patch which include changes to the ZIL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated version of this LGTM and preserves the fastwrite logic. One possible addition which occurred to me is we could add an option to switch between the fastwrite metaslab allocator logic and the alternate OpenZFS logic. This would aid benchmarking.
- After some ZIL changes 6 years ago zil_slog_limit got partially broken due to zl_itx_list_sz not updated when async itx'es upgraded to sync. Actually because of other changes about that time zl_itx_list_sz is not really required to implement the functionality, so this patch removes some unneeded broken code and variables. - Original idea of zil_slog_limit was to reduce chance of SLOG abuse by single heavy logger, that increased latency for other (more latency critical) loggers, by pushing heavy log out into the main pool instead of SLOG. Beside huge latency increase for heavy writers, this implementation caused double write of all data, since the log records were explicitly prepared for SLOG. Since we now have I/O scheduler, I've found it can be much more efficient to reduce priority of heavy logger SLOG writes from ZIO_PRIORITY_SYNC_WRITE to ZIO_PRIORITY_ASYNC_WRITE, while still leave them on SLOG. - Existing ZIL implementation had problem with space efficiency when it has to write large chunks of data into log blocks of limited size. In some cases efficiency stopped to almost as low as 50%. In case of ZIL stored on spinning rust, that also reduced log write speed in half, since head had to uselessly fly over allocated but not written areas. This change improves the situation by offloading problematic operations from z*_log_write() to zil_lwb_commit(), which knows real situation of log blocks allocation and can split large requests into pieces much more efficiently. Also as side effect it removes one of two data copy operations done by ZIL code WR_COPIED case. - While there, untangle and unify code of z*_log_write() functions. Also zfs_log_write() alike to zvol_log_write() can now handle writes crossing block boundary, that may also improve efficiency if ZPL is made to do that. Sponsored by: iXsystems, Inc. Authored by: Alexander Motin <[email protected]> Reviewed by: Matthew Ahrens <[email protected]> Reviewed by: Prakash Surya <[email protected]> Reviewed by: Andriy Gapon <[email protected]> Reviewed by: Steven Hartland <[email protected]> Reviewed by: Brad Lewis <[email protected]> Reviewed by: Richard Elling <[email protected]> Approved by: Robert Mustacchi <[email protected]> Ported-by: Giuseppe Di Natale <[email protected]> OpenZFS-issue: https://www.illumos.org/issues/7578 OpenZFS-commit: openzfs/openzfs@aeb13ac
d2c3a91
to
3409166
Compare
This currently doesn't merge in for testing - 33c316d altered the wr_state piece in the by_dbuf changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. While I think that reducing the IO priority needs more scrutiny, I do not consider that to be a reason to block merging this. We can revisit that in the future if it is found to cause regressions.
@sempervictus That commit is not in HEAD. It is in #6058, which has been refreshed. If this is merged first, it will be simple to rebase on top of it. |
@ryao: Realized that I was on the wrong branch too, thanks. Is there any chance you could rebase the zvol prs on the openzfs zil one since its approved? That also seems to conflict with your zil PR a bit too
|
@ryao @sempervictus merged, you can rebase on top it when your ready. |
due to zl_itx_list_sz not updated when async itx'es upgraded to sync.
Actually because of other changes about that time zl_itx_list_sz is not
really required to implement the functionality, so this patch removes
some unneeded broken code and variables.
single heavy logger, that increased latency for other (more latency critical)
loggers, by pushing heavy log out into the main pool instead of SLOG. Beside
huge latency increase for heavy writers, this implementation caused double
write of all data, since the log records were explicitly prepared for SLOG.
Since we now have I/O scheduler, I've found it can be much more efficient
to reduce priority of heavy logger SLOG writes from ZIO_PRIORITY_SYNC_WRITE
to ZIO_PRIORITY_ASYNC_WRITE, while still leave them on SLOG.
has to write large chunks of data into log blocks of limited size. In some
cases efficiency stopped to almost as low as 50%. In case of ZIL stored on
spinning rust, that also reduced log write speed in half, since head had to
uselessly fly over allocated but not written areas. This change improves
the situation by offloading problematic operations from z*_log_write() to
zil_lwb_commit(), which knows real situation of log blocks allocation and
can split large requests into pieces much more efficiently. Also as side
effect it removes one of two data copy operations done by ZIL code WR_COPIED
case.
Also zfs_log_write() alike to zvol_log_write() can now handle writes crossing
block boundary, that may also improve efficiency if ZPL is made to do that.
Sponsored by: iXsystems, Inc.
Authored by: Alexander Motin [email protected]
Reviewed by: Matthew Ahrens [email protected]
Reviewed by: Prakash Surya [email protected]
Reviewed by: Andriy Gapon [email protected]
Reviewed by: Steven Hartland [email protected]
Reviewed by: Brad Lewis [email protected]
Reviewed by: Richard Elling [email protected]
Approved by: Robert Mustacchi [email protected]
Ported-by: Giuseppe Di Natale [email protected]
OpenZFS-issue: https://www.illumos.org/issues/7578
OpenZFS-commit: openzfs/openzfs@aeb13ac
Checklist:
Signed-off-by
.