On Sun, 2 Dec 2007, Don Lewis wrote:

Yes, already done (except vprint() instead of panic).

But I let these happen and discard IN_ATIME marks if fs_ronly. I thought that the optimization went the other way -- unconditionally setting the marks was very efficient, and discarding them in ufs_itimes() was efficient too. I think this is still true now with larger locking overheads, and the marks should be discarded later in the MNT_NOATIME case too. It is expected that the marks are set much more often than they are looked at by ufs_itimes(), since most calls to ufs_itimes() are in close() and read() is much more common than close().

ufs_itimes() is also called in stat() but I think that is less common than close() (except for some tree walks). WIth non-delayed marking, ufs_itimes() would still have to check fs_ronly, and the only gain would be that it could then skip checking the marks except as an invariants check. But it can gain like that even with delayed setting -- just ignore any old marks while fs_ronly (except as an invariants check), but clear them at mount or unmount time so that there shouldn't be any.

Thanks, I had forgotten about that. In vfs_mark_atime(), there is much more efficiency to be gained by not setting marks that will be discarded, since it takes a VOP to set them and many file systems don't support this setting. However, it is hard for vfs_mark_atime() to know when the mark will be discarded without calling the fs:

- it already doesn't know which fs's support it - it should be checking fs_ronly for ffs

- it seems to be missing locking for MNT_NOATIME and MNT_RDONLY

fs-level locking for MNT_NOATIME and MNT_RDONLY and fs_ronly is dubious too. Upper layers set the MNT flags before giving VOP_MOUNT() a chance to adjust the marks. This is automatically safe in one direction only (e.g., setting MNT_NOATIME or MNT_RDONLY is fail-safe since it stops changes), and always bad for strict invariants.

I now use the following fixes:

% Index: ufs_vnops.c % =================================================================== % RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v % retrieving revision 1.293 % diff -u -2 -r1.293 ufs_vnops.c % --- ufs_vnops.c 8 Nov 2007 17:21:51 -0000 1.293 % +++ ufs_vnops.c 2 Dec 2007 04:56:58 -0000 % @@ -89,4 +89,5 @@ % #endif % % +#include <ufs/ffs/fs.h> % #include <ufs/ffs/ffs_extern.h> % % @@ -137,8 +138,38 @@ % % ip = VTOI(vp); % + /* % + * MNT_RDONLY can barely be trusted here. Full r/o mode is indicated % + * by fs_ronly, and the MNT_RDONLY setting [should] differ from the % + * fs_ronly setting only during transition from r/w mode to r/o mode. % + * We set IN_ACCESS even in full r/o mode, so we must discard it % + * unconditionally here. During the transition, we must convert % + * IN_CHANGE | IN_UPDATE to IN_MODIFIED, since some callers neglect % + * to set IN_MODIFIED. We also set the timestamps indicated by % + * IN_CHANGE | IN_UPDATE normally during the transition, since the % + * update marks may have been set correctly before the transition and % + * not yet converted into timestamps. Callers that set IN_CHANGE | % + * IN_UPDATE during the transition are buggy, since userland writes % + * are supposed to be denied (by MNT_RDONLY checks) during the % + * transition, while kernel writes should should only be for syncs % + * and syncs should not touch timestamps except to convert old % + * update marks to timestamps. Callers that set any update mark or % + * modification flag except IN_ACCESS while in full r/o mode are % + * broken; we will panic for them later. % + */ % if ((vp->v_mount->mnt_flag & MNT_RDONLY) != 0) % - goto out; % + ip->i_flag &= ~IN_ACCESS;