Large Block

[behlendorf]

Refreshed support for large blocks based on the OpenZFS implementing which is currently undergoing review and testing. This patch stack passes all automated testing and basic functional testing. This patch stack would benefit from additional manual testing. See the upstream Illumos review for additional details.

https://reviews.csiden.org/r/51/

[behlendorf]

This change is expected to be merged after the kmem-rework changes. I've tested it up to 8M block sizes and it has help together quite well.

[behlendorf]

Refreshed against the latest master, testing continues to go well. Changes include:

• Addition of the ZFS filesystem/snapshop patches which were omitted in the initial port. These aren't related to large block but pulling them in first minimizes merge conflicts.
• Merged zio_buf_cache and zio_data_buf_cache as a Linux optimization.
• Ztest patch to change the recordsize every 1s during operation to improve test coverage.
• Exported spa_blocksize()
• Xattrs stored in spill blocks limited to 128K for now.

[bjin]

I'm testing this feature, but find that when I set zfs set recordsize=1m zroot, zfs complains with message internal error: Numerical argument out of domain. But settings zfs set recordsize=1m zroot/home works fine as showed by zfs get recordsize. Is there any reason regarding this? Is it intended?

[behlendorf]

@bjin Thanks for the additional testing. I'm not able to case there issue you're describing with the current patch stack. Could you perhaps rebuild everything cleanly to verify what you're seeing.

[bjin]

Looks like it's due to zroot being flagged as bootfs in zpool properties, so I guess it's intended behavior. (Although the error message is bit weird)

[behlendorf]

@bjin Right, that makes sense. When bootfs is set it's inadvisable to enable this because utilities such as GRUB would need to be updated to understand the larger blocks. You're right we could use a better error message. Aside from this have you noticed any issues?

[bjin]

Everything works fine. It's not an exhaustive filesystem testing, but I run several network application (w/ mostly random disk IO) for hours without issues.

[behlendorf]

Thus far I've observed the following two ASSERTS while stress testing these changes. It's not 100% clear that the large block patches introduced these problems, however thus far I haven't been able to reproduce them on master. Before this work can be merged these fails needs to be resolved.

VERIFY(((((zio->io_bp)->blk_prop) >> (62)) & ((1ULL << (1)) - 1))) failed
PANIC at arc.c:3828:arc_write_done()

VERIFY(0) failed
PANIC at dnode_sync.c:621:dnode_sync()

For the above dnode_sync() case the specific values for this failure were 1 > 1 || 0 || 0 || 266240 == 335872.

[ahrens]

@behlendorf regarding the 2 assertion failures you saw, the 1st, in arc_write_done(), might be fixed by removing these 2 lines from zio_write_bp_init():

-        zio->io_bp_override = NULL;
-        BP_ZERO(bp);

We saw a similar failure and that was the fix. Note that this is unrelated to large block support.

[bjin]

@behlendorf Is that possible to merge this into 0.6.4 if these two assertion failure are proven to be unrelated to changes from this pull request? I'm planing creating a large raidz array with 4k sector disks, and large block feature will help avoiding space overhead and improving overall performance.

[behlendorf]

@ahrens thanks for the hint!

@bjin before I can merge the large block changes we need to explain these assertion failures. It sounds like the arc_write_done() one is a long standing issue which we can address in another patch.
But the dnode_sync() one in particular needs to be explained because it indicates the block pointer being written isn't valid. I don't see how the large block changes could have caused this, and it's certainly possible this isn't a new regression but we need to find out.

The only way I've been able to reproduce either of these issues (and it isn't easy) are with some stress tests put together by @nedbass. If you can hit the dnode_sync() issue using just the master branch then we could be confident that these changes aren't introducing this bug.

https://github.com/nedbass/zfsstress

[sempervictus]

I have a feature request for this PR: to allow users to use bootfs (rpool) pools with large blocks when GRUB isn't interacting with ZFS directly.

Aside from systems converted from FreeBSD, we dont have anything in our environment, or client environments, thats sitting on physical disks - most everything we do in production is beholden to some compliance regulations and we have to encrypt the substrate (since Oracle refuses to return any response on inquiries about the legal state of the possible leak or willful disclosure of what Rogue keeps updating so graciously). Since we're using dm-crypt like this, the /boot partitions are not ZFS anyway, and initrd handles loading the zpool after decrypting the underlying volumes.

Until the OpenZFS community figures out how we want to deal with ZFS-native cryptography, key management, and all the fun alongside those pieces, we're going to have to keep doing this to ensure cross platform compat and physical security, while the pools are only growing. While most of the dedicated storage platforms we build do not use an rpool, we're starting to do a reasonable number of application and intermediate systems on ZFS which benefit from a rational implementation of OS snapshots (without the BTRFS "feature" of almost guaranteed data loss or LVM's allocation requirements and shaky restores), and it really wouldn't hurt to have ARC cache appropriate OS blocks in MFU on storage systems since the OS volumes are often encrypted USB sticks/flash cards.

[behlendorf]

@kpande absolutely, refreshed against master. This patch is expected to be merged right after the tag. The test failures which were observed above have been resolved in master and were unrelated to this change. I've dropped the patch to consolidate the data and meta data caches from this patch stack because this change and the ABD patches are both expected to be in the next tag making it unnecessary.

[behlendorf]

@kpande That's not going to be easy until one of them gets merged and the other rebased on to it. For now you may need to just test one or the other.

[behlendorf]

Refreshed with the 128K limit on ZVOLs removed. Multiple people have requested that this limit be removed since it was somewhat arbitrary. It's still not 100% clear that blocks larger than 128K are a good idea for ZVOLs due to the read-modify-write overhead but removing the limit at least allows us to test it.

[behlendorf]

@kpande Thanks for the additional testing and sign-off. I hope to get this work finalized and merged next week.

[avg-I]

Please be aware of this issue https://www.illumos.org/issues/5393 when you import large blocks support.

[behlendorf]

@avg-I thanks for calling that additional patch out. We'll merge it at the same time as these changes. Out of curiosity do you know of any other issues with the large block patches? I ask because I may have observed issues when testing with block sizes over 1MB.

[avg-I]

@behlendorf I haven't actually used larger blocks yet, just had the feature enabled.

[behlendorf]

Refreshed against master and replaced by #3363.