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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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 ;)