[pve-devel] [PATCH storage 1/2] zfs: only use cache when listing images locally

[pve-devel] [PATCH storage 1/2] zfs: only use cache when listing images locally

[Fiona Ebner]

The plugin for remote ZFS storages currently also uses the same
list_images() as the plugin for local ZFS storages. The issue with
this is that there is only one cache which does not remember the
target host where the information originated.

Simply restrict the cache to be used for the local ZFS plugin only. An
alternative solution would be to use a cache for each target host, but
that seems a bit more involved and could still be added in the future.

For example, issues can happen during rescan:
1. Via list_images() and zfs_list_zvol(), ZFSPlugin.pm's zfs_request()
   is executed for a remote ZFS.
2. $cache->{zfs} is set to the result of scanning the images there.
3. Another list_images() for a local ZFS storage happens and uses the
   cache with the wrong information.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/ZFSPlugin.pm     | 7 +++++++
 PVE/Storage/ZFSPoolPlugin.pm | 8 +++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage/ZFSPlugin.pm b/PVE/Storage/ZFSPlugin.pm
index d4dc2a4..1379bcf 100644
--- a/PVE/Storage/ZFSPlugin.pm
+++ b/PVE/Storage/ZFSPlugin.pm
@@ -419,4 +419,11 @@ sub deactivate_volume {
     return 1;
 }
 
+sub list_images {
+    my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
+
+    my $zfs_list = $class->zfs_list_zvol($scfg);
+    return $class->zfs_list_images($storeid, $scfg, $vmid, $vollist, $zfs_list);
+}
+
 1;
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index e264fde..0fa18ce 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -255,10 +255,16 @@ sub list_images {
     my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
 
     $cache->{zfs} = $class->zfs_list_zvol($scfg) if !$cache->{zfs};
+    return $class->zfs_list_images($storeid, $scfg, $vmid, $vollist, $cache->{zfs});
+}
+
+sub zfs_list_images {
+    my ($class, $storeid, $scfg, $vmid, $vollist, $zfs_list) = @_;
+
     my $zfspool = $scfg->{pool};
     my $res = [];
 
-    if (my $dat = $cache->{zfs}->{$zfspool}) {
+    if (my $dat = $zfs_list->{$zfspool}) {
 
 	foreach my $image (keys %$dat) {
 
-- 
2.30.2

[pve-devel] [PATCH storage 2/2] zfs: list zvol: list single pool only for remote ZFS

[Fiona Ebner]

The remote ZFS plugin doesn't use a cache (would need a separate cache
for each target host), so this can speed up the operation.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/ZFSPlugin.pm     | 2 +-
 PVE/Storage/ZFSPoolPlugin.pm | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/PVE/Storage/ZFSPlugin.pm b/PVE/Storage/ZFSPlugin.pm
index 1379bcf..cd5a3b6 100644
--- a/PVE/Storage/ZFSPlugin.pm
+++ b/PVE/Storage/ZFSPlugin.pm
@@ -422,7 +422,7 @@ sub deactivate_volume {
 sub list_images {
     my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
 
-    my $zfs_list = $class->zfs_list_zvol($scfg);
+    my $zfs_list = $class->zfs_list_zvol($scfg, 1);
     return $class->zfs_list_images($storeid, $scfg, $vmid, $vollist, $zfs_list);
 }
 
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index 0fa18ce..2f75fba 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -379,9 +379,11 @@ sub zfs_delete_zvol {
 }
 
 sub zfs_list_zvol {
-    my ($class, $scfg) = @_;
+    my ($class, $scfg, $single_pool) = @_;
 
-    my $text = $class->zfs_request($scfg, 10, 'list', '-o', 'name,volsize,origin,type,refquota', '-t', 'volume,filesystem', '-Hrp');
+    my @param = ('-o', 'name,volsize,origin,type,refquota', '-t', 'volume,filesystem', '-Hrp');
+    push @param, $scfg->{pool} if $single_pool;
+    my $text = $class->zfs_request($scfg, 10, 'list', @param);
     my $zvols = zfs_parse_zvol_list($text);
     return undef if !$zvols;
 
-- 
2.30.2

Re: [pve-devel] [PATCH storage 1/2] zfs: only use cache when listing images locally

[Fabian Grünbichler]

On November 7, 2022 12:00 pm, Fiona Ebner wrote:
> The plugin for remote ZFS storages currently also uses the same
> list_images() as the plugin for local ZFS storages. The issue with
> this is that there is only one cache which does not remember the
> target host where the information originated.
> 
> Simply restrict the cache to be used for the local ZFS plugin only. An
> alternative solution would be to use a cache for each target host, but
> that seems a bit more involved and could still be added in the future.

wouldn't it be sufficient to just do

$cache->{zfs}->{$storeid}

when filling/querying the cache, and combining that with *always* listing only
the storage-relevant pool?

the only case where we actually benefit from listing *all* zfs volumes/datasets
is when
- there are multiple storages configured referencing overlapping parts of the
ZFS hierarchy
- vdisk_list is called with a volume_list with multiple such storages being part
of the set, or with $vmid but no $storeid (rescan, or purging unreferenced guest
disks on guest removal)

in practice, it likely doesn't make much difference since ZFS should cache the
metadata for the overlapping parts in memory anyway (given that we'd then call
'zfs list' in a loop with different starting points).

whereas, for most regular cases listing happens without a cache anyway (or with
a cache, but only a single storage involved), so there is no benefit in querying
volumes belonging to other storages since we are not interested in them anyway.

sidenote: it seems like vdisk_list's volume_list is not used anywhere as parameter?

> 
> For example, issues can happen during rescan:
> 1. Via list_images() and zfs_list_zvol(), ZFSPlugin.pm's zfs_request()
>    is executed for a remote ZFS.
> 2. $cache->{zfs} is set to the result of scanning the images there.
> 3. Another list_images() for a local ZFS storage happens and uses the
>    cache with the wrong information.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [pve-devel] [PATCH storage 1/2] zfs: only use cache when listing images locally

[Fiona Ebner]

Am 16.11.22 um 12:18 schrieb Fabian Grünbichler:
> On November 7, 2022 12:00 pm, Fiona Ebner wrote:
>> The plugin for remote ZFS storages currently also uses the same
>> list_images() as the plugin for local ZFS storages. The issue with
>> this is that there is only one cache which does not remember the
>> target host where the information originated.
>>
>> Simply restrict the cache to be used for the local ZFS plugin only. An
>> alternative solution would be to use a cache for each target host, but
>> that seems a bit more involved and could still be added in the future.
> 
> wouldn't it be sufficient to just do
> 
> $cache->{zfs}->{$storeid}
> 
> when filling/querying the cache, and combining that with *always* listing only
> the storage-relevant pool?

Yes, should work. I'll send a v2 with that.

> 
> the only case where we actually benefit from listing *all* zfs volumes/datasets
> is when
> - there are multiple storages configured referencing overlapping parts of the
> ZFS hierarchy
> - vdisk_list is called with a volume_list with multiple such storages being part
> of the set, or with $vmid but no $storeid (rescan, or purging unreferenced guest
> disks on guest removal)

The cache is already useful if there two ZFS storages, nothing as fancy
as the above needed ;) Then for rescan and others which iterate all
storages, only one zfs list call is issued, rather than one for each ZFS
storage.

> 
> in practice, it likely doesn't make much difference since ZFS should cache the
> metadata for the overlapping parts in memory anyway (given that we'd then call
> 'zfs list' in a loop with different starting points).
> 
> whereas, for most regular cases listing happens without a cache anyway (or with
> a cache, but only a single storage involved), so there is no benefit in querying
> volumes belonging to other storages since we are not interested in them anyway.
> 

Yes, I'd also guess that in practice the benefit of the current list-all
cache is rather limited.

> sidenote: it seems like vdisk_list's volume_list is not used anywhere as parameter?
> 

Seems to be only used in the ZFS tests in pve-storage ;)

Re: [pve-devel] [PATCH storage 1/2] zfs: only use cache when listing images locally

[Fiona Ebner]

Am 16.11.22 um 14:30 schrieb Fiona Ebner:
> Am 16.11.22 um 12:18 schrieb Fabian Grünbichler:
>> On November 7, 2022 12:00 pm, Fiona Ebner wrote:
>>> The plugin for remote ZFS storages currently also uses the same
>>> list_images() as the plugin for local ZFS storages. The issue with
>>> this is that there is only one cache which does not remember the
>>> target host where the information originated.
>>>
>>> Simply restrict the cache to be used for the local ZFS plugin only. An
>>> alternative solution would be to use a cache for each target host, but
>>> that seems a bit more involved and could still be added in the future.
>>
>> wouldn't it be sufficient to just do
>>
>> $cache->{zfs}->{$storeid}
>>
>> when filling/querying the cache, and combining that with *always* listing only
>> the storage-relevant pool?
> 
> Yes, should work. I'll send a v2 with that.
> 

Well, a $storeid-based cache would be useless for both existing callers
using a cache parameter (pvesr's prepare_local_job and Storage.pm's
vdisk_list), because they iterate over the storages once for a given cache.

Should I get rid of the cache here entirely or do we go with this series
after all?

Also, I guess pvesr should switch to using Storage.pm's interface,
rather than talk to the plugin directly.

Re: [pve-devel] [PATCH storage 1/2] zfs: only use cache when listing images locally

[Fabian Grünbichler]

On November 28, 2022 1:19 pm, Fiona Ebner wrote:
> Am 16.11.22 um 14:30 schrieb Fiona Ebner:
>> Am 16.11.22 um 12:18 schrieb Fabian Grünbichler:
>>> On November 7, 2022 12:00 pm, Fiona Ebner wrote:
>>>> The plugin for remote ZFS storages currently also uses the same
>>>> list_images() as the plugin for local ZFS storages. The issue with
>>>> this is that there is only one cache which does not remember the
>>>> target host where the information originated.
>>>>
>>>> Simply restrict the cache to be used for the local ZFS plugin only. An
>>>> alternative solution would be to use a cache for each target host, but
>>>> that seems a bit more involved and could still be added in the future.
>>>
>>> wouldn't it be sufficient to just do
>>>
>>> $cache->{zfs}->{$storeid}
>>>
>>> when filling/querying the cache, and combining that with *always* listing only
>>> the storage-relevant pool?
>> 
>> Yes, should work. I'll send a v2 with that.
>> 
> 
> Well, a $storeid-based cache would be useless for both existing callers
> using a cache parameter (pvesr's prepare_local_job and Storage.pm's
> vdisk_list), because they iterate over the storages once for a given cache.

the vdisk_list one is one-off actions only anyway, so there no cache or possibly
more overhead for multiple run_command executions is not that bad (compared to
the overhead of querying multiple pools while only being interested in one!).

for replication, I guess in most cases this will be a single storage anyway,
unless we frequently pass in unrelated storages for scanning?

> Should I get rid of the cache here entirely or do we go with this series
> after all?

judgment call I guess ;)

> Also, I guess pvesr should switch to using Storage.pm's interface,
> rather than talk to the plugin directly.

yes. this should be a general rule (which is broken in some parts of our code
atm) -> PVE::Storage is the interface to talk to storages, and that (and some
other code in pve-storage) uses the plugins. might require some re-working
though, but with other storage types up for replication in the near future
putting some (more) parts behind an abstraction in pve-storage if needed seems
sensible anyway ;)