Skip to content
This repository has been archived by the owner on Feb 16, 2020. It is now read-only.

First try to implement ashift property. #2

Closed
wants to merge 6 commits into from

Conversation

BjoKaSH
Copy link

@BjoKaSH BjoKaSH commented Aug 5, 2012

Hello Alex,

As pushed forward by Daniel on the mailing list, implements "zpool create -o ashift=xxx ...". I have tested it on Snow Leopard (using my makefile-build system) and it seems to work.

This commit should work as "fast-forward" on your master.

-- Björn

This commit tries to implement the ashift pool property as used
in ZFSonLinux and the ZEVO beta releases. It is based on the
ZFSonLinux implementation, and follows this comment:
openzfs/zfs#195 (comment)
and the resulting final patch for ZFSonLinux found here:
openzfs/zfs@df30f56
The original inventors are

Due to the large distance between macZFS and ZFSonLinux
(pool version 8 vs. 28) it was necessary to redo and adapt their
approach instead of just copying the changes.

Note that this solution is technical not fully correct:
(a) it introduces a pool property that should not exist in pool
version 8
(b) it introduces a new property type, PROP_ONTIME that should not
exist in pool version 8
(c) the ashift value is really a property of individual top-level
vdevs, and not a pool-wide value. A pool can (and should under
certain conditions) have top-level vdevs of different block size.

However, using a pool-wide property is what other implementations do,
so we do the same for the sake of compatibility.

This commit tries to implement the ashift pool property as used
in ZFSonLinux and the ZEVO beta releases.  It is based on the
ZFSonLinux implementation, and follows this comment:
openzfs/zfs#195 (comment)
and the resulting final patch for ZFSonLinux found here:
openzfs/zfs@df30f56
The original inventors are
- Darik Horn <[email protected]>
- Richard Laager <[email protected]>
- Christian Kohlschütter <[email protected]>

Due to the large distance between macZFS and ZFSonLinux
(pool version 8 vs. 28) it was necessary to redo and adapt their
approach instead of just copying the changes.

Note that this solution is technical not fully correct:
(a) it introduces a pool property that should not exist in pool
    version 8
(b) it introduces a new property type, PROP_ONTIME that should not
    exist in pool version 8
(c) the ashift value is really a property of individual top-level
    vdevs, and not a pool-wide value.  A pool can (and should under
    certain conditions) have top-level vdevs of different block size.

However, using a pool-wide property is what other implementations do,
so we do the same for the sake of compatibility.
@BjoKaSH
Copy link
Author

BjoKaSH commented Aug 6, 2012

If have ported this changes over to your untested, to be found here:
BjoKaSH@70dff54

The diff untested_ashift to untested is smaller then the diff maczfs_74+ashift to maczfs_74, because maczfs_78 already has all the infrastructure for pool properties that was still missing from maczfs_74.

However, this commit kernel panics. But as the bare untest kernel panics in the same situation, I am quite sure the panic is unrelated to my ashift implementation.

… ashift property in some placed missed earlier.
@BjoKaSH
Copy link
Author

BjoKaSH commented Aug 8, 2012

I moved the ashift stuff for untested form the untested branch itself into a feature branch untested_ashift. That required rewriting history and dropping my two commits from untesting, both commits ore now part of untested_ashift (unmodified, i.e same IDs as before). Sorry for the confusion

Different topic:
I added the missing input sanitization in the untested_ashift branch, but not yet in the ashift12 branch of maczfs_74. Reason is, that the maczfs_74 revision misses all the needed infrastructure to easily do the input checking. Will think about how I can add something.

This commit adds the missing bits to check the numeric value of the
ashift property.  Please review carefully, since this part is most
different in maczfs_74 from all newer revisions.
@BjoKaSH
Copy link
Author

BjoKaSH commented Aug 11, 2012

Hi Alex,
the last commit tries to add input sanitization. Unfortunately maczfs_74 misses most of the needed infrastructure, so I had to work around the limitations. Especially, I needed a way to know if the property is set at creation time or any other time (which is forbidden). In lack of the needed flags in_74, I instead test the zhp in zfs_validate_props for being NULL. I think it should be zero at creation time and only at creation time. Please review especially that last commit. All the rest should be fine.
-- Björn

…r new pool properties.

This fixes the kernel panic introduced by the previous fix to the ashift
property implementation.  This revision supports setting ashift at pool
creation, rejects resetting the property and correctly reports the property
in zpool get.

Fixes issue 111
@BjoKaSH
Copy link
Author

BjoKaSH commented Aug 25, 2012

Obsoleted by new request #4

Closed by BjoKaSH

@BjoKaSH BjoKaSH closed this Aug 25, 2012
@BjoKaSH BjoKaSH mentioned this pull request Aug 27, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant