On 11/7/19 12:59 PM, Fabian Grünbichler wrote:
On November 7, 2019 12:52 pm, Fabian Ebner wrote:On 11/7/19 9:34 AM, Fabian Grünbichler wrote:On November 6, 2019 1:46 pm, Fabian Ebner wrote:A new mountpoint property is added to the schema for ZFSPool storages. When needed for the first time, the current mount point is determined and written to the storage config.Signed-off-by: Fabian Ebner <f.eb...@proxmox.com> --- PVE/Storage/ZFSPoolPlugin.pm | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm index 16fb0d6..44c8123 100644 --- a/PVE/Storage/ZFSPoolPlugin.pm +++ b/PVE/Storage/ZFSPoolPlugin.pm @@ -32,6 +32,10 @@ sub properties { description => "use sparse volumes", type => 'boolean', }, + mountpoint => { + description => "mount point", + type => 'string', format => 'pve-storage-path', + }, }; }@@ -44,6 +48,7 @@ sub options {disable => { optional => 1 }, content => { optional => 1 }, bwlimit => { optional => 1 }, + mountpoint => { optional => 1 }, }; }@@ -148,11 +153,27 @@ sub path {my ($vtype, $name, $vmid) = $class->parse_volname($volname);my $path = '';+ my $mountpoint = $scfg->{mountpoint}; + + # automatically write mountpoint to storage config if it is not present + if (!$mountpoint) { + $mountpoint = $class->zfs_request($scfg, undef, 'get', '-o', 'value', + 'mountpoint', '-H', $scfg->{pool}); + chomp($mountpoint); + + eval { + PVE::Storage::lock_storage_config(sub { + my $cfg = PVE::Storage::config(); + $cfg->{ids}->{$storeid}->{mountpoint} = $mountpoint; + PVE::Storage::write_config($cfg); + }, "update storage config failed"); + }; + warn $@ if $@; + }if ($vtype eq "images") {if ($name =~ m/^subvol-/ || $name =~ m/^basevol-/) { - # fixme: we currently assume standard mount point?! - $path = "/$scfg->{pool}/$name"; + $path = "$mountpoint/$name"; } else { $path = "/dev/zvol/$scfg->{pool}/$name"; }it would be great if this "get mountpoint property of dataset" (or better: even "get arbitrary properties of dataset") helper could be factored out. we already have two calls to "zfs get -Hp '...' $dataset" that could re-use such a helper. then we could add a single check in activate_storage (maybe just for the branch where we actually just imported the pool?) to verify that the stored mountpoint value is correct, and if not update it (or warn about the problem). that check could have a very low timeout, and errors could be ignored since it is just a double-check. _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-develI noticed that the $scfg parameter in zfs_request is never used. Should I clean that up? Otherwise I'd need to include the $scfg parameter in the helper function as well, where it also isn't needed.it's used by ZFS over iSCSI, where the plugin is derived from ZFSPoolPlugin, so it's not safe to remove ;) _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
I see, thanks. I'll remember to check whether the method is virtual next time.
There are two problems with doing the mount point configuration in activate_storage. First, we might get called by 'create' (API for config triggered by 'pvesm add') which holds a lock on storage.cfg at that time, but we might also get called by something else without such a lock. Second, 'create' will write its own version of the config after calling activate_storage anyways.
It also doesn't help to only do the mount point configuration inside the branch where we just imported, since the pool/dataset might be imported already when we do 'pvesm add' and the second problem is still there.
Now there is plugin->on_add_hook used by 'create' which would also be a good place to configure the mount point. We could activate the storage there, check for the mount point and then hijack the scfg that is passed along to it (also not nice), so that it is written by 'create' later. In activate_storage we could still check whether configured and effective mount points match and warn if they don't.
I don't see a clean way to do the automatic adding of the mount point property (doing it in path() is possible, but we need to assume that our caller doesn't hold the lock on storage.cfg). Maybe it's better to just do the warning in activate_storage and leave it to the user to configure the mount point properly? It feels a bit weird for the plugin code to modify the config; is there an instance where it does?
_______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
