Re: [PATCH storage v2 0/7] add native ZFS over NVMe/TCP backend

["Max R. Carrara" <m.carrara@proxmox.com>]

On Sun Aug 2, 2026 at 5:31 AM CEST, Joaquin Varela wrote:
> Hi,
>
> this series adds an in-tree `zfsnvme` shared storage backend. It reuses the
> remote ZFS lifecycle model of ZFS over iSCSI, publishes zvols through Linux
> nvmet, and uses native NVMe multipath on PVE initiators.
>
> The ZFS dataset remains the source of truth for namespace identity. The
> backend stores NQN, NSID and UUID as ZFS user properties and reconstructs
> derived configfs state after a target restart. Target publication is atomic:
> all namespaces, Host NQN ACLs and authentication keys are restored before
> the subsystem is linked to its NVMe/TCP ports.
>
> This revision follows the feedback on the RFC:
>
> * new Perl modules use v5.36;
> * regular expressions are named constants using n/x and named captures;
> * module-private helpers use lexical subroutines where appropriate;
> * postfix dereferencing is used throughout;
> * no debian/changelog entries are included.
>
> It also makes the all-path-loss policy explicit, checks active users before
> teardown, and handles both the current activation-hints API and the short
> activation form used by cloud-init.
>
> Test target:
>
> * Ubuntu 24.04, kernel 6.8.0-136-generic;
> * ZFS 2.2.2, Linux nvmet, nvme-cli 2.8;
> * one ZFS pool with two independent 100 Mbit/s NVMe/TCP paths.
>
> Initiators were a two-node nested PVE 9.2 cluster running kernel
> 7.0.14-8-pve and nvme-cli 2.13. The test matrix covered thin provisioning
> and discard, snapshots and clones, concurrent allocation, ANA multipath,
> single/all-path faults, bidirectional live migration under guest I/O,
> watchdog fencing, live split-brain isolation, target reboot reconstruction,
> package upgrade protection, and a verified 45.7 GB mixed-I/O soak. No
> dual-writer interval was observed.
>
> The complete report, machine-readable results and raw logs are available at:
>
> https://github.com/joaquinv98/pve-storage/tree/feature/zfs-nvme-tcp
>
> The branch corresponding exactly to this series is:
>
> https://github.com/joaquinv98/pve-storage/tree/submission/zfs-nvme-tcp-rfc-v2
>
> This remains a lab-qualified release candidate; hardware qualification and a
> long-duration production-hardware soak are intentionally not claimed.
>
> Joaquin Varela (7):
>   zfs: make LUN provider dispatch overridable
>   zfs: add native NVMe/TCP storage backend
>   zfsnvme: harden node preflight and storage teardown
>   zfsnvme: make all-path loss policy explicit
>   zfsnvme: accept activation hints from storage API
>   zfsnvme: accept short volume activation calls
>   zfsnvme: restore ACLs before publishing target
>
>  debian/control                   |   1 +
>  src/PVE/Storage.pm               |   2 +
>  src/PVE/Storage/LunCmd/Makefile  |   2 +-
>  src/PVE/Storage/LunCmd/NVMET.pm  | 722 +++++++++++++++++++++++++
>  src/PVE/Storage/Makefile         |   1 +
>  src/PVE/Storage/Plugin.pm        |   2 +-
>  src/PVE/Storage/ZFSNVMePlugin.pm | 893 +++++++++++++++++++++++++++++++
>  src/PVE/Storage/ZFSPlugin.pm     |  45 +-
>  src/test/run_plugin_tests.pl     |   1 +
>  src/test/zfsnvme_test.pm         | 513 ++++++++++++++++++
>  10 files changed, 2158 insertions(+), 24 deletions(-)
>  create mode 100644 src/PVE/Storage/LunCmd/NVMET.pm
>  create mode 100644 src/PVE/Storage/ZFSNVMePlugin.pm
>  create mode 100644 src/test/zfsnvme_test.pm

Thank you very much for sending in this series and for wanting to tackle
this, it's much appreciated!

Some architectural comments -- it might be a lot to read / take in, but
that's mostly because I'm trying to give you as much context as you
might need.

1. I think the overall architecture of this plugin could be simplified a
   lot by inheriting from PVE::Storage::Plugin directly, instead of
   inheriting from PVE::Storage::ZFSPlugin.

   The inheritance chain currently looks like this:

   PVE::SectionConfig -> PVE::Storage::Plugin ->
   PVE::Storage::ZFSPoolPlugin -> PVE::Storage::ZFSPlugin ->
   PVE::Storage::ZFSNVMePlugin

   Maybe you have noticed this yourself already, it gets quite hard to
   keep track of things due to the amount of inheritance we are already
   using for the ZFS things... Adding another plugin in that chain adds
   more complexity in that regard. And Perl doesn't really make it easy
   to work with inheritance either ;)

   So, I think if you instead `use base qw(PVE::Storage::Plugin);`
   directly, it should be both easier for you to implement this plugin,
   and also for us to maintain. "Easier to maintain" here also implies
   that changes to e.g. ZFSPoolPlugin of ZFSPlugin don't accidentally
   introduce bugs or unwanted behavior in ZFSNVMePlugin.

   That being said, feel free to copy the helpers / implementations that
   your plugin needs from ZFSPoolPlugin and ZFSPlugin, and make them
   private subs (`my sub`). Code duplication is perfectly fine in this
   case IMO and doesn't hurt. Please just make sure that things related
   to naming volumes (in PVE) / zvols, parsing, etc. all stay the same
   -- as in, that the same formats are used as with the other ZFS
   plugins.

   Some additional context: I have actually long been planning to make
   ZFSPlugin inherit from PVE::Storage::Plugin directly as well. The
   same goes for LvmThinPlugin; it should not be inheriting from
   LVMPlugin anymore either. Reason for this is that we did actually
   have some regressions in LvmThinPlugin when something seemingly
   unrelated was changed in LVMPlugin. Inheritance always seems to make
   "perfect sense" at first, and then it comes back to bite you later
   on...

   So, that means that whenever I'm untangling ZFSPoolPlugin and
   ZFSPlugin from one another, I can make the helpers there freestanding
   functions in a separate module, e.g. PVE::Storage::Common::ZFS or
   something similar. The helpers you introduce in ZFSNVMePlugin would
   then also be merged / unified with the other ones, if that makes
   sense.

2. Since you're then inheriting from PVE::Storage::Plugin directly, you
   can also omit PVE::Storage::LunCmd::NVMET and instead have those
   helpers there in PVE::Storage::ZFSNVMePlugin directly (maybe with an
   `nvmet_` prefix or something).

3. The `$REMOTE_HELPER` in NVMET.pm should be Perl code instead of BASH,
   as in, the remote functions should all be Perl inside the plugin
   instead.

   Please try to only execute individual commands remotely there as much
   as possible, and do the whole parsing and other logic locally. I know
   that might make things more difficult to deal with and implement at
   first, but it goes a *long* way in terms of maintainability and
   testability later on. You already do a lot of parsing and other
   things really nicely in ZFSNVMePlugin.pm, so I think following a
   similar pattern for handling things on the remote host would be
   optimal.

   Also, the commands you execute (remotely or locally) should not
   contain any BASH-isms, since it's never *really* guaranteed that the
   (remote) server has BASH as default (or even installed in some rare
   cases).

   Finally, don't be afraid to call multiple `ssh ...` commands in
   sequence if it cannot be avoided when implementing this in Perl
   instead of BASH. IMO that's perfectly fine to do here.

The only other suggestion I would make is to replace some usages of Perl
builtins with other PVE-specific helpers and different modules:

- opendir / readdir / closedir --> PVE::File::dir_glob_foreach [glob-foreach]

- glob --> PVE::File::dir_glob_regex [glob-regex]

- IP::Socket::IP->new() --> PVE::Network::tcp_ping [tcp_ping]

- `my $fh = IO::File->new(); print $fh "...";` --> PVE::SysFSTools::file_write [file_write]

(Summarizing this here instead of as inline comments, since that might be
easier for you.)

Those three points are the biggest things that should be addressed IMO.
Otherwise, there is not really anything that I would change much.
I think the code quality is excellent overall, and it's really nice to
see that you added tests! :)

I'll leave you with this for now; the patches for pve-manager and
pve-docs look fine at a first glance. I'll have to see how we'll handle
the docs in particular, since it's been a little while since we
introduced a completely new plugin.

So, I would leave the docs as-is for now (unless something of your
implementation changes); if we need to format / adapt / change something
in particular, we'll follow up with our own patches there.


Overall, I'm quite happy with how this is turning out! I think once the
architectural things have been addressed, we can seriously consider
merging this. Fantastic work so far; thank you very much for your
contributions!


[glob-foreach]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/File.pm;h=7d0f7ab95dec2ef33744243d419e882d490e6218;hb=refs/heads/master#l275
[glob-regex]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/File.pm;h=7d0f7ab95dec2ef33744243d419e882d490e6218;hb=refs/heads/master#l260
[tcp-ping]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Network.pm;h=90a2bf13779e0fae6574bc4040718d0359ccfc57;hb=refs/heads/master#l713
[file-write]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/SysFSTools.pm;h=5d007027907b1c97d3d3742afe65bae17e1039e6;hb=refs/heads/master#l251