Skip to content

Conversation

@gthvn1
Copy link

@gthvn1 gthvn1 commented Feb 12, 2025

Since we are now supporting qcow file tap-ctl list can return strings like:

  • "1564848 0 0 qcow2 /var/run/sr-mount/..."
    Without this patch the type "qcow2" is unknown and xcp-rrdd-iostat generates an
    error like: returned a line that could not be parsed. Ignoring
    This patch fixes the issue.

Since we are now supporting qcow file `tap-ctl list` can return strings like:
  - "1564848    0    0      qcow2 /var/run/sr-mount/..."
Without this patch the type "qcow2" is unknown and xcp-rrdd-iostat generates an
error like: returned a line that could not be parsed. Ignoring
This patch fixes the issue.

Signed-off-by: Guillaume <[email protected]>
@stormi
Copy link
Member

stormi commented Feb 12, 2025

Can the supported list be made a setting so that we can contribute this upstream?

@gthvn1 gthvn1 self-assigned this Feb 12, 2025
@gthvn1
Copy link
Author

gthvn1 commented Feb 12, 2025

You mean a list that can be passed as part of a configuration by a user?

@stormi
Copy link
Member

stormi commented Feb 12, 2025

We already have various settings for which we provide defaults that differ from those of XenServer, and contributed new settings in the past for needs we had but they didn't have on their side. This qcow2 support situation looks a lot like one of these situations to me.

Users could modify these settings, but that's not really something we support. In our case, the first purpose of these settings is to allow different behaviours between XS and XCP-ng, without forking the codebase.

@gthvn1
Copy link
Author

gthvn1 commented Feb 14, 2025

@stormi do you have a link to such contributions about new settings? I don't understand what I should do.

@gthvn1 gthvn1 deleted the branch gtn-8.3-v24.19.2 February 15, 2025 11:00
@gthvn1 gthvn1 closed this Feb 15, 2025
gthvn1 pushed a commit that referenced this pull request Feb 15, 2025
Backport of 3b52b72

This enables PAM to be used in multithreaded mode (currently XAPI has a global lock around auth).

Using an off-cpu flamegraph I identified that concurrent PAM calls are slow due to a call to `sleep(1)`.
`pam_authenticate` calls `crypt_r` which calls `NSSLOW_Init` which on first use will try to initialize the just `dlopen`-ed library.
If it encounters a race condition it does a `sleep(1)`. This race condition can be quite reliably reproduced when performing a lot of PAM authentications from multiple threads in parallel.

GDB can also be used to confirm this by putting a breakpoint on `sleep`:
```
  #0  __sleep (seconds=seconds@entry=1) at ../sysdeps/unix/sysv/linux/sleep.c:42
  #1  0x00007ffff1548e22 in freebl_RunLoaderOnce () at lowhash_vector.c:122
  #2  0x00007ffff1548f31 in freebl_InitVector () at lowhash_vector.c:131
  #3  NSSLOW_Init () at lowhash_vector.c:148
  #4  0x00007ffff1b8f09a in __sha512_crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=0x7ffff31e17b8 "dIJbsXKc0",
  #5  0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=<optimized out>,
  #6  0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=<optimized out>, nullok=nullok@entry=0) at passverify.c:111
  #7  0x00007ffff1dc9139 in _unix_verify_password (pamh=pamh@entry=0x7fffd8002910, name=0x7fffd8002ab0 "pamtest-edvint", p=0x7fffd8005a60 "pamtest-edvint", ctrl=ctrl@entry=8389156) at support.c:777
  #8  0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=<optimized out>, argc=<optimized out>, argv=<optimized out>) at pam_unix_auth.c:178
  #9  0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=<optimized out>, resumed=<optimized out>, h=<optimized out>, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110
  #10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426
  #11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34
  #12 0x00000000005ae567 in XA_mh_authorize (username=username@entry=0x7fffd80028d0 "pamtest-edvint", password=password@entry=0x7fffd80028f0 "pamtest-edvint", error=error@entry=0x7ffff31e1be8) at xa_auth.c:83
  xapi-project#13 0x00000000005adf20 in stub_XA_mh_authorize (username=<optimized out>, password=<optimized out>) at xa_auth_stubs.c:42
```

`pam_start` and `pam_end` doesn't help here, because on `pam_end` the library is `dlclose`-ed, so on next `pam_authenticate` it will have to go through the initialization code again.
(This initialization code would've belonged into `pam_start`, not `pam_authenticate`, but there are several layers here including a call to `crypt_r`).
Upstream has fixed this problem >5 years ago by switching to libxcrypt instead.

Signed-off-by: Edwin Török <[email protected]>
Signed-off-by: Christian Lindig <[email protected]>
@stormi
Copy link
Member

stormi commented Feb 17, 2025

I think this should be discussed as part of xapi-project#6308

@gthvn1
Copy link
Author

gthvn1 commented Feb 17, 2025

I think this should be discussed as part of xapi-project#6308

I don't think so because this is really related to qcow2 and the xapi-project#6308 is more generic to image formats. Of course the use case we are interesting is the qcow2 but it is not specific to qcow2.
Also notice that the commit has been closed as the side effect of deleting gtn-8.3-v24.19.2. It is not merged and need to be reopen.

@stormi
Copy link
Member

stormi commented Feb 17, 2025

It's at least interesting to XAPI maintainers to know how it fits in the big picture.

@gthvn1
Copy link
Author

gthvn1 commented Feb 17, 2025

Agree on that and I can explain the big picture when submitting this upstream but my point is that it is not related with the design of adding the new field supported image formats. So I feel that adding the context of this pull request to the conversation of the design will confuse people a bit.

@gthvn1
Copy link
Author

gthvn1 commented Apr 11, 2025

It was closed because the branch was removed. New #6 created with a new branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants