Allow rewrite skip cloned and snapshotted blocks#18179
Allow rewrite skip cloned and snapshotted blocks#18179amotin wants to merge 1 commit intoopenzfs:masterfrom
Conversation
amotin
force-pushed
the
rewrite_cs
branch
2 times, most recently
from
e889c3f to
5c3b905
Compare
robn
left a comment
robn
left a comment
There was a problem hiding this comment.
This works for me! Some small thoughts in comments, but nothing to stop it I think.
| EXPORT_SYMBOL(dmu_objset_projectquota_present); | ||
| EXPORT_SYMBOL(dmu_objset_projectquota_upgradable); | ||
| EXPORT_SYMBOL(dmu_objset_id_quota_upgrade); | ||
| EXPORT_SYMBOL(dmu_objset_block_is_shared); |
There was a problem hiding this comment.
It hardly matters, but should we wait on exporting it until something external needs it?
There was a problem hiding this comment.
I am open to any formal recommendations. Last I heard this is not needed any more, so I'd be happy if we remove all of them at once.
There was a problem hiding this comment.
Some of them are needed by Lustre. Tony has PR #18161 open which updates the CI to include a test Lustre build so we don't accidentally break things. Once that's in place, I think it does make a lot of sense to whittle these down to what they actually need (and maybe a few interfaces they should switch too).
tests/zfs-tests/tests/functional/cli_root/zfs_rewrite/zfs_rewrite_skip_clone.ksh
Outdated
Show resolved
Hide resolved
| * Returns B_TRUE if block should be skipped. | ||
| */ | ||
| static boolean_t | ||
| zfs_rewrite_skip(dmu_buf_t *db, objset_t *os, uint64_t flags) |
There was a problem hiding this comment.
I'm wondering if we need to consider if rewriting would increase the number of DVAs before skipping (ie, they rewrite after increasing copies=).
If the user is increasing copies=, then they're presumably rewriting to try an improve their redundancy. Is the intent of "skip" clear in that case? In particular brt_maybe_exists() is allowed to return true if the block is not actually cloned at all. So, we might actually skip a rewrite on a non-cloned block that would have actually increased the redundancy on the block.
Personally I think its too fuzzy and weird a case to bother doing much about, but I might put something in the manpage that says the skip flags are advisory and ultimately ZFS will decide whether or not to rewrite a "skipped" block. And, possibly a separate thing to say "maybe don't skip if you're changing copies=". I mean, is anyone really changing copies= anyway? Yes, but not often surely!
There was a problem hiding this comment.
I was (and still am) thinking about adding an option to make rewrite conditional on properties match. But while for copies and checksum it is easy, for some others it is not. So I haven't decided yet.
Meanwhile, if somebody requests to not rewrite snapshots or clones, I am not sure there should be high expectations already.
Rewrite of cloned and snapshotted blocks can allocate additional space, that may be undesired. In some cases it may have sense to still rewrite snapshotted blocks, expecting the snapshots to rotate with time, freeing space. In other cases rewrite of cloned blocks may be acceptable, despite persistent space usage increase. For this reason add them as separate flags to `zfs rewrite`. Signed-off-by: Alexander Motin <[email protected]>
4 participants
Rewrite of cloned and snapshotted blocks can allocate additional space, that may be undesired. In some cases it may have sense to still rewrite snapshotted blocks, expecting the snapshots to rotate with time, freeing space. In other cases rewrite of cloned blocks may be acceptable, despite persistent space usage increase. For this reason add them as separate flags to
zfs rewrite.Types of changes
Checklist:
Signed-off-by.