From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 867931FF17A for ; Tue, 28 Oct 2025 17:34:41 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 339221FF83; Tue, 28 Oct 2025 17:35:11 +0100 (CET) Message-ID: Date: Tue, 28 Oct 2025 17:35:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Fiona Ebner , Proxmox VE development discussion References: <20251024122705.93761-1-f.weber@proxmox.com> <85dab76c-d461-43d1-8f1c-45ea1b31200a@proxmox.com> Content-Language: en-US From: Friedrich Weber In-Reply-To: <85dab76c-d461-43d1-8f1c-45ea1b31200a@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1761669294485 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.012 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [RFC qemu-server/storage 0/3] fix #5779: introduce guest hints to pass rxbounce flag to KRBD X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Fiona and I discussed this off-list (thanks again!), to sum up: - even though there is currently only one hint, we'd like a solution to that still allows to add more hints later on but doesn't generate too much churn now - passing the OS type to enable rxbounce is only relevant at a few callsites, e.g. when activating on (Windows) VM start, not when e.g. activating for offline-cloning. So it would be nice to avoid touching all activate_volumes callsites now - it's likely that other hypothetical hints we might add in the future will be relevant only for VM start as well, but if this assumption turns out to be false, we should be able to adapt without too much hassle. - rxbounce cannot be applied to already-mapped RBD volumes without unmapping them first, but hypothetical future flags (that are set based on guest hints) for other storage plugins might be applicable to already-mapped volumes without unmapping, so we should be able to allow this in the future without too much hassle. - when starting a VM, if a volume is already active without rxbounce, unmapping and re-mapping it (with rxbounce) should be safe [0], but it might not be safe for all callsites - so for now, we can go for a variant of Fiona's suggestion 1 here [1] where: * for the base functionality, we pass the hints only at the {activate,map}_volumes callsites that are relevant for rxbounce (essentially, on VM start). This avoids having to modify all callsites now. If a hypothetical future plugin needs hints also at other callsites, we can still pass the storage hints there * we define a boolean hint 'plugin-may-deactivate' that, if true, tells the plugin that it is free to unmap and a re-map volume if necessary to make use of a hint. If 'plugin-may-deactivate' is 0, the plugin must not unmap+map to apply a hint -- if it can apply a hint to an already-mapped volume without unmap+map, it can still do so. On VM start, we pass 1 for this flag. This way, passing flags at more callsites in the future is doable (if unmap+map isn't safe, we just pass 0). And we keep the door open for hints that plugins can apply without unmap+map. * RBDPlugin::map_volume checks whether an already-mapped volume has rxbounce set in sysfs, and if it hasn't but should (according to the hints) and plugin-may-deactivate is 1, it can unmap and map the volume with rxbounce. * this can result in unnecessary map (without rxbounce)+unmap+map (with rxbounce) sequences e.g. in case of live restore, which is not optimal but also not really problematic. In case of live restore we can probably fix this by passing the storage hints already on the first activation, which can be done on top of the base functionality in v1 or (worst case) as a follow-up. * if a hypothetical future hint+plugin combination actually runs into the problem that a hint was not passed on initial activation, is passed later but cannot be applied anymore then, we still have the option of adding storage hints to more/all activate_volumes callsites later on. * Fiona pointed out a problem with volumes that already active after creation [2]: depending on the hints and the plugin, in such a case the plugin may be able to apply the hint directly or unmap+map to do so. If both doesn't work, we can still think about adding hints also to alloc_image (which would be backwards-compatible). * the hints applying to all volumes passed to activate_volumes is not great as Fiona also mentioned [2], and having an alternative API that allows per-volume hint or even a helper class might be nicer, but since this is kind of orthogonal I won't cover this in the v1. Having such an alternative API might still be nice in the future (also because if we ever need more hints, TPM state and EFI disk are likely candidates, and then, per-volume hints would make sense). Implementing this wouldn't change the plugin API. * we won't add hints to {deactivate,unmap}_volume for now -- they're not relevant for rxbounce, and possibly not relevant for future hints either -- and even if they are, the plugin may be better off to track relevant state on its own, because hypothetical hint X might even change between volume activation and deactivation. If we still need to add this, it can be done in a backwards-compatible way. I'll follow up with a v1. [0] https://lore.proxmox.com/pve-devel/59d16447-5f20-4a38-a7b0-fa4890174440@proxmox.com/ [1] https://lore.proxmox.com/pve-devel/d182c102-204b-43f0-9336-d6faf754fe72@proxmox.com/ [2] https://lore.proxmox.com/pve-devel/85dab76c-d461-43d1-8f1c-45ea1b31200a@proxmox.com/ _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel