From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id B3A841FF09B for ; Mon, 28 Sep 2026 13:53:12 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 8FE53216E6; Mon, 28 Sep 2026 13:53:09 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 28 Sep 2026 13:53:03 +0200 Message-Id: To: "Joaquin Varela" , From: "Max R. Carrara" Subject: Re: [PATCH storage v2 0/7] add native ZFS over NVMe/TCP backend X-Mailer: aerc 0.18.2-0-ge037c095a049 References: In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790596383382 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.356 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 2UPRQURO4YTWJXQPE6CRUTRZPQGNB6CQ X-Message-ID-Hash: 2UPRQURO4YTWJXQPE6CRUTRZPQGNB6CQ X-MailFrom: m.carrara@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 t= he > remote ZFS lifecycle model of ZFS over iSCSI, publishes zvols through Lin= ux > 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 atom= ic: > 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 befo= re > 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 reconstructio= n, > 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-rf= c-v2 > > This remains a lab-qualified release candidate; hardware qualification an= d 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-foreac= h] - glob --> PVE::File::dir_glob_regex [glob-regex] - IP::Socket::IP->new() --> PVE::Network::tcp_ping [tcp_ping] - `my $fh =3D 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=3Dpve-common.git;a=3Dblob;f=3Dsr= c/PVE/File.pm;h=3D7d0f7ab95dec2ef33744243d419e882d490e6218;hb=3Drefs/heads/= master#l275 [glob-regex]: https://git.proxmox.com/?p=3Dpve-common.git;a=3Dblob;f=3Dsrc/= PVE/File.pm;h=3D7d0f7ab95dec2ef33744243d419e882d490e6218;hb=3Drefs/heads/ma= ster#l260 [tcp-ping]: https://git.proxmox.com/?p=3Dpve-common.git;a=3Dblob;f=3Dsrc/PV= E/Network.pm;h=3D90a2bf13779e0fae6574bc4040718d0359ccfc57;hb=3Drefs/heads/m= aster#l713 [file-write]: https://git.proxmox.com/?p=3Dpve-common.git;a=3Dblob;f=3Dsrc/= PVE/SysFSTools.pm;h=3D5d007027907b1c97d3d3742afe65bae17e1039e6;hb=3Drefs/he= ads/master#l251