Reevaluate the need of libzfs.ZFS context manager lock

[rkojedzinszky]

https://github.com/truenas/py-libzfs/commit/bc687e95e92f1a14da4b990b193296c51366fd13 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.

[anodos325]

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

[rkojedzinszky]

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]

@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]

@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]

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]

@anodos325 working on it: https://github.com/openzfs/zfs/pull/15793

[rkojedzinszky]

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