AvgZFSLocking [FreeBSD WIKI]

This page is read only. You can view the source, but not change it. Ask your administrator if you think this is wrong.
==== The Lock ====

''%%spa_namespace_lock%%'' is __the__ lock.

Let's first look at the comments describing it:
[[http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/spa_misc.c#58|Code Snippet]]

Some immediate observations:
  * the lock covers a lot: from simple things like manipulations of a spa collection to covering complex operations on vdevs
  * needless uses of the lock are avoided by applying discipline: keep and use a reference to a spa that always must have instead of looking it up by name

And here is the first trouble with the lock, right in the core code where it belongs:
[[http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/spa.c#2740|Code Snippet]]

Hmm, not so good if the code that does spa opening gets recursed into.
Let's why it happens. ''%%spa_open_common%%'' may call the following chain: ''%%spa_load_best%%'' => ''%%spa_load%%'' => ''%%spa_load_impl%%''.
In the latter function there is a call to ''%%dmu_objset_find%%'':
''%% (void) dmu_objset_find(spa_name(spa), .... %%''

Wow, we have a spa reference, in fact we are still constructing the spa, but call an external function with a spa name instead of the spa itself.
It's not hard to follow through the code that the name is eventually resolved to the ''%%spa_t%%'' again. Via a recursive ''%%spa_open%%'' call.

In fact, there is a function similar to ''%%dmu_objset_find%%'' but with a signature that takes a ''%%spa_t%%'' pointer. But the function takes more arguments,
a callback function that it takes also has a different signature.

This brings us to the following further two observations:
  * API that take a spa or a dataset names is often more convenient to use than API that takes a spa or a dataset pointer
  * the code is littered with a lot of API calls that take a name and which have to do a name to object lookup when the object is readily available

So the picture is not as rosy as the ''%%spa_namespace_lock%%'' [[#Snippet1|description]] would have us believe.
The second [[#Snippet2|snippet]] is a good illustration of that. But there could be other hidden problems with potential lock order reversal.
There would be less reasons to be concerned if the lock served only for spa collection manipulations which would imply that it does not interact with other "external" locks.
But it is not so. For example the lock is held while probing disks.

That operation is especially non-trivial if the involved disks are ZFS zvols.
It is easy to observe the following lock order reversal.

''%%spa_open%%'' takes ''%%spa_namespace_lock%%'' then calls all the way down to ''%%vdev_open%%'', which through the storage subsystem calls into ''%%zvol_open%%'' which, in upstream code, takes ''%%zfsdev_state_lock%%''.
On the other hand, some zvol operations first take ''%%zfsdev_state_lock%%'' and then call some name-based DMU/DSL API function which has to perform a spa lookup which ends up in ''%%spa_open%%'' with ''%%spa_namespace_lock%%''.

On recent %%FreeBSD%% all uses of ''%%zfsdev_state_lock%%'' are replaced with ''%%spa_namespace_lock%%'' and, to avoid ''%%spa_namespace_lock%%'' recursion, any use of pools on zvols is prohibited.
That fixes the described LOR, but I am not happy.
Unlike upstream, %%FreeBSD%% situation was/is aggravated by ''%%g_topology_lock%%'' that has to be taken in vdev operations called from spa_open under ''%%spa_namespace_lock%%''.
''%%g_topology_lock%%'' is also taken in some zvol operations.
The third lock in the mix doesn't add any clarity.

==== Possible solutions ====

  * what %%FreeBSD%% did, but functionality suffers
  * meticulous cleanup of calls that needlessly call name resolution
  * breaking up ''%%spa_namespace_lock%%'' into finer-grained locks thus separating spa collection manipulations from heavy-weight tasks like spa construction and vdev operations
  * make the lock recursive
    * not available in upstream (Solaris, Illumos, and descendant/derivatives)
    * maybe won't help on %%FreeBSD%% because of LORs with GEOM topology lock

----

{{tag>Zfs}}