DragonFlyBSD

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 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.

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 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

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 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.

: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>

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

: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>

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 ;)
:
:-- 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

: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

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 ;)
:
:-- 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

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.