public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command()
Date: Mon, 16 Jun 2025 15:15:24 +0200 (CEST)	[thread overview]
Message-ID: <386024326.3481.1750079724005@webmail.proxmox.com> (raw)
In-Reply-To: <f415f46b-e6ed-40fe-a22c-bf3d67c6f5a4@proxmox.com>


> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 14:57 CEST geschrieben:
> 
>  
> Am 16.06.25 um 14:46 schrieb Fabian Grünbichler:
> >> Fiona Ebner <f.ebner@proxmox.com> hat am 16.06.2025 13:51 CEST geschrieben:
> >> Am 16.06.25 um 13:31 schrieb DERUMIER, Alexandre via pve-devel:
> >>>>> With '-blockdev', it is necessary to activate the volumes to generate
> >>>>> the command line, because it can be necessary to check whether the
> >>>>> volume is a block device or a regular file.
> >>>
> >>> I was thinking about that, but do we have storage with activate_volume
> >>> need to be done for a regular file ?
> >>>
> >>> for lvm plugin for example, we could return always driver=>host_device.
> >>>
> >>> activate_volume is always done in specific plugin, so the plugin should
> >>> be able to tell if it's a block or file storage
> >>>
> >>> only custom path passthrough in vm configuration need to be checked if
> >>> it's a file or device, but we don't have activate_volume anyway
> >>
> >> Good point! It is needed for the default implementation of
> >> qemu_blockdev_options() in Plugin.pm. We could go ahead and have all
> >> plugins use their own implementation of qemu_blockdev_options(), that
> >> would avoid doing any actual IO there. That sounds good to me.
> >>
> >> The downside is that it would break third-party plugins that would
> >> otherwise work fine with the default implementation. But actually, it's
> >> only those that need to do something in activate_volume() to make the
> >> volume accessible. Maybe requiring those to provide their own
> >> qemu_blockdev_options() implementation is an acceptable level of
> >> breakage, not entirely sure. We could even try and just guess in the
> >> default implementation if we cannot access the volume, maybe based on
> >> the presence of 'path'.
> >>
> >> @Fabian does that sound acceptable to you?
> > 
> > not sure I follow completely, to avoid misunderstandings:
> > 
> > variant A:
> > 
> > move activate_volume up front, no breakage, but more churn and higher 
> > chance of leaving activated volumes around in case starting fails
> 
> It's not "no breakage". Plugins that are not happy with the default
> implementation of qemu_blockdev_options() would still fail.

yes. what I meant is no breakage caused by file vs host_device, since if
we activate up front, we can query the returned path which should exist
at that point, and make an informed decision. of course, if a storage
requires a different driver altogether and/or doesn't return an actual
path that we can query, then its plugin needs adaptation in any case.

> 
> > variant B:
> > 
> > move decision which driver to use to plugin via qemu_blockdev_options
> 
> Yes.
> 
> > how would the default/fallback look like here? without activation, we
> > can't rely on path since that doesn't currently require being activated,
> > so we can't use it to distinguish block devices from files prior to
> > activation..
> 
> As written above, we could still try to probe the file like we currently
> do. If that works, great, we don't lose anything compared to variant A.
> If that fails, we look at whether path is set. If yes, then assume it's
> a file-based storage and use the 'file' driver. Otherwise, use the
> 'host_device' driver.

but this assumption will be wrong half the time? if we don't activate
before deciding the driver, then we can only guess that a given path
is a file, and not a block device? if we have to activate before guessing,
then we are back at variant A (or at a mixed variant)?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

  reply	other threads:[~2025-06-16 13:15 UTC|newest]

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-12 14:02 [pve-devel] [PATCH-SERIES qemu-server 00/22] preparation for switch to blockdev Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 01/22] drive: code cleanup: drop unused $vmid parameter from get_path_and_format() Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 02/22] cfg2cmd: require at least QEMU binary version 6.0 Fiona Ebner
2025-06-13  9:18   ` Fabian Grünbichler
2025-06-16  8:47     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 03/22] drive: parse: use hash argument for optional parameters Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 04/22] drive: parse drive: support parsing with additional properties Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 05/22] restore: parse drive " Fiona Ebner
2025-06-13  9:30   ` Fabian Grünbichler
2025-06-16  8:51     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 06/22] drive: remove geometry options gone since QEMU 3.1 Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 07/22] clone disk: io uring check: fix call to determine cache direct Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 08/22] drive: move storage_allows_io_uring_default() and drive_uses_cache_direct() helpers to drive module Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 09/22] drive: introduce aio_cmdline_option() helper Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 10/22] drive: introduce detect_zeroes_cmdline_option() helper Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 11/22] introduce StateFile module for state file related helpers Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 12/22] vm start: move state file handling to dedicated module Fiona Ebner
2025-06-13 10:00   ` Fabian Grünbichler
2025-06-16 10:34     ` Fiona Ebner
2025-06-16 10:54       ` Fabian Grünbichler
2025-06-16 10:57         ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 13/22] vm start: move config_to_command() call further down Fiona Ebner
2025-06-13 10:05   ` Fabian Grünbichler
2025-06-16 10:54     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 14/22] vm start/commandline: also clean up pci reservation when config_to_command() fails Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command() Fiona Ebner
2025-06-13 10:16   ` Fabian Grünbichler
2025-06-17  7:46     ` Fiona Ebner
2025-06-13 10:25   ` Fabian Grünbichler
2025-06-16 11:31   ` DERUMIER, Alexandre via pve-devel
2025-06-16 11:51     ` Fiona Ebner
2025-06-16 12:46       ` Fabian Grünbichler
2025-06-16 12:57         ` Fiona Ebner
2025-06-16 13:15           ` Fabian Grünbichler [this message]
2025-06-16 13:27             ` Fiona Ebner
     [not found]   ` <409e12ad0b53d1b51c30717e6b9df3d370112df4.camel@groupe-cyllene.com>
2025-06-17  6:04     ` DERUMIER, Alexandre via pve-devel
2025-06-17  7:35       ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 16/22] print drive device: explicitly set write-cache starting with machine version 10.0 Fiona Ebner
2025-06-13 10:28   ` Fabian Grünbichler
2025-06-17  7:47     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 17/22] print drive device: set {r, w}error front-end properties " Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 18/22] print drive device: don't reference any drive for 'none' " Fiona Ebner
2025-06-13 11:14   ` Fabian Grünbichler
2025-06-17  7:54     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 19/22] drive: create a throttle group for each drive " Fiona Ebner
2025-06-13 11:23   ` Fabian Grünbichler
2025-06-17  7:58     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline Fiona Ebner
2025-06-13 11:35   ` Fabian Grünbichler
2025-06-17  9:37     ` Fiona Ebner
2025-06-17  9:58       ` Fabian Grünbichler
2025-06-16 11:07   ` DERUMIER, Alexandre via pve-devel
2025-06-17 10:02     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [RFC qemu-server 21/22] blockdev: add support for NBD paths Fiona Ebner
2025-06-16 10:46   ` DERUMIER, Alexandre via pve-devel
2025-06-17 10:11     ` Fiona Ebner
2025-06-12 14:02 ` [pve-devel] [RFC qemu-server 22/22] command line: switch to blockdev starting with machine version 10.0 Fiona Ebner
2025-06-16 10:40   ` DERUMIER, Alexandre via pve-devel
2025-06-18 11:39     ` Fiona Ebner
2025-06-18 12:04       ` DERUMIER, Alexandre via pve-devel
2025-06-16  6:22 ` [pve-devel] [PATCH-SERIES qemu-server 00/22] preparation for switch to blockdev DERUMIER, Alexandre via pve-devel
2025-06-16  8:44   ` Fiona Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=386024326.3481.1750079724005@webmail.proxmox.com \
    --to=f.gruenbichler@proxmox.com \
    --cc=f.ebner@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal