FreeBSD: Add zfs jail property #17768
Conversation
|
under illumos based platform we have |
ihoro
force-pushed
the
0064-zfs-jail-prop
branch
from
9a8e5cb to
a5e0913
Compare
Thanks for taking this moment into consideration. Indeed, the cross-platform term in the codebase is |
man/man7/zfsprops.7
Outdated
| This read-only property reports the name of the jail that mounted the jailed | ||
| dataset. | ||
| The "0" name is used for datasets that are not mounted or not jailed. | ||
| If a jail is renamed, the property will still report its old name from |
There was a problem hiding this comment.
We should mention the reasoning for "0". What do you think of?:
The "0" name is used for datasets that are not mounted or not jailed.
+ This differs from the normal ZFS convention to print dash ('-') for unset values,
+ since '-' can be a valid jail name.There was a problem hiding this comment.
This is a great idea. Fixed, thanks.
"jail": {
"value": "0",
"source": {
"type": "LOCAL",
"data": "-"
}
}I would have expected
|
module/os/freebsd/zfs/zfs_vfsops.c
Outdated
| zfsvfs->z_os->os_dsl_dataset->ds_jailname = | ||
| kmem_zalloc(strlen(pr->pr_name) + 1, KM_SLEEP); | ||
| strcpy(zfsvfs->z_os->os_dsl_dataset->ds_jailname, | ||
| pr->pr_name); |
There was a problem hiding this comment.
You can use kmem_strdup(pr->pr_name); to simplify this.
module/zfs/dsl_prop.c
Outdated
| (ds->ds_jailname && INGLOBALZONE(curproc)) ? | ||
| ds->ds_jailname : "0")); | ||
| VERIFY0(nvlist_add_string(propval, ZPROP_SOURCE, setpoint)); | ||
| VERIFY0(nvlist_add_nvlist(*nvp, "jail", propval)); |
There was a problem hiding this comment.
The fnvlist_* wrappers should be used here, or better yet add the required error handling even if they're almost certain never to fail.
There was a problem hiding this comment.
Sure. It seems I followed the examples from dsl_prop_get_all_impl() itself. Migrated to fnvlist_*, thanks.
A read-only property to report name of the jail that mounted the dataset. Sponsored-by: SkunkWerks, GmbH Signed-off-by: Igor Ostapenko <[email protected]>
ihoro
force-pushed
the
0064-zfs-jail-prop
branch
from
a5e0913 to
607e4ed
Compare
Fixed.
Sure, especially that existing test provides a working baseline. I've added a separate test not to mix multiple concerns into a single one. |
| If the jail is renamed, the property will still report its old name from | ||
| the time the dataset was mounted. |
There was a problem hiding this comment.
It makes me wonder if reporting jail ID would be more predictable.
| char ds_snapname[ZFS_MAX_DATASET_NAME_LEN]; | ||
|
|
||
| #ifdef __FreeBSD__ | ||
| char *ds_jailname; |
There was a problem hiding this comment.
I am not familiar with zones, but wouldn't this functionality apply there too? Would some ds_zonename without the ifdef be more universal?
| struct prison *pr = curthread->td_ucred->cr_prison; | ||
| if (pr != &prison0) { | ||
| zfsvfs->z_os->os_dsl_dataset->ds_jailname = | ||
| kmem_strdup(pr->pr_name); |
There was a problem hiding this comment.
Again, I am not closely familiar with the zone/jail code in ZFS, but I am not sure the delegation should necessarily be tied to a mounting. I suppose some dataset may not be mounted, but be controlled by the jail. Am I wrong?
|
Refreshing the context. Option1On the October's monthly call we discussed why it does the trick on mounting/unmounting. The reason is one-to-many relation we have today, i.e. we can call Option2There was a nice discussion with Pawel, where it's found that initial implementation had no intent to support one-to-many. Probably, that's just an outcome from the absence of explicit check against such cases. It seems we could rework the mechanism to one-to-one only. But it's unknown if it's going to break someone's existing logic on top of that, e.g. right now it's okay to change delegation by a secondary I've tried to check the things for this option and it does not seem to be trivial. The first task is to have an extra check upon
Option comparisonI've tried to compare these options: My intuition for Option1. Probably, listing the jails we delegated to should not be a property and could be provided with something like My intuition for Option2. If one-to-many has no actual use and limiting it to one-to-one does not break existing things then this option covers all the needs by saying whom we delegated to. And we always have RFCThis is a request for comments and suggestions. Probably, I do not see other possible options. I wanted to present already implemented Option2 to compare the actual code and approaches, but now it feels better to discuss them before diving into extra code complexities. |
5 participants
Sponsored-by: SkunkWerks, GmbH
Motivation and Context
It targets #15710 feature request to add a read-only property to report name of the jail that mounted the dataset.
Description
The decisions made:
0name is used for datasets that are not mounted or not jailed. The default zfs style is to use-for a missing value, but it's allowed to have a jail named like that. At the same time there is no way to name a jail as0:jail: name cannot be numeric (unless it is the jid).allprison_lock) to lookup jail name by jid does not feel good for the cases with many jailed datasets and/or jails.0. Such way it covers the requirement not to reveal jail name to the jail itself.zfsprops.7man page was extended to outline the property. It is mentioned within the section of tunables even though it is a read-only one, it seems to be better to keep it closer to thejailedproperty.Hence, the mechanism is simple:
zfs_domount()cachespr_namezfs_umount()clears the nameThe proposed implementation covers usual cases without extra complexity and property retrieval performance impact. The opportunities for improvements are left for the future.
How Has This Been Tested?
Originally the patch was created months ago for 15-CURRENT, it was re-tested for the latest 16-CURRENT. The UI/UX demonstration:
Types of changes
Checklist:
Signed-off-by.