Illumos 5368 - ARC should cache more metadata

[DeHackEd]

5368 ARC should cache more metadata
Reviewed by: Alex Reece [email protected]
Reviewed by: Christopher Siden [email protected]
Reviewed by: George Wilson [email protected]
Reviewed by: Richard Elling [email protected]
Approved by: Dan McDonald [email protected]

References:
illumos/illumos-gate@3a5286a
https://www.illumos.org/issues/5368

Porting notes:

1. Style change to eliminate gcc compiler warning in arc_init
   variable initialization

2. Module parameter exported and added to the man page.

3. Module parameter supports dynamic modification like other
   ARC tunables.

Ported by: DHE [email protected]

[DeHackEd]

Admittedly not tested with a running kernel yet.

[prakashsurya]

I'd recommend to hold off on pulling this patch in. Partly because the ZoL code has addressed this problem in a different way:

commit da8ccd0ee0d4eed08200e706cb2ca1da3bdfb5cf
Author: Prakash Surya <[email protected]>
Date:   Mon Dec 30 09:30:00 2013 -0800

    Prioritize "metadata" in arc_get_data_buf

but also because it'll soon be obsolete in the coming months. I did some work for DelphixOS surrounding ARC performance improvements, which guts this change in favor of something different. That should be out for review on illumos over the next couple months.

[behlendorf]

@prakashsurya thanks for the heads up. We'll watch for the DelphixOS change when you post it for review.

[prakashsurya]

@behlendorf These are the ARC changes that I referenced in my previous comment: https://reviews.csiden.org/r/151/

I think it'll take some care to pull in, due to some difference between the linux and illumos code bases, but shouldn't be too difficult. It'd be interesting to get some performance number and lock profiling info with this patch on linux too, if somebody had the time to run those.

Also, I'd recommend pulling in this change first: https://reviews.csiden.org/r/145/ .. I don't recall it being strictly necessary, but there were a few small locking issues that I ran into due to that L2ARC patch, and pulling that in first will just make it easier to ensure subtle incompatibilities don't creep in.

[behlendorf]

@prakashsurya Thanks for referencing your ARC improvements above. They look very promising, I definitely want to find the time to get you some review comments and testing results. Now that we're past the holidays that should be doable. It would be great if someone with a little time could make a first pass as rebasing your changes against ZoL.

[prakashsurya]

I don't know if this is the right place for this type of commentary, but that patch has landed in illumos:

commit 244781f10dcd82684fd8163c016540667842f203
Author: Prakash Surya <[email protected]>
Date:   Mon Jan 12 19:52:19 2015 -0800

    5497 lock contention on arcs_mtx
    Reviewed by: George Wilson <[email protected]>
    Reviewed by: Matthew Ahrens <[email protected]>
    Reviewed by: Richard Elling <[email protected]>
    Approved by: Dan McDonald <[email protected]>

If somebody can take a first pass and get a pull request open for it against ZoL, I can try and find some time to look it over with the ZoL changes in mind.

[behlendorf]

I agree, it would be great is someone could make a pass at porting this. I'll get to it fairly soon probably but if someone beats me too it that would be great.

[dswartz]

I agree, it would be great is someone could make a pass at porting this.
I'll get to it fairly soon probably but if someone beats me too it that
would be great.

I'll give this a go...

[behlendorf]

Rather than pull the Illumos fix let's hold off, we already have a similar fix in ZoL. We'll only need to drop this code again when the updated arc mutex contention code is merged.

[sempervictus]

Should we leave this open in pending status dependent on the ARC mutex code? We've run this for a number of builds with no ill effects we know of, so at the very least its "pretty stable."

[behlendorf]

I don't mind leaving this open. @sempervictus have you found the patch to be helpful?

[sempervictus]

One specific workload we've seen this help with is datasets backing MySQL databases for Zenoss environments. For one thing, compression ratio on those datasets ends up being something like 9:1 on average, and the access patterns can be unpleasant, especially since ARC has to contend with its own memory requirements (significant, moreso given how we configure the Zenoss stack). With this patch we've seen the wait time curve smooth out, since we're only seeking on disk for the data blocks most of the time, and pulling metadata from ARC. The workload also gets mildly better arcstats on miss %'s with this patch applied.

[prakashsurya]

@sempervictus That actually tells me you're getting hits on data blocks that you otherwise would not (on stock master). This patch will potentially evict metadata in preference for keeping data, which the master code will not do (i.e. when arc_meta_used is between the min and max). If this patch is improving performance and hit rate for you, I'm hopeful the arc lock contention patch will give you similar gains to the ARC hit rate since it has similar logic to this patch (see arc_adjust_type() in the new code).

[sempervictus]

@prakashsurya Thanks for the heads up. Is there any chance you've got a ZoL PR in the works for that stack? I'd assume its not the cleanest port due to memory management and locking differences between the platforms.

[dweeezil]

@sempervictus From a recent email I sent (my project for this weekend is to get it actually working):

If you're interested in seeing the WIP for the patch, I'm pushing it to my "lock-contention-on-arcs_mtx" branch periodically (https://github.com/dweeezil/zfs/tree/lock-contention-on-arcs_mtx). It compiles at this point but I've not tried running it and I don't intend on even trying to run it until I give it a thorough audit. I consider the patch to be in a "rough first pass" form at this point with the main effect being that I've had a chance to look at and try to understand all the chances. Among other things, I've not addressed the dbufs kstat which will have to change somewhat as a result of this work.

[sempervictus]

@dweeezil: thank you much, skimming it now, might even be able to build out in a VM for some testing as you go this weekend. I'm a but curious how this would impact #1971's increased contention tradeoff when the locks are subdivided like this into the sublists.

[dweeezil]

See #3115. I've not yet looked into the #1971 issue yet.

[behlendorf]

Closing. After merging the changes from #3115 we've picked up the changes intended by this patch.