Hello, Agreed that unlocking early is better. Here is a V2 that does it for libdiskfs, I will send a separate patch for libnetfs. Thanks
On Sun, Nov 1, 2026 at 10:42 AM Samuel Thibault <[email protected]> wrote: > Hello, > > Milos Nikic, le jeu. 29 oct. 2026 21:26:39 -0700, a ecrit: > > try_lookup unlocks the directory before fshelp_fetch_root, which locks > > only the translated node again. Every outcome but ENOENT then goes to > > OUT, which unlocks the directory a second time with diskfs_nput. A > > thread holding the directory, such as another lookup in it, loses its > > lock, as with any lookup through a translator, e.g. under /dev or > > /servers. > > > > Release only the directory's reference on those exits. > > Ok. I however see that libnetfs's dir-lookup.c is very similar, > we should avoid them diverging too much. In libnetfs, the 'out' > label expects dnp not to be locked, it is undeed unlocked early, by > netfs_attempt_lookup typically. Does libdiskfs really need to keep it > locked while doing "start the translator" etc.? > > Conversely, the management of deref-ing newpi may be simpler in > libdiskfs than libnetfs? > > Samuel >
From 3712c6f10ef752e76ef6b98626d8bbf7952f7343 Mon Sep 17 00:00:00 2001 From: Milos Nikic <[email protected]> Date: Sun, 1 Nov 2026 20:55:04 -0800 Subject: [PATCH v2 1/3] libdiskfs: Unlock the directory once a lookup has found its node diskfs_S_dir_lookup kept the directory locked after the lookup of each name, and unlocked it only before starting a translator. The exits after that went to OUT, which unlocked it again with diskfs_nput: a thread holding the directory, such as another lookup in it, lost its lock, on any lookup through a translator (under /dev, /servers, ...). Unlock it once the node is found, and a new one created, as libnetfs's dir-lookup does, and from then on only release its reference. A symlink locks it again to look up the next name in it. The translator path no longer unlocks it, nor locks it again when the translator vanished. --- libdiskfs/dir-lookup.c | 46 ++++++++++++++---------------------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/libdiskfs/dir-lookup.c b/libdiskfs/dir-lookup.c index ca314f00d..5c787ccfe 100644 --- a/libdiskfs/dir-lookup.c +++ b/libdiskfs/dir-lookup.c @@ -97,7 +97,7 @@ diskfs_S_dir_lookup (struct protid *dircred, dnp = dircred->po->np; pthread_mutex_lock (&dnp->lock); - diskfs_nref (dnp); /* acquire a reference for later diskfs_nput */ + diskfs_nref (dnp); /* acquire a reference for later diskfs_nrele */ do { @@ -166,6 +166,7 @@ diskfs_S_dir_lookup (struct protid *dircred, else if (!lastcomp) strcpy (retry_name, nextname); err = 0; + pthread_mutex_unlock (&dnp->lock); goto out; } } @@ -182,6 +183,7 @@ diskfs_S_dir_lookup (struct protid *dircred, else if (!lastcomp) strcpy (retry_name, nextname); err = 0; + pthread_mutex_unlock (&dnp->lock); goto out; } else @@ -214,6 +216,12 @@ diskfs_S_dir_lookup (struct protid *dircred, diskfs_drop_dirstat (dnp, ds); } + /* The rest only needs a reference to DNP, as in libnetfs: unlock it, + unless NP is DNP itself. A symlink locks it again to look up the + next name in it. */ + if (np != dnp) + pthread_mutex_unlock (&dnp->lock); + if (err) goto out; @@ -229,8 +237,7 @@ diskfs_S_dir_lookup (struct protid *dircred, mach_port_t dirport; struct iouser *user; - /* Create an unauthenticated port for DNP, and then - unlock it. */ + /* Create an unauthenticated port for DNP. */ err = iohelp_create_empty_iouser (&user); if (! err) { @@ -249,8 +256,6 @@ diskfs_S_dir_lookup (struct protid *dircred, goto out; dirport = ports_get_send_right (newpi); - if (np != dnp) - pthread_mutex_unlock (&dnp->lock); struct fshelp_stat_cookie2 cookie = { .statp = &np->dn_stat, @@ -339,22 +344,8 @@ diskfs_S_dir_lookup (struct protid *dircred, /* ENOENT means there was a hiccup, and the translator vanished while NP was unlocked inside fshelp_fetch_root. - Reacquire the locks, and continue as normal. */ + Continue as normal. */ err = 0; - if (np != dnp) - { - if (!strcmp (filename, "..")) - pthread_mutex_lock (&dnp->lock); - else - { - if (pthread_mutex_trylock (&dnp->lock)) - { - pthread_mutex_unlock (&np->lock); - pthread_mutex_lock (&dnp->lock); - pthread_mutex_lock (&np->lock); - } - } - } } if (S_ISLNK (np->dn_stat.st_mode) @@ -427,6 +418,7 @@ diskfs_S_dir_lookup (struct protid *dircred, diskfs_nput (np); np = 0; + pthread_mutex_lock (&dnp->lock); if (filename == 0) /* symlink to "" was the last component */ { @@ -439,10 +431,7 @@ diskfs_S_dir_lookup (struct protid *dircred, { /* Handle normal nodes */ filename = nextname; - if (np == dnp) - diskfs_nrele (dnp); - else - diskfs_nput (dnp); + diskfs_nrele (dnp); if (!lastcomp) { dnp = np; @@ -562,14 +551,9 @@ diskfs_S_dir_lookup (struct protid *dircred, out: if (np) - { - if (dnp == np) - diskfs_nrele (np); - else - diskfs_nput (np); - } + diskfs_nput (np); if (dnp) - diskfs_nput (dnp); + diskfs_nrele (dnp); if (newpi) ports_port_deref (newpi); -- 2.56.0
