Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reevaluate the need of libzfs.ZFS context manager lock #250

Open
rkojedzinszky opened this issue Jan 3, 2024 · 7 comments · May be fixed by #252
Open

Reevaluate the need of libzfs.ZFS context manager lock #250

rkojedzinszky opened this issue Jan 3, 2024 · 7 comments · May be fixed by #252

Comments

@rkojedzinszky
Copy link
Contributor

bc687e9 added a global lock on zfs objects when used as context manager. Howewer, just when used as context manager. So, this may not protect in every case if the underlying libzfs is not thread safe.

Is this lock still needed? I mean, if I have an application with multiple threads, each thread managing its libzfs.ZFS object, will I still need a global lock to serialize them? Or perhaps, libzfs itself is threadsafe now. I suspect libzfs wants to be threadsafe, so if there is a known issue, then we should report this upstream.

@rkojedzinszky rkojedzinszky linked a pull request Jan 4, 2024 that will close this issue
@anodos325
Copy link
Contributor

anodos325 commented Jan 17, 2024

The lock is still needed. Libzfs is known not to be threadsafe.

@rkojedzinszky
Copy link
Contributor Author

The lock is still needed. Libzfs is known not to be threadsafe.

Can you please provide me some details? I would like to help eliminate that, or if not possible, understand the reasons.

Thanks

@yocalebo
Copy link
Contributor

@rkojedzinszky there are places in libzfs that make non-reentrant syscalls (getpw* and getgrp* come to mind) and there is also global memory that isn't protected by any mechanism.

@rkojedzinszky
Copy link
Contributor Author

@yocalebo Diggint into openzfs:

openzfs/lib$ git grep -E 'get(pw|gr)'
libzfs/libzfs.abi:    <function-decl name='getgrnam' visibility='default' binding='global' size-in-bits='64'>
libzfs/libzfs.abi:    <function-decl name='getpwnam' visibility='default' binding='global' size-in-bits='64'>
libzfs/libzfs_dataset.c:        if (isuser && (pw = getpwnam(cp)) != NULL) {
libzfs/libzfs_dataset.c:        } else if (isgroup && (gr = getgrnam(cp)) != NULL) {
libzpool/kernel.c:crgetgroups(cred_t *cr)

Unfortunately, yes, there are calls to those functions, howewer, only here.

Can you please point me to some global memory in libzfs?

@anodos325
Copy link
Contributor

I think probably the better place to start if you want to make libzfs threadsafe is via upstream openzfs community (mailing lists or slack). Once the changes are in the ZFS version in TrueNAS we will happily remove locks (once we're sure there aren't other issues).

@rkojedzinszky
Copy link
Contributor Author

@anodos325 working on it: openzfs/zfs#15793

@rkojedzinszky rkojedzinszky changed the title Reevaulate the need of libzfs.ZFS context manager lock Reevaluate the need of libzfs.ZFS context manager lock Jan 30, 2024
@rkojedzinszky
Copy link
Contributor Author

@anodos325 openzfs/zfs#15793 has been closed, not by merge but by commits applied to master branch. Can we go further with this?

rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Oct 24, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Oct 25, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Oct 26, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Oct 26, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Oct 28, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 2, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 2, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 4, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 4, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 4, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 5, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 6, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 6, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 6, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 7, 2024
rkojedzinszky added a commit to k-web-s/truenas-middleware that referenced this issue Nov 30, 2024
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 a pull request may close this issue.

3 participants