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

Add "ashift" pool property to zpool create (follow-up of push #195 by rlaager) #277

Merged
merged 0 commits into from
Jun 16, 2011

Conversation

kohlschuetter
Copy link
Contributor

This patch further develops the idea of rlaager's pull request #195, taking Brian's comments into account.

Some disks with sectors larger than 512 bytes (e.g., 4k) can suffer from bad write performance
when "ashift" is not configured correctly at volume level (WDC WD20EARS, for example). This is
caused by the disk not reporting its actual sector size, but 512 instead. When creating a zpool,
ZFS takes that wrong sector size and sets the "ashift" property accordingly (to 9: 1<<9=512),
whereas it should be set to 12 for 4k sectors (1<<12=4096).

To enable some performance tuning, the ashift property allows values between 9 and 20 (to specify
sector sizes of up to 1MB). A value of 0 means "default ZFS behavior".

Example:

zpool create -o ashift=12 tank raidz2 sda sdb sdc sdd

@kohlschuetter
Copy link
Contributor Author

Note on ashift performance:

ashift=12 gives me a ~30MB/s performance boost over ashift=9.
Interestingly, as stevecs already commented in #195, ashift=13 further improves performance. With my setup, writes get faster by about 5 MB/s

My testing setup was 4x WD20EARS-00MVWB0 (the three-platter version, which appears to be faster than the previous four platter build) on a raidz2. I will conduct more detailed tests soon on a 5x WD20EARS setup (still missing a component for that), but write performance shouldn't change.

See commit 5cf4c06 for the corresponding plot.

@stevecs
Copy link

stevecs commented Jun 15, 2011

Thanks for taking this. One note that we probably do not want to have an ashift value greater than 17. Since ZFS has a max block size of 128K there are probably a lot of land-mines in the code that will not handle a device physical size > than the zfs block size. Until that gets sanity checked would suggest to protect the normal users from themselves to keep it in the range of ashift of 9 through 17 inclusive.

@kohlschuetter
Copy link
Contributor Author

Thanks stevecs for this hint, I was not aware of that. It's changed now (plus, we have the man page change initially provided by rlaager).

@behlendorf
Copy link
Contributor

Thanks for taking this on! The technical content in this patch looks very good. It's exactly what I was looking for and it works exactly as designed for me after some light testing. However, I am going to have to ask you to clean up a few cosmetic things before I can merge this pull request.

  • [style] Please stick to the SunOS coding standards, http://www.scribd.com/doc/2798406/C-Style-and-Coding-Standards-for-Solaris. Keeping the code consist is good for the general health and readability of the code.
    • Keep a space in the 'if ()'
    • Indent 8 spaces with a tab, when wrapping lines indent 4 spaces
    • {}'s should be omitted for single line if conditionals
  • [style] Your editor appears to have left several stray tabs at the ends of lines. You can see them easily in the diff, please remove them.
  • [style] I think we can leave out the zfsonlinux custom property comments. If someone looking at the source wants to know where this change came from they can refer to the git commit logs.
  • [testing] The performance data is great, I love to see this sort of thing. But I don't think we want to add it to the repository. A summary of the results in the git commit should be enough. It would be nice to keep the graph in the issue tracker but unfortunately that doesn't look so easy.
  • [process] When submitting the new pull request with fixes make sure you've rebased against the latest master. You'll also want to squash all these minor fixes in to a single commit, there's no reason to preserve this review iteration noise.

After you make the above fixes I'll do some more rigorous tests. Assuming everything works well I'm happy to merge this. I know this is something people have been wanting for a while.

@kohlschuetter
Copy link
Contributor Author

Thanks for your comments, Brian. I will make the fixes as described.

Sorry for not doing it correctly in the first place, it actually was my first day hands-on with ZFS, git, and for a long time my first C patch. Yeah, and well, I shouldn't have used "joe" as an editor :-)

@kohlschuetter kohlschuetter merged commit e130330 into openzfs:master Jun 16, 2011
@kohlschuetter
Copy link
Contributor Author

See pull request #280 as a follow-up.

Due to my git-incompetence I have started my repo from scratch, switched to Eclipse/egit and now hope
everything works fine -- sorry for any inconvenience.

ahrens added a commit to ahrens/zfs that referenced this pull request Apr 24, 2021
mmaybee pushed a commit to mmaybee/openzfs that referenced this pull request Apr 6, 2022
…gging (openzfs#277)

We would like to be able to print the log messages from a core file.
Unfortunately, lazy_static! makes it hard to get to the global object
from the debugger.  We would like to save the pointer in a global
variable so that we can easily dereference it from rust-gdb.

The way we are saving LOG_MESSAGES_PTR currently is not right; we're
saving the location of the variable on the stack, which will be moved
into the "real" location after it is returned.

This commit solves the problem by allocating the data on the heap
(inside a `Box`), so that we can know its address before the initializer
returns.  Because this is a little tricky, and you have to name the
(potentially complicated) type of the variable 3 times, a new macro is
introduced to do this for us.

Now in the debugger you can do:

```
(gdb) p util::logging::LOG_MESSAGES_PTR.p.value.data
$1 = core::cell::UnsafeCell<alloc::collections::vec_deque::VecDeque<alloc::string::String, alloc::alloc::Global>> {value: VecDeque(size=28) = {
...
    "[2022-03-04 23:01:49.089][zfs_object_agent][ERROR] Starting ZFS Object Agent (zfs-0.7.0-6003-g5b402b78c).  Local timezone is +00:00 (+00:00)",
```
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.

3 participants