I was looking at this again and now think I understand what I didn't like at the first look :-)

The idea is to reduce the locking overhead by moving the lock at a higher level, keep it longer and avoid the short time locks.
The patch takes the lock now in opshared.c which is ok, but at the places where the locks were previously taken, now ther is a call to a new function vattr_rdlock() with local level - do we really need this ? Unless there is a possibility that we reach that we reach these places without coming from opshared search I think no.
In my opinion the call at the global level is sufficient, we need to handle the case of nested global locks. In the code this is logged as CRIT, but I think it is not an error, we can have a thread doing a search, which calls plugins which do searches again and try to acquire the global lock. If it is the same thread that should be just fine.
So wouldn't it be sufficient to only have the calls at global level and instead of the "ACQUIRED" flag have a counter to manage netsted locks ?

Thanks Ludwig for your careful review. I think you identified the reason why I also did not like it much without being able to explain why ;)

I agree that the vattr_rdlock calls are useless if all call comes from opshared. Useless because they do nothing. But I would replace them with a comment explaining why it does not need to acquire the lock.

Note that we do need the calls to vattr_wrlock.

Good catch about nested calls, I initially thought it was an error but you are right it can happen. I will implement a counter as you suggested.

Could we put a PR_ASSERT in here so that during development we crash as well?

Another option, rather that write our own counter, is that pthread rw locks have a recursive attribute you can set on init that provides the counter for us. This would mean we do not need to construct our own counter.

@firstyear, the idea of the ticket is to reduce the #calls to pthread rwlock. They were expensive (I noted some recent changes in RHEL8 for perf improvements) and created contention. I am afraid using pthread counter can suffer from the same performance hit.

Well, if we construct our own counter, we could get it wrong. It could be better to use the well vetted pthread rwlock version.

saying this, there is a high risk of deadlock in recursive rwlock setup, so this may not be the best plan either ...

@firstyear, I am a bit lost with your proposal. Here is my understanding.
For simple mutex, pthread_mutex_t used in DS are not recursive. We can request to create a recursive mutext but so far recursive lock are created with PR_NewMonitor (in DS).
For rwlock, I do not know any recursive flag/attribute. Which one did you suggest ?

Never mind. I think I was confusing myself. Ack from me this code looks good,

Thanks William for the reviews !!

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3222

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.