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.

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.

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.

I guess this was supposed to be SLOCK.

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.

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.

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.

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.

Yes, good catch.

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?

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.

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

Move this three lines to helper ?

Yes, will do.

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