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

microzap: set hard upper limit of 1M #16888

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robn
Copy link
Member

@robn robn commented Dec 19, 2024

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

We were running tests with large zap_micro_max_size, and could easily trip the panic in mzap_addent().

Description

The count of chunks in a microzap block is stored as an int16_t (zap_num_chunks). Each chunk is 64 bytes, and the first is used to store a header, so there are 32767 usable chunks, which is just under 2M. 1M is the largest power-2-rounded block size under 2M, so we must set the limit there.

How Has This Been Tested?

echo 16777216 > /sys/module/zfs/parameters/zap_micro_max_size
zpool create tank ...
cd /tank
seq 1 1000000 | xargs touch

This will panic consistently until the tunable value gets down to 1M. With this patch, its safe.

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:

@allanjude
Copy link
Contributor

I was confused for a minute, you meant int16_t, ok.

module/zfs/zap_micro.c Outdated Show resolved Hide resolved
@allanjude
Copy link
Contributor

The on-disk component is:

        uint16_t mze_chunkid;

Is there a reason we use a signed value in memory?
Changing that would let us get to 2 MB, although I am not sure how much value that has

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Dec 19, 2024
The count of chunks in a microzap block is stored as an uint16_t
(mze_chunkid). Each chunk is 64 bytes, and the first is used to store a
header, so there are 32767 usable chunks, which is just under 2M. 1M is
the largest power-2-rounded block size under 2M, so we must set the
limit there.

If it goes higher, the loop in mzap_addent can overflow and fall into
the PANIC case.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn
Copy link
Member Author

robn commented Dec 19, 2024

Last push updates code and commit comment.

It turns out it can actually survive at max 2M. Adding entry 32768 causes a hash collision though, which immediately triggers an upgrade to fatzap. So that really does make 1M the practical upper limit.

@allanjude It might be possible to update everything to use unsigned ints internally to get to 2M (not sure about the hashing), but that would still be a problem if the block is used on an older ZFS (import or replication). The feature flag will help with that after 2.3 of course, but still.

(at this point I'm tempted to deprecate this tunable entirely).

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Looks good to me. But on a quick look I haven't noticed any difficulties with making the 3 variables unsigned either, just to not have artificial the limits in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants