ffs: skip upgrade from ro->rw if namei fails

[rew]

Summary

Move the namei() call above the MNT_UPDATE {} block and skip initialization within that block based on the namei() result.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256511

[kib]

You are locking (supposedly devfs) vnode for very long interval. For instance, ffs_flushfiles() now runs under this lock. I do not like it. If we ever lock devvp inside SU code, this would recurse.

[rew]

In D30870#694401, @kib wrote:

You are locking (supposedly devfs) vnode for very long interval.

Which lock? I dropped the LOCKLEAF flag from NDINIT() thinking the vnode would be unlocked after namei(). Then after the MNT_UPDATE block, grab the lock.

For instance, ffs_flushfiles() now runs under this lock.

Is it the namei() call or the vfs_unbusy/busy switch that causes ffs_flushfiles to run under a different lock than before?

[kib]

Ok, so you only referenced the devvp vnode in namei().

Well, this mean that there are some other issues:

• the vnode needs to be vunref()ed on all error returns inside the update block
• since vget() can return error, or it can return doomed vnode, we return to the similar situation as before, but under different condition.

No?

[rew]

I fumbled on the command line and hit this issue

looking at the code leaves me with a couple questions..

Is there a reason why the namei() lookup and the access checks can't
happen before MNT_UPDATE starts going through the motions?

I've updated the review to give context to what I mean.

Also, I haven't figured out the semantics of MNTK_FPLOOKUP - sys/mount.h
says it's used to indicate that fast path lookup is supported. If I understand, MNTK_FPLOOKUP gets disabled in the beginning of ffs_mount() and renabled
at the end? But on error, MNTK_FPLOOKUP doesn't get set again?
Maybe it doesn't need to be? I assume I'm missing something here.

[kib]

In D30870#768629, @rew wrote:

I fumbled on the command line and hit this issue

looking at the code leaves me with a couple questions..

Is there a reason why the namei() lookup and the access checks can't
happen before MNT_UPDATE starts going through the motions?

It can, but then you have more state to unwind (which your patch does not).

I've updated the review to give context to what I mean.

Also, I haven't figured out the semantics of MNTK_FPLOOKUP - sys/mount.h
says it's used to indicate that fast path lookup is supported. If I understand, MNTK_FPLOOKUP gets disabled in the beginning of ffs_mount() and renabled
at the end? But on error, MNTK_FPLOOKUP doesn't get set again?
Maybe it doesn't need to be? I assume I'm missing something here.

MNTK_FPLOOKUP is the indication of support for lockless cache lookup(9). So yes it is better to be kept since clearing it breaks the optimization. On the other hand, losing the flag does not make system unusable. This one is better to be fixed still, but in dedicated change.

[rew]

In D30870#768901, @kib wrote:

In D30870#768629, @rew wrote:

Is there a reason why the namei() lookup and the access checks can't
happen before MNT_UPDATE starts going through the motions?

It can, but then you have more state to unwind (which your patch does not).

What state needs to be unwound in the updated patch?

MNTK_FPLOOKUP is the indication of support for lockless cache lookup(9). So yes it is better to be kept since clearing it breaks the optimization. On the other hand, losing the flag does not make system unusable. This one is better to be fixed still, but in dedicated change.

Thanks, that makes sense.

[kib]

In D30870#768907, @rew wrote:

In D30870#768901, @kib wrote:

In D30870#768629, @rew wrote:

Is there a reason why the namei() lookup and the access checks can't
happen before MNT_UPDATE starts going through the motions?

It can, but then you have more state to unwind (which your patch does not).

What state needs to be unwound in the updated patch?

No state, I see that you vput/vrele devvp as needed.

[mckusick]

This looks good to me.