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 pull-request #195 and #277) #280

Closed
wants to merge 5 commits into from

Conversation

kohlschuetter
Copy link
Contributor

(see initial comments on pull request #195 and #277; 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)

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

…#195 by

rlaager)

This patch further develops the idea of rlaager's pull request openzfs#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

Re-adding the performance graph from #277, now to the issue only.

I created a placeholder branch for that, so it should not interfere with any pull requests.

test

@behlendorf
Copy link
Contributor

The patch is looking better, and getting used to Git just takes a little time. But it's well worth the effort. Here are a few more comments which should be easily fixed by a quick rebase of your branch. Don't worry to much about this taking a few iterations, that's the point of reviewing these things. And next time you'll know better.

  • [style] Git commit comments. Please keep the summary line of the git commit to 50 characters, and the body to 72. Here's a nice summary of why this is good practice. http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
  • [style] The git commit comment still references 20 as the max ashift instead of 17.
  • [style] As some additional Github spice you can add the line 'Closes zfault.sh -c hard read error fail on raid0 #123' at the end of your commit and them when I merge the commit it will automatically close the referenced issue. It will also generate a link to the commit in the issue so it's clear exactly what patch resolved the issue.
  • [style] Please add a new, /* readonly onetime number properties */, section comment for your zprop_register_number() call in zpool_prop_init(). Also move it after the other readonly number properties and fix the indentation to be consistent with the others.
        if (props != NULL) {
            char *value;
            if (nvlist_lookup_string(props, zpool_prop_to_name(ZPOOL_PROP_ASHIFT),
            &value) == 0)
            zfs_nicestrtonum(NULL, value, &ashift);

            if (ashift > 0)
            verify(nvlist_add_uint64(vdev, ZPOOL_CONFIG_ASHIFT, ashift) == 0);
    }

It should look like this to be consistent with the Solaris coding style. Notice I've also moved the ashift declaration in to this block. It's not always my favorite style either but consistency is king for this sort of thing.

        if (props != NULL) {
                char *value = NULL;
                uint64_t ashift = 0;

                if (nvlist_lookup_string(props,
                    zpool_prop_to_name(ZPOOL_PROP_ASHIFT), &value) == 0)
                        zfs_nicestrtonum(NULL, value, &ashift);

                if (ashift > 0)
                        verify(nvlist_add_uint64(vdev, ZPOOL_CONFIG_ASHIFT,
                            ashift) == 0);
        }
  • [style] Similar formating changes to the above need to be made to the ZPOOL_PROP_ASHIFT case in zpool_valid_proplist().

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 17 (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
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 17 (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
Conflicts:
	cmd/zpool/zpool_vdev.c
	lib/libzfs/libzfs_pool.c
@kohlschuetter
Copy link
Contributor Author

Commit 9774edd should be the right one. I have no clue what happened here with my commits. Me git rookie.

fuhrmannb pushed a commit to fuhrmannb/cstor that referenced this pull request Nov 3, 2020
ahrens pushed a commit to ahrens/zfs that referenced this pull request Apr 25, 2021
sdimitro pushed a commit to sdimitro/zfs that referenced this pull request May 23, 2022
…task() can overflow (openzfs#280)

The variable `index_skips` in `merge_task()` has the default numeric
type, which is `i32`.  If the merge is resumed after processing more
than 2^31 entries, this will overflow, causing the agent to panic.

This commit changes the variable type to `u64`, preventing overflow.

Bonus cleanup: used `#![warn(clippy::default_numeric_fallback)]` to
identify other variables that defaulted to `i32`.  I added explicit
types for these.  Many of them were fine being `u32` because we already
restrict the size of an object to be no more than 2^32 (4GiB).
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.

2 participants