vfs: factor out mount point traversal to a dedicated routine

[mjg]

Summary

this preps other work, which may or may not be the recent reviews by @olce.freebsd_certner.fr

commit 35346230e6186214874c823bb84baea9b3499ec0
Author: Mateusz Guzik <[e-mail]>
Date:   Wed Jul 5 23:01:17 2023 +0000

    vfs: factor out mount point traversal to a dedicated routine
    
    While here tidy up asserts in the area.
    
    Reviewed by:
    Differential Revision:

commit 21bc84b65b389cc047b3d87b8112adb0df6504cf
Author: Mateusz Guzik <[e-mail]>
Date:   Wed Jul 5 22:56:04 2023 +0000

    vfs: stop passing LK_RETRY with LK_UPGRADE in vfs_lookup
    
    vn_lock already returns the expected error.
    
    Reviewed by:
    Differential Revision:

[kib]

I cannot understand the commit message vfs: stop passing LK_RETRY with LK_UPGRADE in vfs_lookup. Is it correct?

sys/kern/vfs_lookup.c

893

after moving mp initialization before the assert

[mjg]

In D40883#930924, @kib wrote:

I cannot understand the commit message vfs: stop passing LK_RETRY with LK_UPGRADE in vfs_lookup. Is it correct?

diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index 588ef88d24df..5a5560a4c26f 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
@@ -1314,11 +1314,9 @@ vfs_lookup(struct nameidata *ndp)
                                crosslkflags &= ~LK_SHARED;
                                crosslkflags |= LK_EXCLUSIVE | LK_CANRECURSE;
                        } else if ((crosslkflags & LK_EXCLUSIVE) != 0) {
-                               vn_lock(dp, LK_UPGRADE | LK_RETRY);
-                               if (VN_IS_DOOMED(dp)) {
-                                       error = ENOENT;
-                                       goto bad2;
-                               }
+                               error = vn_lock(dp, LK_UPGRADE);
+                               if (error != 0)
+                                       goto bad_unlocked;
                                if (dp->v_mountedhere != mp) {
                                        continue;
                                }

i can squash this commit

sys/kern/vfs_lookup.c

893

my line results in the expression dp->v_mountedhere != NULL in the panic string, which i find more readable, but i can change it if you insist

[kib]

In D40883#930935, @mjg wrote:

In D40883#930924, @kib wrote:

I cannot understand the commit message vfs: stop passing LK_RETRY with LK_UPGRADE in vfs_lookup. Is it correct?

diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index 588ef88d24df..5a5560a4c26f 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
@@ -1314,11 +1314,9 @@ vfs_lookup(struct nameidata *ndp)
                                crosslkflags &= ~LK_SHARED;
                                crosslkflags |= LK_EXCLUSIVE | LK_CANRECURSE;
                        } else if ((crosslkflags & LK_EXCLUSIVE) != 0) {
-                               vn_lock(dp, LK_UPGRADE | LK_RETRY);
-                               if (VN_IS_DOOMED(dp)) {
-                                       error = ENOENT;
-                                       goto bad2;
-                               }
+                               error = vn_lock(dp, LK_UPGRADE);
+                               if (error != 0)
+                                       goto bad_unlocked;
                                if (dp->v_mountedhere != mp) {
                                        continue;
                                }

i can squash this commit

This part is not included in the review diff.

sys/kern/vfs_lookup.c

893

Then perhaps VNASSERT() with more explanatory message is better. Or, as bde often suggested, do not check pointers for non-NULL, since deref is much stronger.

[mjg]

first it patches the routin

In D40883#930941, @kib wrote:

In D40883#930935, @mjg wrote:

In D40883#930924, @kib wrote:

I cannot understand the commit message vfs: stop passing LK_RETRY with LK_UPGRADE in vfs_lookup. Is it correct?

diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index 588ef88d24df..5a5560a4c26f 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
@@ -1314,11 +1314,9 @@ vfs_lookup(struct nameidata *ndp)
                                crosslkflags &= ~LK_SHARED;
                                crosslkflags |= LK_EXCLUSIVE | LK_CANRECURSE;
                        } else if ((crosslkflags & LK_EXCLUSIVE) != 0) {
-                               vn_lock(dp, LK_UPGRADE | LK_RETRY);
-                               if (VN_IS_DOOMED(dp)) {
-                                       error = ENOENT;
-                                       goto bad2;
-                               }
+                               error = vn_lock(dp, LK_UPGRADE);
+                               if (error != 0)
+                                       goto bad_unlocked;
                                if (dp->v_mountedhere != mp) {
                                        continue;
                                }

i can squash this commit

This part is not included in the review diff.

code got patched in place and then moved

sys/kern/vfs_lookup.c

893

panicking on a NULL pointer deref wont dump the vnode state normally seen with VNASSERT/VNPASS.

how about: VNPASS((vn_irflag_read(dp) & VIRF_MOUNTPOINT) != 0 && mp != NULL, dp);

[kib]

In D40883#930960, @mjg wrote:

first it patches the routin

In D40883#930941, @kib wrote:

In D40883#930935, @mjg wrote:

In D40883#930924, @kib wrote:

I cannot understand the commit message vfs: stop passing LK_RETRY with LK_UPGRADE in vfs_lookup. Is it correct?

diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index 588ef88d24df..5a5560a4c26f 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
@@ -1314,11 +1314,9 @@ vfs_lookup(struct nameidata *ndp)
                                crosslkflags &= ~LK_SHARED;
                                crosslkflags |= LK_EXCLUSIVE | LK_CANRECURSE;
                        } else if ((crosslkflags & LK_EXCLUSIVE) != 0) {
-                               vn_lock(dp, LK_UPGRADE | LK_RETRY);
-                               if (VN_IS_DOOMED(dp)) {
-                                       error = ENOENT;
-                                       goto bad2;
-                               }
+                               error = vn_lock(dp, LK_UPGRADE);
+                               if (error != 0)
+                                       goto bad_unlocked;
                                if (dp->v_mountedhere != mp) {
                                        continue;
                                }

i can squash this commit

This part is not included in the review diff.

code got patched in place and then moved

Unlike what the commit message says, code remove VN_IS_DOOMED() check, but the LK_RETRY flag is kept in place.

sys/kern/vfs_lookup.c

893

fine with me

[mjg]

indeed. that's weird, anyhow sorted out

[jah]

Looks like a similar cleanup can be done in the needs_exclusive_leaf case at the end of vfs_lookup().

[mjg]

huh, you just made me realize the committed change is buggy in that it fails to unlock dvp. i'll fix it up soon.

[jah]

In D40883#931131, @mjg wrote:

huh, you just made me realize the committed change is buggy in that it fails to unlock dvp. i'll fix it up soon.

@mjg do you still plan to fix this?

[olce]

Hi, indeed, this newly-introduced bug should be fixed, preferably before 14.0. Here's a proposal: D41731.