Add a fo_fill_kinfo fileops method.

[jhb]

Summary

Add a new fo_fill_kinfo method to add type-specific information to
struct kinfo_file.

• Move the various fill_*_info() methods out of kern_descrip.c and into the various file type implementations.
• Rework the support for kinfo_ofile to generate a suitable kinfo_file object for each file and then convert that to a kinfo_ofile structure rather than keeping a second, different set of code that directly manipulates type-specific file information.
• Remove the shm_path() and ksem_info() layering violations.

Test Plan

Booted with a new kernel and verified procstat -af still showed correct
type-specific information (such as paths, etc.)

[jhb]

sys/kern/kern_descrip.c

3136 ↗

(On Diff #1610)

The old code held the FILEDESC_SLOCK while building structure to keep the file object stable and vref'd the vnode when it had to drop the FILEDESC lock to call vn_fullpath(). In order to remove that special knowledge, I took the simple approach of fhold'ing the file while kinfo_file is populated. However, that means that if this races with close, the last close of a file might come from a random procstat(1) command. If this is ok I prefer this approach. Another option would be to pass the 'fdp' pointer down to the fo_fill_kinfo() methods so that they could drop the lock if needed. Then vn_fill_kinfo() could do its own vref() internally to keep the vnode stable for vn_fullpath.

sys/kern/sys_pipe.c

1622 ↗

(On Diff #1610)

The old fill_*_info() routines in kern_descrip.c all checked the f_data pointers against NULL. I think though that as a fileops method, all these NULL checks can't actually happen as fops should be set to badfileops before f_data can be cleared. So, I'm inclined to remove the checks for f_data being NULL from the various fill_kinfo() methods.

[jhb]

Don't return an error from badfo_fill_kinfo().

[mjg]

sys/kern/kern_descrip.c

3264 ↗

(On Diff #1634)

I guess this was supposed to be SLOCK.

3136 ↗

(On Diff #1610)

I vote for passing fdp down and relocking only when needed. With current patch we do a lot of atomic ops for no real gain.

[kib]

sys/kern/kern_descrip.c

3000 ↗

(On Diff #1610)

Would it allow to avoid the 'dummy' hack if you have two explicitely callable helpers, one to fill kif fields from vnode which do not depend on struct file content, and another to fill kif members which do not depend on struct file as well.

The rest of kif, which is initialized from the struct file, will be set directly in export_vnode_to_kinfo, instead of filling dummy.

3136 ↗

(On Diff #1610)

I think this is somewhat undesirable. The close release adv locks, which, I think, is best done in the context of the real lock owner. Another issue I can think of is the wrong credentials passed to VOP_CLOSE().

Also, slow device close would block procstat(1) instead of original process.

sys/kern/sys_pipe.c

1622 ↗

(On Diff #1610)

This actually depends on the sysctl thread doing a hold on the file, otherwise other thread, which actually owns the ref, could do fdrop ? Then we are called, but f_data is cleared afterward.

[jhb]

sys/kern/kern_descrip.c

3264 ↗

(On Diff #1634)

Yes, good catch.

3000 ↗

(On Diff #1610)

We would only need it for vnodes, so it could just be a direction function call of vn_fill_kinfo() or something similar. I had a similar thought as well.

sys/kern/sys_pipe.c

1622 ↗

(On Diff #1610)

Yes, but don't I in fact need the hold so it doesn't change out from under me? FILEDESC_SLOCK() is another way of doing a hold since closing the file requires an XLOCK to remove the entry from the table, yes?

[jhb]

sys/kern/kern_descrip.c

3136 ↗

(On Diff #1610)

Note that in terms of FILEDESC_SLOCK/SUNLOCK this is the same number of atomic ops as before (we unlocked it around sbuf_bcat() and SYSCTL_OUT() previously). It is only the fhold/fdrop that add extra ops. The gain was that it made the fill_kinfo methods simpler to write. No other fileops methods have to know if a filedesc lock is held that must be dropped before doing operations that would introduce a LOR like vn_fullpath(). However, I was more worried about correctness if a thread calling sysctl closed a file because it raced with close or exit.

[jhb]

Push FILEDESC lock down into fill_kinfo methods instead of holding file
across fill_kinfo call. Also, call into vnode code directly when exporting
vnodes instead of creating a dummy file object.

[jhb]

Remove unneeded NULL checks.

[kib]

sys/kern/kern_descrip.c

2933 ↗

(On Diff #1692)

Might be, take a chance to make the struct static const ?

3012 ↗

(On Diff #1692)

Move this three lines to helper ?

[jhb]

sys/kern/kern_descrip.c

2933 ↗

(On Diff #1692)

Yes, will do.

3012 ↗

(On Diff #1692)

Ok. I almost did it originally but was worried at the proliferation of helper routines. However, I agree it is cleaner to consolidate this.

[jhb]

Fixes from Konstantin.

[jhb]

Closed by commit rS271976 (authored by @jhb).