Page MenuHomeFreeBSD

Add KERN_LOCKF
ClosedPublic

Authored by kib on Apr 2 2022, 10:41 PM.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Apr 2 2022, 10:41 PM
kib edited the summary of this revision. (Show Details)

Add libprocstat bits. Still no procstat(1).

I'll admit I didn't look at the code in detail, but it looks ok to me.

It won't be useful for NFSv4, since the locks are not kept in
the generic lock structures (except to the NFS server when
vfs.nfsd.enable_locallocks is set non-zero).

A VOP call would allow the NFSv4 client to report locks
(but not their file pathnames), so I don't know if it is worth
the bother?

ps: Good to see that you are at least able to do some stuff.

I can't imagine what living in Kyiv would be like right now.

I'll admit I didn't look at the code in detail, but it looks ok to me.

It won't be useful for NFSv4, since the locks are not kept in
the generic lock structures (except to the NFS server when
vfs.nfsd.enable_locallocks is set non-zero).

A VOP call would allow the NFSv4 client to report locks
(but not their file pathnames), so I don't know if it is worth
the bother?

pathes are documented in libprocstat(3) as optional.

When I thought about the code structure, I indeed considered two options: either iterate over lf_lock_states list, as is done in the current review, or iterate over all vnodes and then looking at v_lockf for it. Second option did not stayed for too long, because we typically have several millions of vnodes cached, while typical system only have several dozens of locks. Second option was considered at all because the locking is more natural for it: vnode lock is before lockf state locks. I have to do some trick which slightly decreases the consistency of the reported snapshot.

May be we could have some additional code to iterate over NFSv4+ mounts to gather fs-specific adv lock implementation data. This definitely cannot be VOP, it should be VFS_OP-like interface, for later.

sys/kern/kern_lockf.c
2525 ↗(On Diff #104534)

How does fsidx get set. Maybe I just can't spot it,
but I don't see it being set in the above code?

2530 ↗(On Diff #104534)

Although it would probably be less efficient,
due to making multiple passes through the above
for each local fs, if the code from line #2494 to here
were "per file system", then it could be a VFS_xxx() op,
with the above being the vfs_stdxxxop().
(That's why I was looking for where fsidx got set.)

And, yes, you were correct. For the NFS client, it
would need to be a VFS_xxx() call that returns all
locks for the file system.

Or, maybe it could do the above to cover the local
file systems and then a VFS_xxx() call for each remote
fs?

Anyhow, I'll leave it up to you.
As you noted, it could be a future enhancement, maybe?

kib marked 2 inline comments as done.Apr 4 2022, 12:09 AM
kib added inline comments.
sys/kern/kern_lockf.c
2525 ↗(On Diff #104534)

Right, it should has been moved to the loop below.

kib marked an inline comment as done.

Use per-fs vfs_report_lockf method. If it is null, standard implementation is used.

sys/kern/kern_lockf.c
2494 ↗(On Diff #104543)

My concern with this version is performance when there
are a lot of file systems. Peter Errikson sets up storage
servers with 20,000 file systems on them. (I think he had
one that had over 80,000 file systems on it.)

I'm almost tempted to suggest going back to your previous
version (one pass through all lf_advlock() locks) and then
(it doesn't need to be done now) possibly a VFS_REPORT_LOCKF()
call done for all mounts. In this case, the default call would just
return. Then file systems that keep locks other ways, like the
NFSv4 client, could implement a non-NULL VFS_REPORT_LOCKF().

Anyhow, do whatever you think is best.

kib marked an inline comment as done.Apr 4 2022, 10:51 PM
kib added inline comments.
sys/kern/kern_lockf.c
2494 ↗(On Diff #104543)

This code is definitely not a hot path. In worst case, the report of the advisory locks list would take a long time.

If somebody complains, we can do some optimizations, for now I prefer to keep your advise to allow for eventual proper reporting for private adv locking implementations.

Looks fine to me now.

I can cobble to-gether a nfs_report_lockf() once this goes
into main.

This revision is now accepted and ready to land.Apr 4 2022, 11:10 PM
sys/kern/kern_lockf.c
2492 ↗(On Diff #104543)

How is it possible to have vp == NULL?

2530 ↗(On Diff #104543)

Did you mean to include LK_RETRY here? vn_lock(LK_RETRY) never fails.

2541 ↗(On Diff #104543)

What's the purpose of initializing fullpath?

2652 ↗(On Diff #104543)

This fits on one line.

kib marked 5 inline comments as done.Apr 6 2022, 11:42 PM
kib added inline comments.
sys/kern/kern_lockf.c
2541 ↗(On Diff #104543)

Because it is done by other places. I am not sure at what point vn_fullpath() started doing something to fullpath in case of error.

For now I kept it as is. If you insist, I would remove the initialization.

kib marked an inline comment as done.
This revision now requires review to proceed.Apr 6 2022, 11:43 PM
markj added inline comments.
lib/libprocstat/libprocstat.3
507 ↗(On Diff #104694)
509 ↗(On Diff #104694)
515 ↗(On Diff #104694)
518 ↗(On Diff #104694)

Maybe, "For each lock, unique identifiers for the locked file and its mount point are guaranteed to be provided."

sys/sys/user.h
475 ↗(On Diff #104694)

Consider renaming them to KLOCKF_* to match the "kinfo_lockf" name.

This revision is now accepted and ready to land.Apr 7 2022, 12:04 AM
kib marked 5 inline comments as done.

Rename symbols KLOCK_ -> KLOCKF_.
Man page editing.

This revision now requires review to proceed.Apr 7 2022, 1:23 AM
markj added inline comments.
lib/libprocstat/libprocstat.3
507 ↗(On Diff #104700)
509 ↗(On Diff #104700)

missed it before, sorry

This revision is now accepted and ready to land.Apr 7 2022, 2:08 AM
kib marked 2 inline comments as done.Apr 7 2022, 1:39 PM