Bug 264723
| Summary: | the KERN_LOCKF sysctl returns kl_file_fsid that doesn't match st_dev from stat() | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Damjan Jovanovic <damjan.jov> |
| Component: | kern | Assignee: | Konstantin Belousov <kib> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | kib, markj |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
Konstantin that's your commit (eca39864f702e577eba3bc7e9992d1e5e56eba58), can you please have a look? Seems reasonable to me, but then the name kl_file_fsid should change to kl_file_dev, IMO. Or the code should use VOP_GETATTR instead (but this is probably slower?) and perform the same translation as other consumers. I intend to commit this patch after tinderbox, as is. Renaming the field is somewhat unfortunate proposal, because this stuff is already in stable. If doing the rename, we probably should still keep some compat #define. I propose to consider this later. (In reply to Konstantin Belousov from comment #3) The sysctl is brand new (not in releng/13.1), so it hasn't had much time to grow consumers and I doubt renaming will break much code. But it's just a suggestion, and indeed can be done later if at all. "fsid" is widely used as the name for dev_t fields in sys/user.h: struct kinfo_file, struct kinfo_ovmentry, struct kinfo_vmentry, and struct kinfo_vmobject use it. I don't think we should rename. We are not changing the meaning of the field, we're changing a meaningless field to meaningful field. Code can only break if it relies on meaningful values, but currently kl_file_fsid is meaningless, almost like an uninitialized variable. And it's only in CURRENT, so it can freely change. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=8ae769491303715c68e79aaf0e4e2f5c639151f9 commit 8ae769491303715c68e79aaf0e4e2f5c639151f9 Author: Damjan Jovanovic <damjan.jov@gmail.com> AuthorDate: 2022-06-17 13:28:16 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2022-06-18 09:34:17 +0000 KERN_LOCKF: report kl_file_fsid consistently with stat(2) PR: 264723 Reviewed by: kib Discussed with: markj MFC after: 1 week sys/kern/kern_lockf.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c1731fa54dd1eec310729e81e18dd601201405dc commit c1731fa54dd1eec310729e81e18dd601201405dc Author: Damjan Jovanovic <damjan.jov@gmail.com> AuthorDate: 2022-06-17 13:28:16 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2022-06-24 19:37:33 +0000 KERN_LOCKF: report kl_file_fsid consistently with stat(2) PR: 264723 (cherry picked from commit 8ae769491303715c68e79aaf0e4e2f5c639151f9) sys/kern/kern_lockf.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) |
The new KERN_LOCKF sysctl (great work btw) returns struct kinfo_lockf, whose kl_file_fsid matches neither the st_dev from stat(), nor the kf_file_fsid from kinfo_getfiles() (even though stat() and kinfo_getfiles() match each other). For example compare the FSID in "procstat advlock" for a locked file, with the st_dev in "stat -s" output on it. vop_stdstat() derives st_dev like this: ---snip--- if (vap->va_fsid != VNOVAL) sb->st_dev = vap->va_fsid; else sb->st_dev = vp->v_mount->mnt_stat.f_fsid.val[0]; ---snip--- vn_fill_kinfo_vnode() derives kf_file_fsid the same way: ---snip--- if (va.va_fsid != VNOVAL) kif->kf_un.kf_file.kf_file_fsid = va.va_fsid; else kif->kf_un.kf_file.kf_file_fsid = vp->v_mount->mnt_stat.f_fsid.val[0]; ---snip--- But vfs_report_lockf() uses the entire mnt_stat.f_fsid, not just array element 0, and never uses va.va_fsid: ---snip--- fsidx = mp->mnt_stat.f_fsid; ... memcpy(&klf->kl.kl_file_fsid, &fsidx, sizeof(fsidx)); ---snip--- vfs_report_lockf() does call VOP_STAT(), which is (usually?) the vop_stdstat() function above, so it should be able to just copy its correctly populated st_dev to kl_file_fsid. When I do that, as in the following patch, it seems to work, the FSID in "procstat advlock" matches the st_dev in "stat -s": ---snip--- diff --git a/sys/kern/kern_lockf.c b/sys/kern/kern_lockf.c index cad208197e7..98e29b2c929 100644 --- a/sys/kern/kern_lockf.c +++ b/sys/kern/kern_lockf.c @@ -2479,7 +2479,6 @@ vfs_report_lockf(struct mount *mp, struct sbuf *sb) struct ucred *ucred; char *fullpath, *freepath; struct stat stt; - fsid_t fsidx; STAILQ_HEAD(, kinfo_lockf_linked) locks; int error, gerror; @@ -2522,7 +2521,6 @@ vfs_report_lockf(struct mount *mp, struct sbuf *sb) gerror = 0; ucred = curthread->td_ucred; - fsidx = mp->mnt_stat.f_fsid; while ((klf = STAILQ_FIRST(&locks)) != NULL) { STAILQ_REMOVE_HEAD(&locks, link); vp = klf->vp; @@ -2532,8 +2530,7 @@ vfs_report_lockf(struct mount *mp, struct sbuf *sb) error = VOP_STAT(vp, &stt, ucred, NOCRED); VOP_UNLOCK(vp); if (error == 0) { - memcpy(&klf->kl.kl_file_fsid, &fsidx, - sizeof(fsidx)); + klf->kl.kl_file_fsid = stt.st_dev; klf->kl.kl_file_rdev = stt.st_rdev; klf->kl.kl_file_fileid = stt.st_ino; freepath = NULL; ---snip---