-
-
usr/src/lib/libzfs/common/libzfs_dataset.c (Diff revision 1) volsize_from_vdevs()
doesn't seem to consider metadata requirements (L1+ and copies). This won't
be seen in small volumes, but will be important for large volumes. -
usr/src/test/zfs-tests/tests/functional/refreserv/refreserv_006_pos.ksh (Diff revision 1) nit: we've been trying to change the ZTS test names to something more human-friendly.
I'd suggest renaming as:- refreserv_006_pos.ksh -> refreserv_raidz.ksh
- refreserv_007_pos.ksh -> refreserv_multi_raidz.ksh
-
usr/src/test/zfs-tests/tests/functional/refreserv/refreserv_006_pos.ksh (Diff revision 1) for consistency
$parity
can beparity
-
usr/src/test/zfs-tests/tests/functional/refreserv/refreserv_007_pos.ksh (Diff revision 1) this seems like a copy-n-paste from refreserv_006_pos.ksh, is that intentional?
9318 vol_volsize_to_reservation does not account for raidz skip blocks
Review Request #1997 — Created June 13, 2019 and submitted
Information | |
---|---|
mgerdts | |
illumos-gate | |
Reviewers | |
general | |
This makes it so that
refreservation=auto
causes therefreservation
to be sized based on the least efficient top-level vdev in the pool.
- Performed a full run of zfstest on a baseline (pkg://omnios/entire@11-151031.0:20190603T195911Z) and on new bits. No new failures were observed. Note that some tests caused zfs-on-zfs deadlock in the baseline so I commented those tests.
- Added new tests to cover new functionality. They pass with 3 disks, with 14+ disks, with 512 byte or 4096 sectors. 512x3 512x20 4096x3 4096x20
- Created raidz{1,2,3} pools with various numbers of 4Kn disks, created volumes with various values for
volblocksize
, then filled the volumes. Verified that referenced never exceeded refreservation and that refreservation was unreasonbly high. results - Did volume overwrite test on a bunch of other whacky multi-raidz{1,2,3} and mirror combo pools. results
- While testing
volblocksize=512
onashift=9
pools, I found that things are whack even in the baseline. See illumos#11237.
Change Summary:
Addressed all of Richard's feedback.
Testing Done: |
|
---|
-
In libzfs_dataset.c, line 5191, the comment talks about updating the relevant test. Should there be an associated change in that test for the modifications in this function?
-
For the record:
To: Mike Gerdts <mike.gerdts@joyent.com> From: Sanjay Nadkarni <sanjay.nadkarni@nexenta.com> Subject: RB 9318 vol_volsize_to_reservation does not account for raidz skip blocks Date: Sat, 15 Jun 2019 18:26:14 -0600 Hi Mike, I can't seem to login to RB. I've reviewed your code and it looks too me. -Sanjay
-
-
usr/src/lib/libzfs/common/libzfs_dataset.c (Diff revision 2) Could you add a comment here explaining this math? The use of "SPA_OLD_BLOCKSIZE / tsize" is especially non-obvious. Explaining that the refreservation already takes into account this "raid-z deflation", but we need to essentially adjust the deflation ratio based on the actual block size.
Also you might mention that we are assuming all the data blocks will be this size, but that isn't actually the case if compression is enabled. But that is OK since compression won't make the space usage any worse.
You might also talk about how we're applying this to the total size (including both data blocks and indirect blocks). Since smaller blocks have worse RAID-Z space inflation, and the data blocks are (typically) larger than indirect blocks, and the data blocks are the vast majority of the space, this is all fine.
Change Summary:
Address feedback from Matt
Diff: |
Revision 3 (+517 -15)
|
---|
-
Just a few comments on the test files. Thank you.
-
Other tests will create flat files to use to back the zpool when specific configurations like this are required. Do you think we could do that here and in the next test instead of requiring the user to create/use a large number of disks?
If this test created its own backing files then we wouldn't have to worry as much about the 4k vs 512 sector size logic. On my machine zfs uses a 512 sector size for file-backed zpools.
Personally, I only run this test suite with three disks.
-
-
-
-
usr/src/test/zfs-tests/tests/functional/refreserv/refreserv_raidz.ksh (Diff revision 3) Should this be default_cleanup_noexit or default_cleanup instead?
-
usr/src/test/zfs-tests/tests/functional/refreserv/refreserv_raidz.ksh (Diff revision 3) Could use the TESTVOL variable here (from default.cfg).
Change Summary:
address Kody's feedback
Diff: |
Revision 4 (+517 -15)
|
---|
-
-
usr/src/lib/libzfs/common/libzfs_dataset.c (Diff revision 4) why not use parens here to be explicit and then remove the comment above?
-
-
usr/src/test/zfs-tests/tests/functional/refreserv/refreserv_raidz.ksh (Diff revision 4) personal preference is to use a switch here. Something like ``` case "$bps" in 512) allshifts=(9 10 17) maxpct=151 ;; 4096) allshifts=(12 13 17) maxpct=110 ;; *) log_fail "bytes/sector != (512 | 4096)" ;; esac
Change Summary:
Address Richard's feedback
Diff: |
Revision 5 (+523 -15)
|
---|