[developer] [openzfs/openzfs] 9337 zfs get all is slow due to uncached metadata (#599)
Reviewed by: Prakash Surya
Reviewed by: George Wilson
This project's goal is to make read-heavy channel programs and zfs(1m)
administrative commands faster by caching all the metadata that they will
need in the dbuf layer. This will prevent the data from being evicted, so
that any future call to i.e. zfs get all won't have to go to disk (very
much). There are two parts:
The dbuf_metadata_cache. We identify what to put into the cache based on
the object type of each dbuf. Caching objset properties os
{version,normalization,utf8only,casesensitivity} in the objset_t. The reason
these needed to be cached is that although they are queried frequently,
they aren't stored in a dbuf type which we can easily recognize and cache in
the dbuf layer; instead, we have to explicitly store them. There's already
existing infrastructure for maintaining cached properties in the objset
setup code, so I simply used that.
Testing performance
Setup:
Disabled kmem_flags
Tuned dbuf_cache_max_bytes very low (128K)
Tuned zfs_arc_max very low (64M)
Created test pool with 400 filesystems, and 100 snapshots per filesystem.
Later on in testing, added 600 more filesystems (with no snapshots) to make
sure scaling didn't look different between snapshots and filesystems.
Results:
| Test | Time (trunk / diff) | I/Os (trunk / diff) |
++-+-+
| zpool import | 0:05 / 0:06 |12.9k / 12.9k|
| zfs get all (uncached) | 1:36 / 0:53 |16.7k / 5.7k |
| zfs get all (cached) | 1:36 / 0:51 |16.0k / 6.0k |
Most cases are faster, and we do a reasonable amount less I/O in every case
(except for zpool import where we weren't expecting an improvement). Great :)
Upstream bug: DLPX-28586
Reviewed at: http://reviews.delphix.com/r/34845/
You can view, comment on, or merge this pull request online at:
https://github.com/openzfs/openzfs/pull/599
-- Commit Summary --
* 9337 zfs get all is slow due to uncached metadata
-- File Changes --
M usr/src/uts/common/fs/zfs/dbuf.c (182)
M usr/src/uts/common/fs/zfs/dmu.c (108)
M usr/src/uts/common/fs/zfs/dmu_objset.c (8)
M usr/src/uts/common/fs/zfs/sys/dbuf.h (14)
M usr/src/uts/common/fs/zfs/sys/dmu.h (7)
M usr/src/uts/common/fs/zfs/sys/dmu_objset.h (12)
M usr/src/uts/common/fs/zfs/zfs_vfsops.c (52)
-- Patch Links --
https://github.com/openzfs/openzfs/pull/599.patch
https://github.com/openzfs/openzfs/pull/599.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openzfs/openzfs/pull/599
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/discussions/T644dae5d5a17704c-Mf5be77bc517e7b5e42c4dfdc
Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 8727 Native data and metadata encryption for zfs (#489)
@ahrens pushed 1 commit. a048a01 fix zero-length encryption -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/openzfs/openzfs/pull/489/files/fda8b076ac99bf44fbf977c9bcd94cccf9636e88..a048a01fa13fac430a9f9c838f70569e54122101 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T91797982fdd5b7d9-M2de0d2b64ca96700287513d6 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 8727 Native data and metadata encryption for zfs (#489)
I added a commit to this PR which fixes the issue with `Invalid dnode block MAC`. In blocks of dnodes, only the bonus buffers are encrypted. If there are no bonus buffers (which is always the case with ZVOLs), then the amount of data to decrypt will be 0, but we still want to go through the decryption routines for the MAC validation (the MAC covers more than just the bonus buffers). The problem is that `ccm_init_ctx()` tries to allocate "the amount of encrypted data" which is zero, but `kmem_alloc(0)` returns NULL, which `ccm_init_ctx()` interprets as an allocation failure, so it fails the decrypt operation with `CRYPTO_HOST_MEMORY`. On ZoL, the problem doesn't occur because this call to `kmem_alloc()` was changed to `vmem_alloc()`. The fix is for `ccm_init_ctx()` to only try to allocate (and check for an error from allocation) if we have a nonzero length. I checked for similar uses of kmem_alloc(length) in `usr/src/common/crypto` and didn't find any. I also made a change in `crypto_uio_data()` which fixes a mismerge from the original ZoL crypto commit (and is superficially similar to the bug but didn't actually fix it). @lundman Be sure to pull down my commit before you rebase / force push to your github branch. @GernotS I wasn't able to reproduce the NULL pointer dereference when using L2ARC, but it's very possible that is due to improper error handling of this same spurious decryption error. Could you run your L2ARC test again with this fix and let me know if it works now? @tcaputi said he'd bring this fix to ZoL - Thanks! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/489#issuecomment-375717708 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T91797982fdd5b7d9-M10542107ffda1912eb171371 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9337 zfs get all is slow due to uncached metadata (#599)
tcaputi approved this pull request. This patch looks good and all the edge cases I could think of are handled appropriately (zfs upgrade, etc). As a minor convenience, I would want a way to determine how much memory my metadata cache is using (in ZoL we would make this a read-only tunable, maybe you can already see this in Illumos with your debugging tools). I would also be interested to know what the limiting performance factor is after applying this diff. 51 seconds still seems like a long time when all the data is already in memory. This, however, is not really a concern for this PR. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/599#pullrequestreview-106650144 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T644dae5d5a17704c-M52593237719a1891e9e13fe9 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 9337 zfs get all is slow due to uncached metadata (#599)
@tcaputi To determine the cache size, you could look at `dbuf_caches[DB_DBUF_METADATA_CACHE].size`. While this is trivial with mdb (assuming you have root access), I agree it would be nice to add a kstat for this. IIRC, it seemed like kind of a pain since the kstats seem to be specific to each .c file, and there isn't already a kstat set up in dbuf.c. But that doesn't mean we shouldn't do it at some point :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/599#issuecomment-375810016 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T644dae5d5a17704c-Mc7e019ff12d62a302f8726e4 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 8727 Native data and metadata encryption for zfs (#489)
@ahrens My apologies for missing the modes.c line, I have pulled the commit and added to osx. Alas, I can easily make crypto+cache panic happen on osx, (and not so easily on illumos) ``` arc.c arc_read 6180 : ASSERT3( hdr->b_l1hdr.b_buf == ((void *)0)) failed (0xff889975ce88 == 0) ``` -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/489#issuecomment-375822913 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T91797982fdd5b7d9-M0965371b63d424df9e9a9ed0 Delivery options: https://openzfs.topicbox.com/groups
[developer] Re: [openzfs/openzfs] 8727 Native data and metadata encryption for zfs (#489)
@lundman `crypto+cache` meaning these bits with a l2arc (cache) device? What is your workload? If you're able to reproduce on illumos with the latest bits, can you send me a dump? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/489#issuecomment-375829942 -- openzfs: openzfs-developer Permalink: https://openzfs.topicbox.com/groups/developer/discussions/T91797982fdd5b7d9-M94e8b94170f0ee604a053595 Delivery options: https://openzfs.topicbox.com/groups
