Bug #1013

atimes of binaries not properly updated

Added by corecode about 6 years ago. Updated over 5 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

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

History

#1 Updated by dblazakis about 6 years ago

On Mon, May 19, 2008 at 2:26 PM, Simon 'corecode' Schubert
<> 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 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.

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.

#2 Updated by dblazakis about 6 years ago

On Tue, May 20, 2008 at 3:53 PM, Dionysus Blazakis
<> wrote:
> On Mon, May 19, 2008 at 2:26 PM, Simon 'corecode' Schubert
> <> 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 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.
>
> 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

#3 Updated by dblazakis about 6 years ago

On Wed, May 21, 2008 at 4:03 PM, Dionysus Blazakis
<> wrote:
> On Tue, May 20, 2008 at 3:53 PM, Dionysus Blazakis
> <> wrote:
>> On Mon, May 19, 2008 at 2:26 PM, Simon 'corecode' Schubert
>> <> 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 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.
>>
>> 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
>
>>
>> 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.

#4 Updated by dillon about 6 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
<>

#5 Updated by dblazakis about 6 years ago

On Wed, May 21, 2008 at 8:19 PM, Matthew Dillon
<> 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
> <>
>

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(NOCRED) 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

#6 Updated by dillon about 6 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(NOCRED) 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
<>

#7 Updated by dblazakis about 6 years ago

On Fri, May 23, 2008 at 7:16 PM, Matthew Dillon
<> 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(NOCRED) 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).

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

#8 Updated by dillon about 6 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

#9 Updated by dblazakis about 6 years ago

On Mon, May 26, 2008 at 1:28 PM, Matthew Dillon
<> 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 ;)
> :
> :-- Dion
>
> Yah, 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

#10 Updated by dillon over 5 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.

Also available in: Atom PDF