Re: [pve-devel] [PATCH v3 qemu-server 09/10] migrate: add remote migration handling
On 04.01.22 17:44, Roland wrote:
>>> +sub phase2_start_remote_cluster {
>>> + my ($self, $vmid, $params) = @_;
>>> +
>>> + die "insecure migration to remote cluster not implemented\n"
>>> + if $params->{migrate_opts}->{type} ne 'websocket';
>>> +
>>> + my $remote_vmid = $self->{opts}->{remote}->{vmid};
>>> +
>>> + my $res = PVE::Tunnel::write_tunnel($self->{tunnel}, 10,
>>> "start", $params);
>>
>> 10 seconds feels a bit short to me.
@Fabian(s):
Why not use the same as vm_start_nolock + some buffer? E.g.,
($params->{timeout} // config_aware_timeout($conf, $resume)) + 10;
That should be quite generous, as the worst case time for the start for
incoming migration, which is always paused so not doing much, is normally
way lower than a cold-start.
I mean, we're in an worker task and do not really care much if setup needs
a bit longer.
>>
> Please, administrators like tunables and knobs for changing default values.
>
@Roland
sure, but exposing all hundreds+ of timeout and other parameters will just
overload most users and produce guides to extend timeouts for issues that
should be actually fixed in the setup, i.e., when the timeout is just a symptom
of a bad config/hw/...
> Not only for being empowered to fix things themselves but also to be
> able to dig into a problem and find the root cause...
>
With PVE et. al being under AGPLv3 you're already empowered to just change the
code, so lets keep it simple(r) to most users, if a timeout is actually to short
we can always change it to a better fitting value (at least if reports about
that
reach us).
> I remember that i had more then one occasion , where i grepped for
> timeout or other limiting values in proxmox or other softwares source,
> and often gave up in the end, because it was too deeply hidden or i got
> too many hits/findings.
>
same would happen if we expose every possible parameter as knob, you'd have
hundreds and get many hits on searches..
> Finding such without knowing the code can often be like searching for
> the needle in a haystack and extremely frustrating.
sure and I can imagine that it's frustrating, but you can always ask here, on
pve-user or other channels about the issue at hand and people more used to
the code can often better guide you to the actual parameter location, or give
some other input.
>
> I would be happy, if such important values would get defined with some
> descriptive variable name at a suitable location, maybe even with some
> comment what's it all about ( even if it's not meant to be changed/tuned)
>
everybody sees specific knobs as more or less important, so that's not a a clear
cut at all. For comments I'd also suggest using git blame to get the commit
(message) that introduced it, often there's some info encoded in there too.
Also, if you ever got some reasoning into a special timeout that had no info
or comment whatsoever we're naturally happy to accept patches that add one.
> just my 10 cents...
thanks for your input. IMO this specific one can be improved without exposing
a new tunable (see start of my reply), and in general I prefer to avoid to many
tunables, we can still add them if it shows that there's quite some demand for
a specific one, while removing would break compat..
cheers,
Thomas
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager] ui: fix novnc scaling radio buttons
when having selected 'off', the next start of the window has both
radiobuttons selected and no change is possible anymore. It seems that
the 'checked: true' triggers only after the 'init' function.
So instead remove the 'checked: true', and add the default in the init
function.
Signed-off-by: Dominik Csapak
---
www/manager6/window/Settings.js | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/www/manager6/window/Settings.js b/www/manager6/window/Settings.js
index 4aa7137c..b151adcd 100644
--- a/www/manager6/window/Settings.js
+++ b/www/manager6/window/Settings.js
@@ -34,10 +34,8 @@ Ext.define('PVE.window.Settings', {
var username = sp.get('login-username') || Proxmox.Utils.noneText;
me.lookupReference('savedUserName').setValue(Ext.String.htmlEncode(username));
- var vncMode = sp.get('novnc-scaling');
- if (vncMode !== undefined) {
- me.lookupReference('noVNCScalingGroup').setValue({
noVNCScalingField: vncMode });
- }
+ var vncMode = sp.get('novnc-scaling') || 'scale';
+ me.lookupReference('noVNCScalingGroup').setValue({
noVNCScalingField: vncMode });
let summarycolumns = sp.get('summarycolumns', 'auto');
me.lookup('summarycolumns').setValue(summarycolumns);
@@ -379,7 +377,6 @@ Ext.define('PVE.window.Settings', {
name: 'noVNCScalingField',
inputValue: 'scale',
boxLabel: 'Local Scaling',
- checked: true,
}, {
xtype: 'radiofield',
name: 'noVNCScalingField',
--
2.30.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH zfsonlinux/stable-6 0/2] update zfs to 2.0.7
2.0.7 contains a few commits which might affect our users e.g.: `ZFS send/recv with ashift 9->12 leads to data corruption` the second commit is a cherry-pick from our current master (abigail failures should not cause the build to abort) built and booted the kernel on one of our hardware-testhosts Aron Xu (1): d/rules: allow abigail to fail Stoiko Ivanov (1): update submodule and patches to ZFS 2.0.7 debian/patches/0005-Enable-zed-emails.patch | 2 +- debian/patches/0007-Use-installed-python3.patch | 6 +++--- debian/rules| 2 +- upstream| 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) -- 2.30.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH zfsonlinux/stable-6 1/2] update submodule and patches to ZFS 2.0.7
Signed-off-by: Stoiko Ivanov --- debian/patches/0005-Enable-zed-emails.patch | 2 +- debian/patches/0007-Use-installed-python3.patch | 6 +++--- upstream| 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/debian/patches/0005-Enable-zed-emails.patch b/debian/patches/0005-Enable-zed-emails.patch index e837d7e9..f605a356 100644 --- a/debian/patches/0005-Enable-zed-emails.patch +++ b/debian/patches/0005-Enable-zed-emails.patch @@ -13,7 +13,7 @@ Signed-off-by: Thomas Lamprecht 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/zed/zed.d/zed.rc b/cmd/zed/zed.d/zed.rc -index df560f921..4ce7af744 100644 +index 1c278b2ef..41c075c09 100644 --- a/cmd/zed/zed.d/zed.rc +++ b/cmd/zed/zed.d/zed.rc @@ -15,7 +15,7 @@ diff --git a/debian/patches/0007-Use-installed-python3.patch b/debian/patches/0007-Use-installed-python3.patch index 789ce2ad..a44ee403 100644 --- a/debian/patches/0007-Use-installed-python3.patch +++ b/debian/patches/0007-Use-installed-python3.patch @@ -28,7 +28,7 @@ index 3788543b0..c7ee4ae9a 100755 typeset -i cnt=0 diff --git a/tests/zfs-tests/tests/functional/rsend/send_encrypted_files.ksh b/tests/zfs-tests/tests/functional/rsend/send_encrypted_files.ksh -index f89cb3b31..375d483f7 100755 +index d52f0261a..18356b017 100755 --- a/tests/zfs-tests/tests/functional/rsend/send_encrypted_files.ksh +++ b/tests/zfs-tests/tests/functional/rsend/send_encrypted_files.ksh @@ -87,7 +87,7 @@ log_must xattrtest -f 10 -x 3 -s 32768 -r -k -p /$TESTPOOL/$TESTFS2/xattrsadir @@ -41,10 +41,10 @@ index f89cb3b31..375d483f7 100755 log_must zfs set compression=off xattr=on $TESTPOOL/$TESTFS2 diff --git a/tests/zfs-tests/tests/functional/rsend/send_realloc_dnode_size.ksh b/tests/zfs-tests/tests/functional/rsend/send_realloc_dnode_size.ksh -index 394fe95bb..43560aac5 100755 +index 551ed15db..bd30488ea 100755 --- a/tests/zfs-tests/tests/functional/rsend/send_realloc_dnode_size.ksh +++ b/tests/zfs-tests/tests/functional/rsend/send_realloc_dnode_size.ksh -@@ -93,7 +93,7 @@ log_must zfs snapshot $POOL/fs@c +@@ -88,7 +88,7 @@ log_must zfs snapshot $POOL/fs@c # 4. Create an empty file and add xattrs to it to exercise reclaiming a #dnode that requires more than 1 slot for its bonus buffer (Zol #7433) log_must zfs set compression=on xattr=sa $POOL/fs diff --git a/upstream b/upstream index ef686e96..ad81baab 16 --- a/upstream +++ b/upstream @@ -1 +1 @@ -Subproject commit ef686e96ec1f907cac39552fa0e21298a6f38df5 +Subproject commit ad81baab7779cd2113669d30de72bb925e684525 -- 2.30.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH zfsonlinux/stable-6 2/2] d/rules: allow abigail to fail
From: Aron Xu (cherry picked from debian upstream [0] commit 5ae98b5499022c2c127d546a7b5aeb906f6f2a6b) [0] https://salsa.debian.org/zfsonlinux-team/zfs Signed-off-by: Stoiko Ivanov --- debian/rules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debian/rules b/debian/rules index 826bdc98..e5bb4a56 100755 --- a/debian/rules +++ b/debian/rules @@ -50,7 +50,7 @@ override_dh_auto_test: override_dh_auto_test: ifeq (amd64,$(DEB_HOST_ARCH)) # Upstream provides an ABI guarantee that we validate here - $(MAKE) checkabi + -$(MAKE) checkabi endif # The dh_auto_test rule is disabled because -- 2.30.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH manager] ui: fix novnc scaling radio buttons
On 11.01.22 11:26, Dominik Csapak wrote: > when having selected 'off', the next start of the window has both > radiobuttons selected and no change is possible anymore. It seems that > the 'checked: true' triggers only after the 'init' function. > > So instead remove the 'checked: true', and add the default in the init > function. > > Signed-off-by: Dominik Csapak > --- > www/manager6/window/Settings.js | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH 1/1] Add TOTP authentification for ACME DNS INWX
On 01.01.22 21:06, Nils Sandmann wrote: > Signed-off-by: Nils Sandmann > --- > src/dns-challenge-schema.json | 4 > 1 file changed, 4 insertions(+) > > applied, many thanks for your contribution! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH zfsonlinux] cherry-pick lock-inversion patch for zvol_open
the changes to zvol_open added to 2.1.2 (for coping with kernel
changes in 5.13) seem to have introduced a lock order inversion [0].
(noticed while reviewing the 2.0.6->2.0.7 changes (the patch was
applied after 2.1.2 was already tagged)
[0] https://github.com/openzfs/zfs/pull/12863
Signed-off-by: Stoiko Ivanov
---
did not reproduce the dead-lock myself (in my very limited tests)
but given that our 2.1.2 packages (and kernel modules) have not seen
too much testing in public yet - thought it makes sense to proactively
pull this in
quickly tested a kernel+userspace with this patch on a physical testhost
.../0012-Fix-zvol_open-lock-inversion.patch | 212 ++
debian/patches/series | 1 +
2 files changed, 213 insertions(+)
create mode 100644 debian/patches/0012-Fix-zvol_open-lock-inversion.patch
diff --git a/debian/patches/0012-Fix-zvol_open-lock-inversion.patch
b/debian/patches/0012-Fix-zvol_open-lock-inversion.patch
new file mode 100644
index ..eb74550f
--- /dev/null
+++ b/debian/patches/0012-Fix-zvol_open-lock-inversion.patch
@@ -0,0 +1,212 @@
+From Mon Sep 17 00:00:00 2001
+From: Brian Behlendorf
+Date: Fri, 17 Dec 2021 09:52:13 -0800
+Subject: [PATCH] Fix zvol_open() lock inversion
+
+When restructuring the zvol_open() logic for the Linux 5.13 kernel
+a lock inversion was accidentally introduced. In the updated code
+the spa_namespace_lock is now taken before the zv_suspend_lock
+allowing the following scenario to occur:
+
+down_read <=== waiting for zv_suspend_lock
+zvol_open <=== holds spa_namespace_lock
+__blkdev_get
+blkdev_get_by_dev
+blkdev_open
+...
+
+ mutex_lock <== waiting for spa_namespace_lock
+ spa_open_common
+ spa_open
+ dsl_pool_hold
+ dmu_objset_hold_flags
+ dmu_objset_hold
+ dsl_prop_get
+ dsl_prop_get_integer
+ zvol_create_minor
+ dmu_recv_end
+ zfs_ioc_recv_impl <=== holds zv_suspend_lock via zvol_suspend()
+ zfs_ioc_recv
+ ...
+
+This commit resolves the issue by moving the acquisition of the
+spa_namespace_lock back to after the zv_suspend_lock which restores
+the original ordering.
+
+Additionally, as part of this change the error exit paths were
+simplified where possible.
+
+Reviewed-by: Tony Hutter
+Reviewed-by: Rich Ercolani
+Signed-off-by: Brian Behlendorf
+Closes #12863
+(cherry picked from commit 8a02d01e85556bbe3a1c6947bc11b8ef028d4023)
+Signed-off-by: Stoiko Ivanov
+---
+ module/os/linux/zfs/zvol_os.c | 121 --
+ 1 file changed, 58 insertions(+), 63 deletions(-)
+
+diff --git a/module/os/linux/zfs/zvol_os.c b/module/os/linux/zfs/zvol_os.c
+index 44caadd58..69479b3f7 100644
+--- a/module/os/linux/zfs/zvol_os.c
b/module/os/linux/zfs/zvol_os.c
+@@ -496,8 +496,7 @@ zvol_open(struct block_device *bdev, fmode_t flag)
+ {
+ zvol_state_t *zv;
+ int error = 0;
+- boolean_t drop_suspend = B_TRUE;
+- boolean_t drop_namespace = B_FALSE;
++ boolean_t drop_suspend = B_FALSE;
+ #ifndef HAVE_BLKDEV_GET_ERESTARTSYS
+ hrtime_t timeout = MSEC2NSEC(zvol_open_timeout_ms);
+ hrtime_t start = gethrtime();
+@@ -517,7 +516,36 @@ retry:
+ return (SET_ERROR(-ENXIO));
+ }
+
+- if (zv->zv_open_count == 0 && !mutex_owned(&spa_namespace_lock)) {
++ mutex_enter(&zv->zv_state_lock);
++ /*
++ * Make sure zvol is not suspended during first open
++ * (hold zv_suspend_lock) and respect proper lock acquisition
++ * ordering - zv_suspend_lock before zv_state_lock
++ */
++ if (zv->zv_open_count == 0) {
++ if (!rw_tryenter(&zv->zv_suspend_lock, RW_READER)) {
++ mutex_exit(&zv->zv_state_lock);
++ rw_enter(&zv->zv_suspend_lock, RW_READER);
++ mutex_enter(&zv->zv_state_lock);
++ /* check to see if zv_suspend_lock is needed */
++ if (zv->zv_open_count != 0) {
++ rw_exit(&zv->zv_suspend_lock);
++ } else {
++ drop_suspend = B_TRUE;
++ }
++ } else {
++ drop_suspend = B_TRUE;
++ }
++ }
++ rw_exit(&zvol_state_lock);
++
++ ASSERT(MUTEX_HELD(&zv->zv_state_lock));
++
++ if (zv->zv_open_count == 0) {
++ boolean_t drop_namespace = B_FALSE;
++
++ ASSERT(RW_READ_HELD(&zv->zv_suspend_lock));
++
+ /*
+* In all other call paths the spa_namespace_lock is taken
+* before the bdev->bd_mutex lock. However, on open(2)
+@@ -542,84 +570,51 @@ retry:
+* the kernel so the only option is to return the error for
+* the caller to handle it.
+*/
+- if (!mutex_tryenter(&spa_namespace_lock)) {
+- rw_
[pve-devel] applied: [PATCH zfsonlinux] cherry-pick lock-inversion patch for zvol_open
On 11.01.22 16:39, Stoiko Ivanov wrote: > the changes to zvol_open added to 2.1.2 (for coping with kernel > changes in 5.13) seem to have introduced a lock order inversion [0]. > > (noticed while reviewing the 2.0.6->2.0.7 changes (the patch was > applied after 2.1.2 was already tagged) > > [0] https://github.com/openzfs/zfs/pull/12863 > Signed-off-by: Stoiko Ivanov > --- > did not reproduce the dead-lock myself (in my very limited tests) > but given that our 2.1.2 packages (and kernel modules) have not seen > too much testing in public yet - thought it makes sense to proactively > pull this in > > quickly tested a kernel+userspace with this patch on a physical testhost > > .../0012-Fix-zvol_open-lock-inversion.patch | 212 ++ > debian/patches/series | 1 + > 2 files changed, 213 insertions(+) > create mode 100644 debian/patches/0012-Fix-zvol_open-lock-inversion.patch > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] zfs max record size tunable
hi all! i recently was playing with the ZOL module parameters, while tuning compression algos for my filer, and ran into a scenario that would be valuable knowledge here as well (i think) not to go down the rabbit hole of why one might or might not want to do this too far, but TLDR: recordsize controls the chunk size of data handed to whatever compression algorithm is in use for that fs/vol, and it can increase the compression attained in some cases (and many relevant to pxm imo, but… tangent… :) ) if you increase the max record size on a host (default is 1Mb, max is 16mb) via the kernel module tunable, it’s set ba k to default on reboot… now, vols/fs with larger recordsize than kernel module has configured is fine. no problems occur. BUT. if you try to create a file system/vol with a larger recordsize than the module (currently) allows, zfs complains. this is especially problematic when using zfs replication… if you try to send a fs/vol/snapshot which has a recordsize larger than target host supports, the send fails oddly. i can see both sides of bug/feature mindset… but what i figured was relevant here contextually is that it might be worth adding some logic to evaluate the recordsize and the relevant module tuning and make some noise if the user tries to do something that won’t work and won’t be obvious why.. if this does not feel relevant here, mea culpa. im really a fan of the way pxm works… y’all have done a really nice job stitching a lot of complicated things together well. thank you for making my life as an admin of infra easier. i really appreciate it. UNRELATED FEEDBACK: i would love it if there was a stream of “command line executions performed on your behalf in response to you driving the UI” that the user could observe (the ui is almost doing this already, just not quite as verbosely ((or durably)) as i had in mind… i was thinking of sort-of… “admin by osmosis mode” wherein a novice user could educate themselves by performing ops and observing the cmdline log file essentially… the reason behind this stems from things like: i need to take node X out for maintenance… i want to evacuate all the vms or i need to upgrade node X’s storage… evacuate all vms and destroy all ceph OSDs on it… that’s not really easy to do via the existing UI, and seeing the exact cmds pxm is doing for me under the covers would help scripting something sane… alternatively, adding some entire-host scoped one-click UI features would help alleviate this scenario too… so… if that’s something in the works… great ❤️🤡🐺W [= The contents of this message have been written, read, processed, erased, sorted, sniffed, compressed, rewritten, misspelled, overcompensated, lost, found, and most importantly delivered entirely with recycled electrons =] ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
