Date: Thu, 24 Jul 2008 01:42:51 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jaakko Heinonen <jh@saunalahti.fi> Cc: freebsd-fs@freebsd.org, ighighi@gmail.com Subject: Re: birthtime initialization Message-ID: <20080724000618.Q16961@besplex.bde.org> In-Reply-To: <20080723103424.GA1856@a91-153-120-204.elisa-laajakaista.fi> References: <200806020800.m528038T072838@freefall.freebsd.org> <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi> <20080722215249.K17453@delplex.bde.org> <20080723103424.GA1856@a91-153-120-204.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 23 Jul 2008, Jaakko Heinonen wrote:
> On 2008-07-22, Bruce Evans wrote:
>>> + VATTR_NULL(vap);
>>
>> I want to initialize va_birthtime to { -1, 0 } here only. Don't
>> initialize the whole vattr here. VOP_GETTATR() is supposed to initalize
>> everything, but doesn't for va_birthtime. If there any other fields
>> that VOP_GETTATR() doesn't initialize, then these should be searched
>> for and fixed instead of setting them to the garbage value given by
>> vattr_null.
>
> At least xfs gets it wrong for several fields.
>
> /*
> * Fields with no direct equivalent in XFS
> * leave initialized by VATTR_NULL
> */
> #if 0
> vap->va_filerev = 0;
> vap->va_birthtime = va.va_ctime;
> vap->va_vaflags = 0;
> vap->va_flags = 0;
> vap->va_spare = 0;
> #endif
That's amazingly bad.
First, the fields shouldn't be initialized using VATTR_NULL() in
VOP_GETATTR(). Doing so breaks the preinitialization that we want to
add (maybe also layering). This bug seems to be present in only the
following file systems:
fdescfs, mqfs, pseudofs, tmpfs, xfs
The uninitialized fields should give stack garbage.
Second, VNOVAL is an extremly bogus default value. For va_flags, it
gives all flags set, so ls -lo output would be weird (and wrong since
the flags aren't actually there).
Third, va_vaflags and va_spare aren't part of the VOP_GETATTR() API.
va_vaflags is an input parameter for VOP_SETATTR(). va_spare is just
spare (unused). VATTR_NULL() initializes va_vaflags to 0, not VNOVAL
(as is required for the usual case in VOP_SETTATR()), and it knows
better than to initialize unused fields (it also doesn't initialize
unnamed padding -- stack garbage in this is OK since vattr is never
copied directly to userland).
After deleting the bogus initializations, we're left with va_filerev,
va_birthtime and va_flags. Most file systems don't support these, so
they could usefully all be handled by defaulting them as in the proposed
changes for va_birthtime.
>>> Index: sys/ufs/ufs/ufs_vnops.c
>>> ...
>>> Index: sys/fs/msdosfs/msdosfs_vnops.c
>>> ...
>>> Index: sys/nfsclient/nfs_subs.c
>>
>> There are a probably more file systems that have missing or slightly
>> incorrect (all zero) settings of va_birthtime.
>
> Many file systems misses settings of va_birthtime. That's the reason why
> I initialized it in vn_stat(). I have seen four types of
> initializations:
>
> 1) Support and set birthtime. (UFS2, tmpfs, msdosfs (not all
> variants of msdosfs support birthtime), nfs4?)
>
> 2) Set birthtime to zero. (UFS1, nfs (nfs zeroes the vattr structure))
Zeroing it is almost as bad as VATTR_NULL()ing it.
> 3) Initialize vattr with VATTR_NULL() but not birthtime explicitly. Thus
> tv_sec and tv_nsec are set to -1 (VNOVAL). (devfs, xfs, portalfs,
> pseudofs)
>
> 4) Not initialize birthtime at all. Those would be fixed by initializing the
> birthtime in vn_stat(). (cd9660, hpfs, ntfs, smbfs, udf, ext2fs,
> reiserfs)
> I couldn't test but I suspect that also coda belongs to this group.
>
> So I see two ways to fix:
>
> - initialize birthtime in vn_stat() and add/fix explicit setting for group 2
> and 3 file systems or
> - add explicit initialization to all file systems missing it
> (groups 3 and 4) and fix group 2 to initialize birthtime to correct value
(3) and (4) are only different due to bugs. I want to initialize
va_birthtime and maybe a couple of other fields in vn_stat(), and depend
on this and not initialize to the same or a worse value in case (3). This
requires removing VATTR_NULL() or zeroing in some cases and checking that
everything is still initialized. All old fields should be handled by
explicit initialization as in ffs1, and all new fields should have defaults.
> I have updated the patch per your comments and checked more file
> systems. I have verified that with this patch these file systems return
> correct birthtime values (real birthtime or {-1, 0} if not supported):
>
> UFS2, UFS1, cd9660, nfs, ext2fs, smbfs, reiserfs, xfs, ntfs, devfs,
> procfs, linprocfs, tmpfs, msdosfs, portalfs, udf.
I don't want the case (3). Otherwise good.
>
> For pseudofs I set birthtime to current time.
I don't like this. birthtime should be <= all other file times. If
a file system doesn't support birthtime, then it could also set birthtime
= mtime, but that isn't useful either. Better set it to -1 as in ffs1
(exept ffs1 set it to 0).
>
> %%%
> Index: sys/kern/vfs_vnops.c
> ===================================================================
> --- sys/kern/vfs_vnops.c (revision 180729)
> +++ sys/kern/vfs_vnops.c (working copy)
> @@ -703,6 +703,13 @@ vn_stat(vp, sb, active_cred, file_cred,
> #endif
>
> vap = &vattr;
> +
> + /*
> + * Not all file systems initialize birthtime.
> + */
Change to something like:
/*
* Initialize defaults for new and/or unusual fields, so that file
* systems which don't support these fields don't need to know
* about them.
*/
> + vap->va_birthtime.tv_sec = -1;
> + vap->va_birthtime.tv_nsec = 0;
> +
> error = VOP_GETATTR(vp, vap, active_cred, td);
> if (error)
> return (error);
> Index: sys/ufs/ufs/ufs_vnops.c
> ===================================================================
> --- sys/ufs/ufs/ufs_vnops.c (revision 180729)
> +++ sys/ufs/ufs/ufs_vnops.c (working copy)
> @@ -410,7 +410,7 @@ ufs_getattr(ap)
> vap->va_mtime.tv_nsec = ip->i_din1->di_mtimensec;
> vap->va_ctime.tv_sec = ip->i_din1->di_ctime;
> vap->va_ctime.tv_nsec = ip->i_din1->di_ctimensec;
> - vap->va_birthtime.tv_sec = 0;
> + vap->va_birthtime.tv_sec = -1;
> vap->va_birthtime.tv_nsec = 0;
> vap->va_bytes = dbtob((u_quad_t)ip->i_din1->di_blocks);
> } else {
Can just delete birthtime references here. Unless I've missed a bzero.
> Index: sys/nfsclient/nfs_subs.c
> ===================================================================
> --- sys/nfsclient/nfs_subs.c (revision 180729)
> +++ sys/nfsclient/nfs_subs.c (working copy)
> @@ -628,6 +628,8 @@ nfs_loadattrcache(struct vnode **vpp, st
> vap->va_rdev = rdev;
> mtime_save = vap->va_mtime;
> vap->va_mtime = mtime;
> + vap->va_birthtime.tv_sec = -1;
> + vap->va_birthtime.tv_nsec = 0;
> vap->va_fsid = vp->v_mount->mnt_stat.f_fsid.val[0];
> if (v3) {
> vap->va_nlink = fxdr_unsigned(u_short, fp->fa_nlink);
Need to remove the zeroing and check other fields before defaulting
birthtime here.
> Index: sys/fs/pseudofs/pseudofs_vnops.c
> ===================================================================
> --- sys/fs/pseudofs/pseudofs_vnops.c (revision 180729)
> +++ sys/fs/pseudofs/pseudofs_vnops.c (working copy)
> @@ -200,7 +200,7 @@ pfs_getattr(struct vop_getattr_args *va)
> vap->va_fsid = vn->v_mount->mnt_stat.f_fsid.val[0];
> vap->va_nlink = 1;
> nanotime(&vap->va_ctime);
> - vap->va_atime = vap->va_mtime = vap->va_ctime;
> + vap->va_atime = vap->va_mtime = vap->va_birthtime = vap->va_ctime;
>
> switch (pn->pn_type) {
> case pfstype_procdir:
I don't understand why it doesn't have _any_ persistent storage for times.
> Index: sys/fs/portalfs/portal_vnops.c
> ===================================================================
> --- sys/fs/portalfs/portal_vnops.c (revision 180729)
> +++ sys/fs/portalfs/portal_vnops.c (working copy)
> @@ -462,6 +462,8 @@ portal_getattr(ap)
> nanotime(&vap->va_atime);
> vap->va_mtime = vap->va_atime;
> vap->va_ctime = vap->va_mtime;
> + vap->va_birthtime.tv_sec = -1;
> + vap->va_birthtime.tv_nsec = 0;
> vap->va_gen = 0;
> vap->va_flags = 0;
> vap->va_rdev = 0;
This uses both bzero() and vattr_null(). Oops, I only grepped for use
of VATTR_NULL() when I looked for bogus initializations above.
VATTR_NULL() is the public interface and vattr_null() is an implementation
detail. Add the following file systems to the list of file systems with
bogusly initialized vattr's in VOP_GETATTR():
devfs, portalfs
These both misuse bzero() and vattr_null(). There are no other misuses
of vattr_null().
> Index: sys/fs/devfs/devfs_vnops.c
> ===================================================================
> --- sys/fs/devfs/devfs_vnops.c (revision 180729)
> +++ sys/fs/devfs/devfs_vnops.c (working copy)
> @@ -543,6 +543,8 @@ devfs_getattr(struct vop_getattr_args *a
>
> vap->va_rdev = cdev2priv(dev)->cdp_inode;
> }
> + vap->va_birthtime.tv_sec = -1;
> + vap->va_birthtime.tv_nsec = 0;
> vap->va_gen = 0;
> vap->va_flags = 0;
> vap->va_nlink = de->de_links;
See above.
> Index: sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c
> ===================================================================
> --- sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c (revision 180729)
> +++ sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c (working copy)
> @@ -263,6 +263,8 @@ _xfs_getattr(
> vap->va_atime = va.va_atime;
> vap->va_mtime = va.va_mtime;
> vap->va_ctime = va.va_ctime;
> + vap->va_birthtime.tv_sec = -1;
> + vap->va_birthtime.tv_nsec = 0;
> vap->va_gen = va.va_gen;
> vap->va_rdev = va.va_rdev;
> vap->va_bytes = (va.va_nblocks << BBSHIFT);
See above (need to do somethign about the VATTR_NULL()).
> %%%
Grepping for va_.*flags in only sys/fs/ shows the following problems
in VOP_SETATTR():
- coda: sets va_vaflags in a macro but never uses va_vaflags (needed
for layering?)
- ntfs: sets va_flags to ip->i_flag -- nonsense -- i_flag is an internal
flag that has nothing to do with va_flags
- nwfs: sets va_vaflags in nwfs_attr_cacheenter(), but I think nothing uses
this setting.
- smbfs: sets va_vaflags in smbfs_attrcachelookup() ...
- tmpfs: sets va_vaflags and also va_spare.
and the following non-problems:
- all except msdosfs set va_flags to 0, so defaulting va_flags to 0 and
deleting most settings of it would work well.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080724000618.Q16961>