vfs: rename vop_mmapped() to vop_update_atime()
AcceptedPublic
Actions

Authored by kib on Fri, Jun 19, 7:56 PM.

Details

Reviewers

rmacklem
mckusick
jah
markj

Summary

This reflects the actual functionality of the VOP.  While there, add the
explicit struct timespec argument for the VOP allowing the caller to set
specific atime, not just request an update for it.

Requested by:   rmacklem

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Fri, Jun 19, 7:56 PM

Herald added subscribers: olce, imp. · View Herald TranscriptFri, Jun 19, 7:56 PM

What is not clear to me, should ufs_itimes() only use the passed timespec for atime update, or it shall be used for all times as I did in the initial version of the patch.
Perhaps this is the question to Rick, since I believe the only caller with tsa != NULL would be nfs server.

Had considered changing this name too, glad to see this.

What's the expected semantics on passing an explicit atime? The UFS implementation here may cause setting the modified and created times in addition to the access one depending on present inode flags. Is that intended?

In D57681#1322365, @kib wrote:

What is not clear to me, should ufs_itimes() only use the passed timespec for atime update, or it shall be used for all times as I did in the initial version of the patch.
Perhaps this is the question to Rick, since I believe the only caller with tsa != NULL would be nfs server.

I would say that, if VOP_SET_ATIME() is only being used for atime,
then the tsa != NULL should only apply to atime.

(The NFS client can provide separate times for atime/mtime/birthtime,
but that results in multiple VOP_SETATTR() calls for each. It is the atime
case for read-only file systems like ZFS snapshots that caused the problem
where the server would reply EROFS for attempting to update atime for
a clone/copy from the read-only fs.)

Limit tsa usage in ufs_itimes() to atime updates.

This looks ok to me, although I'll admit I am not
conversant with UFS semantics, so I'll leave that
to others.

This revision is now accepted and ready to land.Fri, Jun 19, 8:53 PM

In D57681#1322456, @rmacklem wrote:

This looks ok to me, although I'll admit I am not
conversant with UFS semantics, so I'll leave that
to others.

All existing callers pass tsa == NULL, so the intent is that the semantic does not change.
It is the question whether your case is served when tsa != NULL is provided.

In D57681#1322466, @kib wrote:

In D57681#1322456, @rmacklem wrote:

This looks ok to me, although I'll admit I am not
conversant with UFS semantics, so I'll leave that
to others.

All existing callers pass tsa == NULL, so the intent is that the semantic does not change.
It is the question whether your case is served when tsa != NULL is provided.

I guess it does. I don't know UFS, but it does seem "inefficient" to first do
UFS_INODE_SET_FLAG_SHARED(), which locks/unlocks the vnode interlock and
then call ufs_itimes(), which locks the vnode interlock again and calls ufs_itimes_locked().

Maybe ufs_update_atime() should..

		if (ap->a_ts != NULL) {
                        VI_LOCK(vp);
                        ITOV(vp)->i_flags |= IN_ACCESS;
			ufs_itimes_locked(vp, ap->a_ts);
                        VI_UNLOCK(vp);
                } else
                        UFS_INODE_SET_FLAG_SHARED(ip, IN_ACCESS);

Instead of what is in line#852-854. But I doubt it makes any
difference from a correctness point of view.

Don't relock the interlock.

This revision now requires review to proceed.Fri, Jun 19, 10:24 PM

rmacklem accepted this revision.Fri, Jun 19, 10:50 PM

This revision is now accepted and ready to land.Fri, Jun 19, 10:50 PM

jah added inline comments.Fri, Jun 19, 10:52 PM

sys/ufs/ufs/ufs_vnops.c
169–175	Same question as @olce above: when tsa != NULL, would we ever expect IN_UPDATE/IN_CHANGE to be set on the inode, and if so would the desired behavior always be to set them to the current time instead of tsa? Also, it sounds as though the tsa != NULL case is in support of some upcoming NFS feature?

rmacklem added inline comments.Sat, Jun 20, 12:21 AM

sys/ufs/ufs/ufs_vnops.c
169–175	Not an upcoming feature. Commit b5815ee99a01 is what kib@ referred to when he suggested I look at the use of VOP_MMAPPED() for it. The NFS problem/fix is somewhat convoluted.. The NFSv4.2 Clone/Copy operations are not defined to update atime on the input file. As such, what happens to atime when done on an NFSv4.2 server is undefined. An email discussion on a FreeBSD list (you could find it in the email archives if you really wanted to) resulted in a concensus that atime should be updated. As such, the solution was to add a Setattr of atime to the compound RPC that does Clone or Copy against the NFSv4.2 server. The FreeBSD NFSv4.2 server does a VOP_SETATTR() of atime, which fails with EROFS for a read-only fs (the reported problem was a ZFS snapshot). kib@ suggested that VOP_MMAPPED() could be used instead of VOP_SETATTR() to set atime and avoid this failure. However, the NFSv4 Setattr can specify either setting of atime to the server's time or to a time provided by the client (the FreeBSD client does the former, but who knows what other NFSv4.2 clients might choose to do). Therefore, I suggested the optional "time" argument to handle the case where the NFSv4.2 client sets atime with a client provided time. For VOP_UPDATE_ATIME() { VOP_MMAPPED() renamed } I would say the answer is no for the NFS server's use. (As in, no to setting any time other than atime.) Basically, the difference between using VOP_SETATTR() and VOP_MMAPPED() is that the latter will not "fail with EROFS" for a read-only fs and, as such, is preferable to use when only setting atime.