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

Managing zfs cache devices requires lots of ram #3038

Conversation

dweeezil
Copy link
Contributor

This is the foundation of a patch stack on which the port of the illumos "5497 lock contention on arcs_mtx" will be based. I'll refer to #2971, #3021 and #3029 at this point as related issues. I discovered the importance of this patch during my own groundwork but I'll note that @prakashsurya also suggested it in #2971.

As has been typical with my Illumos patch stacks in the past, I don't expect this one to be merged as-is but I'm posting this so other developers can get an idea of the issues involved with getting our code more in line with upstream insofar as recent ARC changes are concerned.

As a foundation to this, I've reverted 68121a0 because illumos 5497 eliminates arc_evict_ghost() and, I believe, mostly accomplished the same things. I've also reverted ecf3d9b because it was the source of numerous noisy merge conflicts and this commit adds a new cache for the l2arc stuff. I plan on re-adding the DDT parts of ecf3d9b (which should be quite trivial).

The main conflicting ZoL commits at this point are 6e1d727 and e0b0ca9. The former fixed the size calculations for various L2ARC stats and the latter is @behlendorf's addition of the dbuf kstat. I've not tested either but I'll point out arc_buf_info() regarding the latter commit because I'm pretty sure I did the Right Thing there.

@prakashsurya
Copy link
Member

I've only read through the commit messages and pull request description, but the intent here looks good to me. I don't suspect there will be much linux specific changes that will need to be made, but I know that are some memory allocation changes that need to be inspected with linux in mind (e.g. arc_hdr_realloc()).

@dweeezil
Copy link
Contributor Author

Yes, good points. I had fully expected to redo this patch stack several times before the ultimate goal of porting illumos 5497 is achieved. For the moment, the initial note in this pull request will have to suffice.

@behlendorf behlendorf added this to the 0.6.5 milestone Feb 6, 2015
dweeezil and others added 4 commits March 1, 2015 17:56
This reverts commit 037763e.

XXX - expand this comment as to why we're reverting it.
This reverts commit 68121a0.

Conflicts:
	module/zfs/arc.c
This reverts commit ecf3d9b.

Conflicts:
	module/zfs/arc.c
	module/zfs/ddt.c
	module/zfs/spa_misc.c
5369 arc flags should be an enum
5370 consistent arc_buf_hdr_t naming scheme
Reviewed by: Matthew Ahrens <mahrens@delphix.com>
Reviewed by: Alex Reece <alex.reece@delphix.com>
Reviewed by: Sebastien Roy <sebastien.roy@delphix.com>
Reviewed by: Richard Elling <richard.elling@richardelling.com>
Approved by: Richard Lowe <richlowe@richlowe.net>
@dweeezil
Copy link
Contributor Author

dweeezil commented Mar 2, 2015

See #3115. This patch requires an overhaul of the ARC tracepoint support which I'm working on now.

5408 managing ZFS cache devices requires lots of RAM
Reviewed by: Christopher Siden <christopher.siden@delphix.com>
Reviewed by: George Wilson <george.wilson@delphix.com>
Reviewed by: Matthew Ahrens <mahrens@delphix.com>
Reviewed by: Don Brady <dev.fs.zfs@gmail.com>
Reviewed by: Josef 'Jeff' Sipek <josef.sipek@nexenta.com>
Approved by: Garrett D'Amore <garrett@damore.org>
Ported by: Tim Chase <tim@chase2k.com>

Porting notes:

Due to the restructuring of the ARC-related structures, this
patch conflicts with at least the following existing ZoL commits:

    6e1d727
    Fix inaccurate arcstat_l2_hdr_size calculations

        The ARC_SPACE_HDRS constant no longer exists and has been
        somewhat equivalently replaced by HDR_L2ONLY_SIZE.

    e0b0ca9
    Add visibility in to cached dbufs

        The new layering of l{1,2}arc_buf_hdr_t within the arc_buf_hdr
        struct requires additional structure member names to be used
        when referencing the inner items.  Also, the presence of L1 or L2
        inner member is indicated by flags using the new HDR_HAS_L{1,2}HDR
        macros.
@dweeezil dweeezil force-pushed the managing-zfs-cache-devices-requires-lots-of-ram branch from 3eed257 to dac17ff Compare March 23, 2015 01:13
@dweeezil
Copy link
Contributor Author

I've re-pushed this with, hopefully, the tracepoint support working. It has not been rebased on to a current master yet. See #3115.

@prakashsurya
Copy link
Member

Just an FYI, this patch introduces a regression in the L2ARC size accounting reported by zpool list -v. I have a fix that just landed in illumos in this commit:

commit a52fc310ba80fa3b2006110936198de7f828cd94
Author: Prakash Surya <prakash.surya@delphix.com>
Date:   Tue Apr 14 18:03:33 2015 -0700

    5701 zpool list reports incorrect "alloc" value for cache devices
    Reviewed by: Matthew Ahrens <mahrens@delphix.com>
    Reviewed by: George Wilson <george@delphix.com>
    Reviewed by: Alek Pinchuk <alek.pinchuk@nexenta.com>
    Approved by: Dan McDonald <danmcd@omniti.com>

but that fix also relies on the "arc lock contention" patch being ported here: #3115

@dweeezil
Copy link
Contributor Author

@prakashsurya Thanks for the heads-up. I'm hoping to get back to working on #3115 Real Soon Now that, hopefully, I'm done chasing runaway metadata (for the time being).

@dweeezil
Copy link
Contributor Author

Closing because this will be a part of the #3115 patch stack.

@dweeezil dweeezil closed this May 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants