Remove some dead ARC code. #14340

amotin · 2022-12-29T17:12:32Z

Every ARC buffer holds a reference on the header. It means headers with buffers are never evictable. When we are evicting a header there can be no more buffers to free. Just assert that.

b_evict_lock seems not protecting anything now. Remove it.

Buffers checksum should also be freed with the last uncompressed buffer, so it should not be there also when we are evicting the header.

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)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

amotin · 2022-12-29T17:14:13Z

This PR also depends on #14243, so it includes one as the first commit for now.

amotin · 2023-01-04T17:25:35Z

After discussion with @grwilson we decided to also scrap b_evict_lock. It does not seem to protect anything now.

Every ARC buffer holds a reference on the header. It means headers with buffers are never evictable. When we are evicting a header, there can be no more buffers to free. Just assert that. b_evict_lock seems not protecting anything now. Remove it. Buffers checksum should also be freed with the last uncompressed buffer, so it should not be there also when we are evicting the header. Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc.

grwilson · 2023-01-06T12:57:56Z

module/zfs/arc.c

+	ASSERT0(hdr->b_l1hdr.b_bufcnt);
+	ASSERT3P(hdr->b_l1hdr.b_buf, ==, NULL);
+	ASSERT0(zfs_refcount_count(&hdr->b_l1hdr.b_refcnt));


What do you think about making some or all of these VERIFYs to make sure that the assumptions here are valid in non-debug builds?

This is a very hot single-threaded piece of code, limiting system performance on some workloads. I do not want VERIFY() impossible here without a good reason. Any particular reason you think this may work different for debug and non-debug builds?

If you're convinced that this can't happen then no need to be defensive. My concern was that if we find that the b_buf is not being destroyed because of some missed code path, then we end up with a memory leak which would be hard to track down. Doing a simple VERIFY3P(hdr->b_l1hdr.b_buf, ==, NULL) would allow for a quick diagnosis on non-debug bits. If you've stressed tested this significantly then I would agree that leaving these off makes sense if they can't happen.

I've added the assertions to protect from some logical bugs. Since it didn't fire yet on any tests, I tend to think it won't, and the assertions will just make sure that whoever modify the code in future will have a guard rail.

And it likely won't leak any way. It will get evicted to ghost state with the buffer, and on eviction from there arc_hdr_destroy() frees any possible remaining buffers. I thought to remove it also, but appeared it is actually used when arc_buf_destroy() calls remove_reference() for anonymous header without getting the hash lock.

Ok. The arc behaves very differently in non-debug builds so some of the inherit race conditions may not show up there. I will trust that you're confident enough in the change.

Every ARC buffer holds a reference on the header. It means headers with buffers are never evictable. When we are evicting a header, there can be no more buffers to free. Just assert that. b_evict_lock seems not protecting anything now. Remove it. Buffers checksum should also be freed with the last uncompressed buffer, so it should not be there also when we are evicting the header. Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc.

As part of openzfs#14340 I've removed b_evict_lock, that played no role in ARC eviction process. But appears it played a secondary role of protecting b_hdr pointers in arc_buf_t during header reallocation by arc_hdr_realloc_crypt(), that, as found in openzfs#15293, may cause use after free races if some encrypted block is read while being synced after been writen. After closer look on b_evict_lock I still do not believe it covered all the possible races, so I am not eager to resurrect it. Instead this refactors arc_hdr_realloc_crypt() into arc_realloc_crypt() and moves its calls from arc_write_ready() into upper levels ready callbacks, like dbuf_write_ready(), where it is protected by existing db_mtx, protecting also all the arc buffer accesses. Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc.

To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 64 bytes per header, but with couple patches saving 24 bytes, the net growth is only 40 bytes with total header size of 240 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc.

To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 64 bytes per header, but with couple patches saving 32 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc.

To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc.

To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc.

To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of #14340. As result, as was found in #15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Reviewe-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc. Closes #15293 Closes #15347

To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Reviewe-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc. Closes openzfs#15293 Closes openzfs#15347

To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of #14340. As result, as was found in #15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Reviewe-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc. Closes #15293 Closes #15347

To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Reviewe-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alexander Motin <mav@FreeBSD.org> Sponsored by: iXsystems, Inc. Closes openzfs#15293 Closes openzfs#15347

amotin requested review from behlendorf and grwilson December 29, 2022 17:12

amotin added the Status: Code Review Needed Ready for review and testing label Dec 29, 2022

amotin force-pushed the arc2 branch 4 times, most recently from 2e552cb to 65f47fb Compare January 4, 2023 17:23

amotin force-pushed the arc2 branch from 65f47fb to 4318560 Compare January 4, 2023 17:32

amotin force-pushed the arc2 branch from 4318560 to ac2b1eb Compare January 5, 2023 00:38

behlendorf approved these changes Jan 5, 2023

View reviewed changes

ryao approved these changes Jan 5, 2023

View reviewed changes

grwilson reviewed Jan 6, 2023

View reviewed changes

grwilson approved these changes Jan 9, 2023

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 9, 2023

behlendorf merged commit 289f7e6 into openzfs:master Jan 9, 2023

amotin mentioned this pull request Sep 29, 2023

ARC: Move header reallocation under db_mtx lock #15330

Closed

13 tasks

amotin deleted the arc2 branch September 29, 2023 15:37

amotin mentioned this pull request Oct 3, 2023

ARC: Drop different size headers for crypto #15347

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some dead ARC code. #14340

Remove some dead ARC code. #14340

amotin commented Dec 29, 2022 •

edited

Loading

amotin commented Dec 29, 2022

amotin commented Jan 4, 2023

grwilson Jan 6, 2023

amotin Jan 6, 2023 •

edited

Loading

grwilson Jan 7, 2023

amotin Jan 7, 2023

amotin Jan 7, 2023

grwilson Jan 9, 2023

Remove some dead ARC code. #14340

Remove some dead ARC code. #14340

Conversation

amotin commented Dec 29, 2022 • edited Loading

Types of changes

Checklist:

amotin commented Dec 29, 2022

amotin commented Jan 4, 2023

grwilson Jan 6, 2023

Choose a reason for hiding this comment

amotin Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

grwilson Jan 7, 2023

Choose a reason for hiding this comment

amotin Jan 7, 2023

Choose a reason for hiding this comment

amotin Jan 7, 2023

Choose a reason for hiding this comment

grwilson Jan 9, 2023

Choose a reason for hiding this comment

amotin commented Dec 29, 2022 •

edited

Loading

amotin Jan 6, 2023 •

edited

Loading