Skip to content
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

Make ZIL operations on zvols use _by_dnode routines #6058

Merged
merged 1 commit into from
Jun 13, 2017

Conversation

ryao
Copy link
Contributor

@ryao ryao commented Apr 22, 2017

Description

This continues what was started in
0eef1bd by fully converting zvols to
avoid unnecessary dnode_hold() calls. This saves a small amount of CPU
time and slightly improves latencies onsynchronous operations on zvols.

Motivation and Context

I am working on zvol performance at work. While working on larger changes, I noticed unnecessary overhead that is easy to eliminate. Eliminating it brings some small latency improvements from reduced overhead.

How Has This Been Tested?

It has been tested against prophetstor's internal 0.6.5.9 branch using fio:

fio --name=writeiops --filename=/dev/zvol/e7c61cba0e8e4cec8692570e7c9280bf/test --direct=1 --rw=randwrite --bs=4k --numjobs=4 --iodepth=32 --direct=1 --iodepth_batch=16 --iodepth_batch_complete=16 --runtime=1200 --ramp_time=5 --norandommap --time_based --ioengine=libaio --group_reporting --random_distribution=pareto:0.9

The test hardware is 12x M630-DC SSDs, 256GB of RAM and dual Intel(R) Xeon(R) CPU E5-2630 v4 CPUs (20 cores). I tested on a 500GB zvol. I was careful to wait until the cache was hot enough that we weren't reading metadata before evaluating impact.

The branch is using a variation of #5824 (that predates my first day at my new employer last week), so the numbers are not directly applicable to head, but average latencies dropped by ~15%.

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)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@mention-bot
Copy link

@ryao, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @ahrens and @tuxoko to be potential reviewers.

@ryao ryao changed the title Do not do a dnode hold in zvol_get_data Switch synchronous IO on zvols to use _by_dbuf routines Apr 26, 2017
@ryao
Copy link
Contributor Author

ryao commented Apr 26, 2017

I am working on bigger changes, but this should help #4880.

@ryao ryao force-pushed the zvol_zil branch 5 times, most recently from f489809 to 33c316d Compare April 26, 2017 17:46
@ryao ryao changed the title Switch synchronous IO on zvols to use _by_dbuf routines Make ZIL operations on zvols use _by_dbuf routines Apr 26, 2017
@ryao
Copy link
Contributor Author

ryao commented Apr 26, 2017

@behlendorf The failure is #5195, which is a pre-existing issue. It is unrelated to the patch here.

Also, preliminary testing at Prophetstor with a SLOG device shows a 30% improvement in IOPS from this patch alone.

Edit: I neglected to notice that I had also backported the changes to use by_dnode for zvol_write and zvol_read. They had made little difference in my initial tests (without a slog), but I kept them in my local branch. The dnode_hold in dmu_write_bio definitely would have had some contention with the dnode_hold in the ZIL path. I do not have access to the machine with SLOG that was used at work to isolate this change from that, but I imagine the actual improvement is somewhat less than 30%. Nevertheless, this is a step in the right direction. I believe that the improvement comes mainly from enabling ZIL batch processing to iterate faster.

@ryao ryao added OpenZFS Review Type: Performance Performance improvement or performance problem labels Apr 26, 2017
@ryao
Copy link
Contributor Author

ryao commented Apr 27, 2017

@sempervictus If you have a synchronous IO intensive workload, this might give you a small boost. You might want to try testing it.

@sempervictus
Copy link
Contributor

sempervictus commented Apr 27, 2017 via email

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

@ryao definitely progress is the right direction! There was discussion in PR4802 of taking this farther and replacing the dbuf_t with a dnode_t in the zvol_state_t. This would remove the need for the _by_dbuf wrappers, and it might buy you a little more performance since you'll be able to eliminate the DB_DNODE_ENTER/EXIT.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

@ryao let's do this by switching the dbuf_t to a dnode_t in the zvol_state_t and converting all the existing holds.

@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label May 2, 2017
@behlendorf
Copy link
Contributor

@ryao if you want these improvements to make 0.7.0 can you address the review feedback. We're trying to wrap up the release.

@ryao
Copy link
Contributor Author

ryao commented Jun 9, 2017

@behlendorf I like the idea of switching to the _by_dnode routines. This is now refreshed.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. This ends up being a nice simplification too.

@behlendorf behlendorf removed the Status: Work in Progress Not yet ready for general review label Jun 9, 2017
@behlendorf
Copy link
Contributor

@ryao one last rebase to resolve a merge conflict and this should be good to go.

@ryao
Copy link
Contributor Author

ryao commented Jun 9, 2017

@behlendorf It is rebased.

@ryao ryao changed the title Make ZIL operations on zvols use _by_dbuf routines Make ZIL operations on zvols use _by_dnode routines Jun 10, 2017
@behlendorf
Copy link
Contributor

@ryao thanks, I'll get this merged after the buildbot finishes its work.

@sempervictus
Copy link
Contributor

Those test failures dont look too good, crash seems to be coming from:

[ 4304.650875] VERIFY(dn->dn_type != DMU_OT_NONE) failed
[ 4304.655793] PANIC at dbuf.c:2303:dbuf_create()

which seems related to this :)

@behlendorf
Copy link
Contributor

@sempervictus I can see why you thought this might be related since it paniced in dbuf_create(), but it's not. The panic occurred in the zil code for a filesystem and not a zvol, it's more likely this is related to 1b7c1e5 which was recently merged. The link for the log in the previous comment.

@sempervictus
Copy link
Contributor

sempervictus commented Jun 10, 2017 via email

@behlendorf
Copy link
Contributor

@sempervictus are you able to reproduce those zloop failures on the current master branch as of commit 88c3012. If so can you determine as of which commit you started seeing them.

module/zfs/dmu.c Outdated
@@ -965,6 +965,9 @@ int
dmu_read_by_dnode(dnode_t *dn, uint64_t offset, uint64_t size, void *buf,
uint32_t flags)
{
if (size == 0)
return (0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this added? I don't see why it should be needed here or in the write path.

Copy link
Contributor Author

@ryao ryao Jun 12, 2017

Choose a reason for hiding this comment

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

The previous dmu_read_uio_dbuf() function did it. Upon review, it turns out to be unnecessary. I'll remove it.

This continues what was started in
0eef1bd by fully converting zvols
to avoid unnecessary dnode_hold() calls. This saves a small amount
of CPU time and slightly improves latencies of operations on zvols.

Signed-off-by: Richard Yao <richard.yao@prophetstor.com>
@ryao
Copy link
Contributor Author

ryao commented Jun 13, 2017

@behlendorf The failures seem to have disappeared. Why they appeared when I pushed this initally is something of a mystery, but we are passing now. I have made all of the changes you requested. It should be fine to merge now.

@behlendorf
Copy link
Contributor

Thanks @ryao, I'll get this merged.

It turns out the original failure reported by @sempervictus wasn't introduced by this PR. I've seen it one other time against master. I believe it was either introduced by commit 1b7c1e5 or possibly 38240eb. As soon as we can determine which commit actually introduced it we're going to want to either revert that change or if it's straight forward fix it.

I also resubmitted the two failed test runs for the updated version of this patch since they hit unrelated issues. I just wanted to confirm that before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Performance Performance improvement or performance problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants