Bug #1013
closed
atimes of binaries not properly updated
Added by corecode almost 16 years ago. Updated over 15 years ago.
0%
Description
Hey,
something is wrong with our atimes update:
% stat -x `which mplayer`
File: "/usr/pkg/bin/mplayer"
Size: 21084730 FileType: Regular File
Mode: (0555/-r-xr-xr-x) Uid: ( 0/ root) Gid: ( 0/
wheel)
Device: 91,3 Inode: 579162 Links: 1
Access: Fri May 2 01:00:44 2008
Modify: Wed Apr 30 00:16:03 2008
Change: Wed Apr 30 00:16:03 2008
But I'm sure I watched MacGyver yesterday night, with mplayer. Maybe it
was in the buffer cache since May 2nd and thus was not updated? Actually
running mplayer right now (after reboot) does not update the atime either.
cheers
simon
Updated by dblazakis almost 16 years ago
On Mon, May 19, 2008 at 2:26 PM, Simon 'corecode' Schubert
<corecode@fs.ei.tum.de> wrote:
Hey,
something is wrong with our atimes update:
% stat -x `which mplayer`
File: "/usr/pkg/bin/mplayer"
Size: 21084730 FileType: Regular File
Mode: (0555/-r-xr-xr-x) Uid: ( 0/ root) Gid: ( 0/ wheel)
Device: 91,3 Inode: 579162 Links: 1
Access: Fri May 2 01:00:44 2008
Modify: Wed Apr 30 00:16:03 2008
Change: Wed Apr 30 00:16:03 2008But I'm sure I watched MacGyver yesterday night, with mplayer. Maybe it was
in the buffer cache since May 2nd and thus was not updated? Actually
running mplayer right now (after reboot) does not update the atime either.
I took a look and it seems like bumping atime on a execve or mmap was
never done. The other BSDs and Linux do. I took a shot at a patch.
It is based on FBSD.
The patch against HEAD is at:
http://dblaz.beevomit.org/dfly/atime.patch
I wasn't sure if the atime update in mmap should happen further below
or not. Also, not sure if I had to mess with vnode locking in
kern_exec -- please let me know.
-- Dion
Ah, and I just thought that maybe our stat(2) man page should be
updated to add execve and mmap in the atime blurb.
Updated by dblazakis almost 16 years ago
On Tue, May 20, 2008 at 3:53 PM, Dionysus Blazakis
<dion.blazakis@gmail.com> wrote:
On Mon, May 19, 2008 at 2:26 PM, Simon 'corecode' Schubert
<corecode@fs.ei.tum.de> wrote:Hey,
something is wrong with our atimes update:
% stat -x `which mplayer`
File: "/usr/pkg/bin/mplayer"
Size: 21084730 FileType: Regular File
Mode: (0555/-r-xr-xr-x) Uid: ( 0/ root) Gid: ( 0/ wheel)
Device: 91,3 Inode: 579162 Links: 1
Access: Fri May 2 01:00:44 2008
Modify: Wed Apr 30 00:16:03 2008
Change: Wed Apr 30 00:16:03 2008But I'm sure I watched MacGyver yesterday night, with mplayer. Maybe it was
in the buffer cache since May 2nd and thus was not updated? Actually
running mplayer right now (after reboot) does not update the atime either.I took a look and it seems like bumping atime on a execve or mmap was
never done. The other BSDs and Linux do. I took a shot at a patch.
It is based on FBSD.The patch against HEAD is at:
http://dblaz.beevomit.org/dfly/atime.patch
Based on some discussion in IRC, I added a check in case vn_mark_atime
was called without a process context. For now, it will use NOCRED in
this case (equivalent to NULL).
I also added the note to the man page.
Updated patch:
http://dblaz.beevomit.org/dfly/atime-2.patch
-- Dion
Updated by dblazakis almost 16 years ago
On Wed, May 21, 2008 at 4:03 PM, Dionysus Blazakis
<dion.blazakis@gmail.com> wrote:
On Tue, May 20, 2008 at 3:53 PM, Dionysus Blazakis
<dion.blazakis@gmail.com> wrote:On Mon, May 19, 2008 at 2:26 PM, Simon 'corecode' Schubert
<corecode@fs.ei.tum.de> wrote:Hey,
something is wrong with our atimes update:
% stat -x `which mplayer`
File: "/usr/pkg/bin/mplayer"
Size: 21084730 FileType: Regular File
Mode: (0555/-r-xr-xr-x) Uid: ( 0/ root) Gid: ( 0/ wheel)
Device: 91,3 Inode: 579162 Links: 1
Access: Fri May 2 01:00:44 2008
Modify: Wed Apr 30 00:16:03 2008
Change: Wed Apr 30 00:16:03 2008But I'm sure I watched MacGyver yesterday night, with mplayer. Maybe it was
in the buffer cache since May 2nd and thus was not updated? Actually
running mplayer right now (after reboot) does not update the atime either.I took a look and it seems like bumping atime on a execve or mmap was
never done. The other BSDs and Linux do. I took a shot at a patch.
It is based on FBSD.The patch against HEAD is at:
http://dblaz.beevomit.org/dfly/atime.patchBased on some discussion in IRC, I added a check in case vn_mark_atime
was called without a process context. For now, it will use NOCRED in
this case (equivalent to NULL).I also added the note to the man page.
Updated patch:
http://dblaz.beevomit.org/dfly/atime-2.patch-- Dion
I wasn't sure if the atime update in mmap should happen further below
or not. Also, not sure if I had to mess with vnode locking in
kern_exec -- please let me know.-- Dion
Ah, and I just thought that maybe our stat(2) man page should be
updated to add execve and mmap in the atime blurb.cheers
simon--
Serve - BSD ++ RENT this banner advert ++ ASCII Ribbon /"\
Work - Mac ++ space for low €€€ NOW!1 ++ Campaign \ /
Party Enjoy Relax | http://dragonflybsd.org Against HTML \
Dude 2c 2 the max ! http://golden-apple.biz Mail + News / \
Hrm. Nevermind. The ucred's passed to the fs vnops are dereferenced
without check.
Updated by dillon almost 16 years ago
:Hrm. Nevermind. The ucred's passed to the fs vnops are dereferenced
:without check.
You can try passing FSCRED, which is basically a root cred from
process 0.-Matt
Matthew Dillon
<dillon@backplane.com>
Updated by dblazakis almost 16 years ago
On Wed, May 21, 2008 at 8:19 PM, Matthew Dillon
<dillon@apollo.backplane.com> wrote:
:Hrm. Nevermind. The ucred's passed to the fs vnops are dereferenced
:without check.You can try passing FSCRED, which is basically a root cred from
process 0.-Matt
Matthew Dillon
<dillon@backplane.com>
The issue is that FSCRED and NOCRED are checked in the kern_prot.c
helper functions, but the vnops functions in the various file systems
dereference the struct ucred pointers without checking for a
NULL or 0xFFFFFFFF(FSCRED) pointers. So what is the ideal
solution? Should the ucred API be extended in kern_prot to do the uid
check that the file systems do (while taking into account NOCRED and
FSCRED)?
And I thought this was such a simple patch ;)
-- Dion
Updated by dillon almost 16 years ago
:The issue is that FSCRED and NOCRED are checked in the kern_prot.c
:helper functions, but the vnops functions in the various file systems
:dereference the struct ucred pointers without checking for a
:NULL or 0xFFFFFFFF(FSCRED) pointers. So what is the ideal
:solution? Should the ucred API be extended in kern_prot to do the uid
:check that the file systems do (while taking into account NOCRED and
:FSCRED)?
:
:And I thought this was such a simple patch ;)
:
:-- Dion
No, I would not change the API.There are three 'real' creds available that can be used. First, there
is the current process cred. Second, there is the cred stored with
the file pointer (struct file -> f_cred), and third there is the
cred associated with the proc0 (proc0.p_ucred).After looking at your patch I think we actually should avoid using
SETATTR to update the atime. That is a very active and expensive VOP
and kinda messes up the critical path.If we want to update the atime from exec*() and from mmap() we should
have access to the cred in the file pointer. What I would then do is
actually create a new VOP op and let the filesystem do whatever internal
actions it feels is reasonable to do the update. So, e.g. we would
implement a new VOP called, say, VOP_SIGNAL_ACCESS(vp, cred) (maybe you
can come up with a better name), with a default operation which is just
a NOP. UFS and HAMMER would then implement that VOP (the only two
filesystems we really care about insofar as atime goes), allowing them
to optimize the operation.Our exec and mmap code would then call the VOP.How does that sound for a plan?-Matt
Matthew Dillon
<dillon@backplane.com>
Updated by dblazakis almost 16 years ago
On Fri, May 23, 2008 at 7:16 PM, Matthew Dillon
<dillon@apollo.backplane.com> wrote:
:The issue is that FSCRED and NOCRED are checked in the kern_prot.c
:helper functions, but the vnops functions in the various file systems
:dereference the struct ucred pointers without checking for a
:NULL or 0xFFFFFFFF(FSCRED) pointers. So what is the ideal
:solution? Should the ucred API be extended in kern_prot to do the uid
:check that the file systems do (while taking into account NOCRED and
:FSCRED)?
:
:And I thought this was such a simple patch ;)
:
:-- DionNo, I would not change the API.
There are three 'real' creds available that can be used. First, there
is the current process cred. Second, there is the cred stored with
the file pointer (struct file -> f_cred), and third there is the
cred associated with the proc0 (proc0.p_ucred).
Ah. I suppose the proc0 cred would be the one to use when no process
context is available, at least, that's how it is done when
initializing f_cred.
Sound good to me. Pretty much what FBSD does. I just wasn't sure if
adding another VOP was the way to go -- I didn't realize the setattr
was expensive. It would explain their choice.
Also, I was thinking about how to add a helper function for chmod
setattr code, but it seems you've already done it ;)
-- Dion
Updated by dillon almost 16 years ago
:Sound good to me. Pretty much what FBSD does. I just wasn't sure if
:adding another VOP was the way to go -- I didn't realize the setattr
:was expensive. It would explain their choice.
:
:Also, I was thinking about how to add a helper function for chmod
:setattr code, but it seems you've already done it ;)
:
:-- Dion
Yah, I hadn't quite finished implement HAMMER's setattr. It should
be fully implemented now.-Matt
Updated by dblazakis almost 16 years ago
On Mon, May 26, 2008 at 1:28 PM, Matthew Dillon
<dillon@apollo.backplane.com> wrote:
:Sound good to me. Pretty much what FBSD does. I just wasn't sure if
:adding another VOP was the way to go -- I didn't realize the setattr
:was expensive. It would explain their choice.
:
:Also, I was thinking about how to add a helper function for chmod
:setattr code, but it seems you've already done it ;)
:
:-- DionYah, I hadn't quite finished implement HAMMER's setattr. It should
be fully implemented now.-Matt
New patch with latest plan:
http://dblaz.beevomit.org/dfly/atime-4.patch
Implemented the new VOP for UFS, not for HAMMER.
-- Dion
Updated by dillon over 15 years ago
Committed with modifications. My modifications removed the timespec passed to
the VOP, implemented the KNOTE for UFS, implemented the operation for HAMMER,
and supplied a default VOP function for remaining VFS's.