MFV r323535: 8585 improve batching done in zil_commit()
ClosedPublic
Actions

Authored by avg on Sep 13 2017, 11:41 AM.

Details

Reviewers

mav
jpaetzel
allanjude

Group Reviewers

ZFS

Commits

rS324011: MFV r323535: 8585 improve batching done in zil_commit()

Summary

https://github.com/illumos/illumos-gate/commit/1271e4b10dfaaed576c08a812f466f6e81370e5e
https://www.illumos.org/issues/8585
https://github.com/openzfs/openzfs/pull/447

This review request is to invite reviewers and testers for this rather complex change.

Also, please note that this MFV reverts FreeBSD commit rS314549 to make the merge easier.
Another note is that at present our emulation of cv_timedwait_hires is rather poor, so I elected to use cv_timedwait_sbt directly.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 11539
Build 11892: arc lint + arc unit

Event Timeline

avg created this revision.Sep 13 2017, 11:41 AM

Herald added subscribers: delphij, imp. · View Herald TranscriptSep 13 2017, 11:41 AM

avg edited the summary of this revision. (Show Details)Sep 13 2017, 11:49 AM

avg added a reviewer: allanjude.

I red it quite briefly, but I suspect that use of cv_timedwait_sbt() together gethrtime() may not work right due to rounding issues. As is was already mentioned by others, in most cases use of SBT_1NS is a direct request for troubles. My fault.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zil.c
2335	I have doubt this will work correctly. SBT_1NS is too approximate, especially for use with long periods of time such as uptime.

imp added inline comments.Sep 13 2017, 7:44 PM

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zil.c
2335	gibbs recently committed code that fixes this issue...

avg added a subscriber: • ian.Sep 15 2017, 6:37 PM

avg added inline comments.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zil.c
2335	`nstosbt` in sys/sys/time.h? Seems like it was added by @ian in rS321686. Thank you for the hint! I also fell into the honeypot that the commit warns about.
2335	Another alternative would be to use sbintime_t for `sleep` and `wakeup` variables and minimize conversions.

a better solution to sbintime_t vs hrtime_t

@avg Please see also D11779 which has another nstosbt() change in it that I didn't want to commit without review because I don't know if there's anything special about committing into the sys/cddl hierarchy.