Skip site navigation (1)Skip section navigation (2)
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>