|
»
»
birthtime initialization
|
View:
New views
13 Messages
—
Rating Filter:
Alert me
|
 |
|
|
|
Re: birthtime initialization
by Poul-Henning Kamp
::
Rate this Message:
In message <20080722075718.GA1881@...>, Jaakko
Heinonen writes: >On 2008-06-02, Bruce Evans wrote: >[about patch for ext2fs in PR kern/122047] >> % + vap->va_birthtime.tv_sec = 0; >> % + vap->va_birthtime.tv_nsec = 0; >> >> This is unrelated and should be handled centrally. Almost all file >> systems get this wrong. Most fail to set va_birthtime, so stat() >> returns kernel stack garbage for st_birthtime. ffs1 does the same >> as the above. msdosfs does the above correctly, by setting tv_sec to >> (time_t)-1 in unsupported cases. > >How about this patch? Looks like something Kirk forgot to me. We want to macroize the NOVAL for timespec instead of spreading -1 casts all over. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 phk@... | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Bruce Evans-4
::
Rate this Message:
On Tue, 22 Jul 2008, Jaakko Heinonen wrote:
> On 2008-06-02, Bruce Evans wrote: > [about patch for ext2fs in PR kern/122047] >> % + vap->va_birthtime.tv_sec = 0; >> % + vap->va_birthtime.tv_nsec = 0; >> >> This is unrelated and should be handled centrally. Almost all file >> systems get this wrong. Most fail to set va_birthtime, so stat() >> returns kernel stack garbage for st_birthtime. ffs1 does the same >> as the above. msdosfs does the above correctly, by setting tv_sec to >> (time_t)-1 in unsupported cases. > > How about this patch? > > %%% > Index: sys/kern/vfs_vnops.c > =================================================================== > --- sys/kern/vfs_vnops.c (revision 180588) > +++ sys/kern/vfs_vnops.c (working copy) > @@ -703,6 +703,9 @@ vn_stat(vp, sb, active_cred, file_cred, > #endif > > vap = &vattr; > + /* Not all file systems initialize birthtime. */ > + VATTR_NULL(vap); > + > error = VOP_GETATTR(vp, vap, active_cred, td); > if (error) > return (error); 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. Similarly, if there are any fields that aren't supported by most file systems, then they should be searched for and defaulted like va_birthtime instead of requiring indivual file systems to invent a default value for them. > 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. > The patch adds VATTR_NULL() call to vn_stat() to initialize the vattr > structure before VOP_GETATTR() call. VATTR_NULL() initializes > va_birthtime.tv_sec and va_birthtime.tv_nsec to -1 (VNOVAL). I also > changed UFS1 and msdosfs to use consistent values. NFS needs explicit > initialization because otherwise values would be set to 0 due to memory > obtained with M_ZERO flag. VNOVAL = -1 only accidentally gives the correct value for va_birthtime.tv_sec. It gives a wrong value for va_birthtime.tv_nsec. It is better to set va_birthtime.tv_sec explicitly to -1. This -1 is only accidentantally equal to VNOVAL. Fortunately, this accident doesn't prevent VOP_GETATTR() from setting va_birthtime, since VNOVAL is only magic for VOP_SETATTR(). phk replied (but didn't quote enough, so I merged this manually): >> Looks like something Kirk forgot to me. >> We want to macroize the NOVAL for timespec instead of spreading >> -1 casts all over. This isn't a problem for the "GET" interface since VNOVAL doesn't apply to it. Also, the casts of -1 aren't really needed. ufs_settattr() doesn't have them for time_t's, and vattr_null() doesn't have them for anything. The correctness of this depends on the type of time_t (and the other va field times). In userland we're supposed to cast -1 to time_t for error detection in mktime() etc. In userland, time_t can be any arithmetic time so it is possible for (time_t)-1 != -1. Even there, I think there is only a problem if time_t is an unsigned intergral type shorter than int. Compilers may warn about other cases. ufs_settatr() has the casts for va_bytes (bogus cast of va_bytes to int, which breaks its value), va_uid, va_gid and va_mode. For va_mode, there is a problem -- the same one as in my example for time_t above -- va_mode is u_short so it cannot equal -1 (after the default promotions) except on exotic systems. For va_uid and va_gid, the casts were needed 15 years ago when uid_t and gid_t were 16 bits. I can't see any problem with omitting the cast for va_bytes -- va_bytes is u_quad_t, which is certainly at least as large as int, so it can equal VNOVAL = -1 after the default promotions though it cannot represent any negative value (now C's conversion rules requires (uquad_t)-1 == -1, and it would be a compiler bug to warn about expressions that depend on these rules). In vattr_null(), the assignments go the other way and VNOVAL = -1 always gets converted to the intended value (which is not always -1). C's conversion rules are depended on even more here to do something reasonable with (foo_t)-1. I wouldn't like VNOVAL being replaced by VNOTIMESPECVAL, VNOUIDVAL, ... etc. Recently I noticed a commit that replaced (struct foo *)0 by NULL together with less contentions replacements of plain 0 by NULL. Old code that tries to be careful uses (struct foo *)0 (or a macro NULLFOO for this) too much. Now that NULL is Standard we can just use plain NULL. Similarly for plain VNOVAL except in a few cases where -1 doesn't get converted right. Bruce _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
|
|
|
Re: birthtime initialization
by Bruce Evans-4
::
Rate this Message:
On Tue, 22 Jul 2008, Pedro Giffuni wrote:
> Tim has some patches I made to add support for birthtime in libarchive (only in extended pax format) as a LIBARCHIVE.creationtime attribute. > > Since birthtime is set by modifying mtime twice with utimes(2), the only criteria I used to determine if birthtime should be stored is if it was less than mtime. I hope something can be done to make that behavior consistent with UFS2 in all other filesystems. Can't it check for st_birthtime.tv_sec being != 0 or -1? The erroneous default of 0 might interact badly with file systems written by buggy versions of tar that set times to 0. Bruce _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Pedro Giffuni
::
Rate this Message:
--- Mar 22/7/08, Bruce Evans <brde@...> ha scritto:
... > > > Tim has some patches I made to add support for > birthtime in libarchive (only in extended pax format) as a > LIBARCHIVE.creationtime attribute. > > > > Since birthtime is set by modifying mtime twice with > utimes(2), the only criteria I used to determine if > birthtime should be stored is if it was less than mtime. I > hope something can be done to make that behavior consistent > with UFS2 in all other filesystems. > > Can't it check for st_birthtime.tv_sec being != 0 or > -1? OK, I can do that, in fact I had it like that originally but then strictly speaking those values are valid and I had to check for birthtime==mtime anyways. Admittedly no BSD system was available before Jan 1st 1970 so I will modify the check to avoid those times. Pedro. Posta, news, sport, oroscopo: tutto in una sola pagina. Crea l'home page che piace a te! www.yahoo.it/latuapagina _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Jaakko Heinonen-2
::
Rate this Message:
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 > > 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)) 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 > I wouldn't like VNOVAL being replaced by VNOTIMESPECVAL, VNOUIDVAL, > ... etc. I agree with this. 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. For pseudofs I set birthtime to current time. %%% 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. + */ + 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 { 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); 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: 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; 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; 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); %%% -- Jaakko _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Bruce Evans-4
::
Rate this Message:
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 _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Jaakko Heinonen-2
::
Rate this Message:
On 2008-07-24, Bruce Evans wrote:
> First, the fields shouldn't be initialized using VATTR_NULL() in > VOP_GETATTR(). > Second, VNOVAL is an extremly bogus default value. Except for va_fsid because there's this check in vn_stat(): if (vap->va_fsid != VNOVAL) sb->st_dev = vap->va_fsid; else sb->st_dev = vp->v_mount->mnt_stat.f_fsid.val[0]; What do you think that is a proper default value for va_rdev? Some file systems set it to 0 and some to VNOVAL. > 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. Unfortunately moving initializations to vn_stat() breaks things. For example vm_mmap_vnode() uses VOP_GETATTR() to determine which file flags are set. Thus moving va_flags initialization to vn_stat() breaks mmap. In theory this could be a potential problem for birthtime too. > > 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) > > I don't want the case (3). Otherwise good. Thank you for your valuable comments. I will try to update the patch. -- Jaakko _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Bruce Evans-4
::
Rate this Message:
On Fri, 25 Jul 2008, Jaakko Heinonen wrote:
> On 2008-07-24, Bruce Evans wrote: >> First, the fields shouldn't be initialized using VATTR_NULL() in >> VOP_GETATTR(). > >> Second, VNOVAL is an extremly bogus default value. > > Except for va_fsid because there's this check in vn_stat(): > > if (vap->va_fsid != VNOVAL) > sb->st_dev = vap->va_fsid; > else > sb->st_dev = vp->v_mount->mnt_stat.f_fsid.val[0]; Hmm, this is remarkably broken too. In VOP_GETATTR() for file systems under sys/fs: - the following file systems set va_fsid to dev2udev(<the mount point>) (and thus defeat the better default above): cd9660, hpfs, msdosfs, ntfs, udf, unionfs - the following file systems don't seem to set va_fsid (and thus set st_dev to stack garbage) - the following file systems set va_fsid to VNOVAL via VATTR_NULL(): fdescfs - the following file systems set va_fsid to VNOVAL via vattr_null(): devfs, portalfs - the following file systems set va_fsid to VNOVAL via obscure means: coda (?) - the following file systems set va_fsid to mnt_stat.f_fsid.val[0] directly: nullfs, nwfs (?), pseudofs, smbfs (?), tmpfs > What do you think that is a proper default value for va_rdev? Some file > systems set it to 0 and some to VNOVAL. Either NODEV or VNOVAL explicitly translated late to NODEV. NODEV is (dev_t)(-1) (bug: this has parentheses in all the wrong places -- it should be ((dev_t)-1), so VNOVAL = -1 assigned to va_rdev of type dev_t equals NODEV and the identity translation works. >> 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. > > Unfortunately moving initializations to vn_stat() breaks things. For > example vm_mmap_vnode() uses VOP_GETATTR() to determine which file flags > are set. Thus moving va_flags initialization to vn_stat() breaks > mmap. Oops. > In theory this could be a potential problem for birthtime too. It's a bit dangerous, but most callers to VOP_GETATTR() except vn_stat() only want a couple of fields, and hopefully none want new fields. Maybe the public interface should be vop_getattr() which sets defaults and calls VOP_GETATTR(). Does this work with layering? There is negative point to inlining most VOPs, and for VOP_GETATTR(), no one cares about the much higher overhead of setting all fields in it when only a couple are wanted. Bruce _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Jaakko Heinonen-2
::
Rate this Message:
Hi, I further updated the patch. On 2008-07-25, Bruce Evans wrote: > On Fri, 25 Jul 2008, Jaakko Heinonen wrote: > > On 2008-07-24, Bruce Evans wrote: > >> First, the fields shouldn't be initialized using VATTR_NULL() in > >> VOP_GETATTR(). I removed VATTR_NULL() from xfs, mqueuefs, pseudofs, tmpfs, devfs fdescfs and vattr_null() from devfs and portalfs. I also removed zeroing from nfs. > > What do you think that is a proper default value for va_rdev? Some file > > systems set it to 0 and some to VNOVAL. > > Either NODEV or VNOVAL explicitly translated late to NODEV. I changed following file systems to initialize va_rdev to NODEV instead of 0 or VNOVAL (when appropriate): mqueuefs, tmpfs, portalfs, smbfs, ntfs, fdescfs and msdosfs. Also in vn_stat() va_rdev is now initialized to VNOVAL and explicitly translated to NODEV after the VOP_GETATTR() call. I have tested the patch with these file systems: UFS2, UFS1, ext2fs, ntfs, cd9660, udf, procfs, devfs, xfs, reiserfs, fdescfs, msdosfs, mqueuefs, nfs, smbfs, portalfs. %%% Index: sys/nfsclient/nfs_vnops.c =================================================================== --- sys/nfsclient/nfs_vnops.c (revision 182592) +++ sys/nfsclient/nfs_vnops.c (working copy) @@ -631,6 +631,8 @@ nfs_getattr(struct vop_getattr_args *ap) struct vnode *vp = ap->a_vp; struct nfsnode *np = VTONFS(vp); struct thread *td = curthread; + struct vattr *vap = ap->a_vap; + struct vattr vattr; caddr_t bpos, dpos; int error = 0; struct mbuf *mreq, *mrep, *md, *mb; @@ -646,12 +648,12 @@ nfs_getattr(struct vop_getattr_args *ap) /* * First look in the cache. */ - if (nfs_getattrcache(vp, ap->a_vap) == 0) + if (nfs_getattrcache(vp, &vattr) == 0) goto nfsmout; if (v3 && nfsaccess_cache_timeout > 0) { nfsstats.accesscache_misses++; nfs3_access_otw(vp, NFSV3ACCESS_ALL, td, ap->a_cred); - if (nfs_getattrcache(vp, ap->a_vap) == 0) + if (nfs_getattrcache(vp, &vattr) == 0) goto nfsmout; } nfsstats.rpccnt[NFSPROC_GETATTR]++; @@ -661,10 +663,27 @@ nfs_getattr(struct vop_getattr_args *ap) nfsm_fhtom(vp, v3); nfsm_request(vp, NFSPROC_GETATTR, td, ap->a_cred); if (!error) { - nfsm_loadattr(vp, ap->a_vap); + nfsm_loadattr(vp, &vattr); } m_freem(mrep); nfsmout: + vap->va_type = vattr.va_type; + vap->va_mode = vattr.va_mode; + vap->va_nlink = vattr.va_nlink; + vap->va_uid = vattr.va_uid; + vap->va_gid = vattr.va_gid; + vap->va_fsid = vattr.va_fsid; + vap->va_fileid = vattr.va_fileid; + vap->va_size = vattr.va_size; + vap->va_blocksize = vattr.va_blocksize; + vap->va_atime = vattr.va_atime; + vap->va_mtime = vattr.va_mtime; + vap->va_ctime = vattr.va_ctime; + vap->va_gen = vattr.va_gen; + vap->va_flags = vattr.va_flags; + vap->va_rdev = vattr.va_rdev; + vap->va_bytes = vattr.va_bytes; + return (error); } Index: sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c =================================================================== --- sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c (revision 182592) +++ sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c (working copy) @@ -240,7 +240,6 @@ _xfs_getattr( /* extract the xfs vnode from the private data */ //xfs_vnode_t *xvp = (xfs_vnode_t *)vp->v_data; - VATTR_NULL(vap); memset(&va,0,sizeof(xfs_vattr_t)); va.va_mask = XFS_AT_STAT|XFS_AT_GENCOUNT|XFS_AT_XFLAGS; @@ -273,15 +272,9 @@ _xfs_getattr( /* * 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 return (0); } Index: sys/ufs/ufs/ufs_vnops.c =================================================================== --- sys/ufs/ufs/ufs_vnops.c (revision 182592) +++ sys/ufs/ufs/ufs_vnops.c (working copy) @@ -434,8 +434,6 @@ 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_nsec = 0; vap->va_bytes = dbtob((u_quad_t)ip->i_din1->di_blocks); } else { vap->va_rdev = ip->i_din2->di_rdev; Index: sys/kern/uipc_mqueue.c =================================================================== --- sys/kern/uipc_mqueue.c (revision 182592) +++ sys/kern/uipc_mqueue.c (working copy) @@ -1128,7 +1128,6 @@ mqfs_getattr(struct vop_getattr_args *ap struct vattr *vap = ap->a_vap; int error = 0; - VATTR_NULL(vap); vap->va_type = vp->v_type; vap->va_mode = pn->mn_mode; vap->va_nlink = 1; @@ -1145,7 +1144,7 @@ mqfs_getattr(struct vop_getattr_args *ap vap->va_birthtime = pn->mn_birth; vap->va_gen = 0; vap->va_flags = 0; - vap->va_rdev = 0; + vap->va_rdev = NODEV; vap->va_bytes = 0; vap->va_filerev = 0; vap->va_vaflags = 0; Index: sys/kern/vfs_vnops.c =================================================================== --- sys/kern/vfs_vnops.c (revision 182592) +++ sys/kern/vfs_vnops.c (working copy) @@ -703,6 +703,17 @@ vn_stat(vp, sb, active_cred, file_cred, #endif vap = &vattr; + + /* + * Initialize defaults for new and 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; + vap->va_fsid = VNOVAL; + vap->va_rdev = VNOVAL; + error = VOP_GETATTR(vp, vap, active_cred); if (error) return (error); @@ -750,7 +761,10 @@ vn_stat(vp, sb, active_cred, file_cred, sb->st_nlink = vap->va_nlink; sb->st_uid = vap->va_uid; sb->st_gid = vap->va_gid; - sb->st_rdev = vap->va_rdev; + if (vap->va_rdev == VNOVAL) + sb->st_rdev = NODEV; + else + sb->st_rdev = vap->va_rdev; if (vap->va_size > OFF_MAX) return (EOVERFLOW); sb->st_size = vap->va_size; Index: sys/fs/pseudofs/pseudofs_vnops.c =================================================================== --- sys/fs/pseudofs/pseudofs_vnops.c (revision 182592) +++ sys/fs/pseudofs/pseudofs_vnops.c (working copy) @@ -191,7 +191,6 @@ pfs_getattr(struct vop_getattr_args *va) if (!pfs_visible(curthread, pn, pvd->pvd_pid, &proc)) PFS_RETURN (ENOENT); - VATTR_NULL(vap); vap->va_type = vn->v_type; vap->va_fileid = pn_fileno(pn, pvd->pvd_pid); vap->va_flags = 0; Index: sys/fs/tmpfs/tmpfs_vnops.c =================================================================== --- sys/fs/tmpfs/tmpfs_vnops.c (revision 182592) +++ sys/fs/tmpfs/tmpfs_vnops.c (working copy) @@ -351,8 +351,6 @@ tmpfs_getattr(struct vop_getattr_args *v node = VP_TO_TMPFS_NODE(vp); - VATTR_NULL(vap); - tmpfs_update(vp); vap->va_type = vp->v_type; @@ -371,11 +369,9 @@ tmpfs_getattr(struct vop_getattr_args *v vap->va_gen = node->tn_gen; vap->va_flags = node->tn_flags; vap->va_rdev = (vp->v_type == VBLK || vp->v_type == VCHR) ? - node->tn_rdev : VNOVAL; + node->tn_rdev : NODEV; vap->va_bytes = round_page(node->tn_size); - vap->va_filerev = VNOVAL; - vap->va_vaflags = 0; - vap->va_spare = VNOVAL; /* XXX */ + vap->va_filerev = 0; return 0; } Index: sys/fs/portalfs/portal_vnops.c =================================================================== --- sys/fs/portalfs/portal_vnops.c (revision 182592) +++ sys/fs/portalfs/portal_vnops.c (working copy) @@ -452,8 +452,6 @@ portal_getattr(ap) struct vnode *vp = ap->a_vp; struct vattr *vap = ap->a_vap; - bzero(vap, sizeof(*vap)); - vattr_null(vap); vap->va_uid = 0; vap->va_gid = 0; vap->va_size = DEV_BSIZE; @@ -463,7 +461,7 @@ portal_getattr(ap) vap->va_ctime = vap->va_mtime; vap->va_gen = 0; vap->va_flags = 0; - vap->va_rdev = 0; + vap->va_rdev = NODEV; /* vap->va_qbytes = 0; */ vap->va_bytes = 0; /* vap->va_qsize = 0; */ Index: sys/fs/devfs/devfs_vnops.c =================================================================== --- sys/fs/devfs/devfs_vnops.c (revision 182592) +++ sys/fs/devfs/devfs_vnops.c (working copy) @@ -499,8 +499,6 @@ devfs_getattr(struct vop_getattr_args *a KASSERT(de != NULL, ("Null dir dirent in devfs_getattr vp=%p", vp)); } - bzero((caddr_t) vap, sizeof(*vap)); - vattr_null(vap); vap->va_uid = de->de_uid; vap->va_gid = de->de_gid; vap->va_mode = de->de_mode; Index: sys/fs/smbfs/smbfs_node.c =================================================================== --- sys/fs/smbfs/smbfs_node.c (revision 182592) +++ sys/fs/smbfs/smbfs_node.c (working copy) @@ -438,7 +438,7 @@ smbfs_attr_cachelookup(struct vnode *vp, va->va_atime = va->va_ctime = va->va_mtime; /* time file changed */ va->va_gen = VNOVAL; /* generation number of file */ va->va_flags = 0; /* flags defined for file */ - va->va_rdev = VNOVAL; /* device the special file represents */ + va->va_rdev = NODEV; /* device the special file represents */ va->va_bytes = va->va_size; /* bytes of disk space held by file */ va->va_filerev = 0; /* file modification number */ va->va_vaflags = 0; /* operations flags */ Index: sys/fs/ntfs/ntfs_vnops.c =================================================================== --- sys/fs/ntfs/ntfs_vnops.c (revision 182592) +++ sys/fs/ntfs/ntfs_vnops.c (working copy) @@ -191,7 +191,7 @@ ntfs_getattr(ap) vap->va_nlink = (ip->i_nlink || ip->i_flag & IN_LOADED ? ip->i_nlink : 1); vap->va_uid = ip->i_mp->ntm_uid; vap->va_gid = ip->i_mp->ntm_gid; - vap->va_rdev = 0; /* XXX UNODEV ? */ + vap->va_rdev = NODEV; vap->va_size = fp->f_size; vap->va_bytes = fp->f_allocated; vap->va_atime = ntfs_nttimetounix(fp->f_times.t_access); Index: sys/fs/fdescfs/fdesc_vnops.c =================================================================== --- sys/fs/fdescfs/fdesc_vnops.c (revision 182592) +++ sys/fs/fdescfs/fdesc_vnops.c (working copy) @@ -391,8 +391,6 @@ fdesc_getattr(ap) switch (VTOFDESC(vp)->fd_type) { case Froot: - VATTR_NULL(vap); - vap->va_mode = S_IRUSR|S_IXUSR|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH; vap->va_type = VDIR; vap->va_nlink = 2; @@ -407,7 +405,7 @@ fdesc_getattr(ap) vap->va_ctime = vap->va_mtime; vap->va_gen = 0; vap->va_flags = 0; - vap->va_rdev = 0; + vap->va_rdev = NODEV; vap->va_bytes = 0; break; @@ -421,7 +419,6 @@ fdesc_getattr(ap) error = fo_stat(fp, &stb, td->td_ucred, td); fdrop(fp, td); if (error == 0) { - VATTR_NULL(vap); vap->va_type = IFTOVT(stb.st_mode); vap->va_mode = stb.st_mode; #define FDRX (VREAD|VEXEC) Index: sys/fs/msdosfs/msdosfs_vnops.c =================================================================== --- sys/fs/msdosfs/msdosfs_vnops.c (revision 182592) +++ sys/fs/msdosfs/msdosfs_vnops.c (working copy) @@ -334,7 +334,7 @@ msdosfs_getattr(ap) vap->va_uid = pmp->pm_uid; vap->va_gid = pmp->pm_gid; vap->va_nlink = 1; - vap->va_rdev = 0; + vap->va_rdev = NODEV; vap->va_size = dep->de_FileSize; fattime2timespec(dep->de_MDate, dep->de_MTime, 0, 0, &vap->va_mtime); vap->va_ctime = vap->va_mtime; %%% -- Jaakko _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Bruce Evans-4
::
Rate this Message:
On Wed, 3 Sep 2008, Jaakko Heinonen wrote:
> I further updated the patch. > > On 2008-07-25, Bruce Evans wrote: >> On Fri, 25 Jul 2008, Jaakko Heinonen wrote: >>> On 2008-07-24, Bruce Evans wrote: >>>> First, the fields shouldn't be initialized using VATTR_NULL() in >>>> VOP_GETATTR(). > > I removed VATTR_NULL() from xfs, mqueuefs, pseudofs, tmpfs, devfs > fdescfs and vattr_null() from devfs and portalfs. I also removed zeroing > from nfs. > >>> What do you think that is a proper default value for va_rdev? Some file >>> systems set it to 0 and some to VNOVAL. >> >> Either NODEV or VNOVAL explicitly translated late to NODEV. > > I changed following file systems to initialize va_rdev to NODEV instead > of 0 or VNOVAL (when appropriate): mqueuefs, tmpfs, portalfs, smbfs, > ntfs, fdescfs and msdosfs. Also in vn_stat() va_rdev is now initialized > to VNOVAL and explicitly translated to NODEV after the VOP_GETATTR() > call. > > I have tested the patch with these file systems: UFS2, UFS1, ext2fs, > ntfs, cd9660, udf, procfs, devfs, xfs, reiserfs, fdescfs, msdosfs, > mqueuefs, nfs, smbfs, portalfs. I like this version, but didn't check many details. > Index: sys/kern/vfs_vnops.c > =================================================================== > --- sys/kern/vfs_vnops.c (revision 182592) > +++ sys/kern/vfs_vnops.c (working copy) > @@ -703,6 +703,17 @@ vn_stat(vp, sb, active_cred, file_cred, > #endif > > vap = &vattr; > + > + /* > + * Initialize defaults for new and 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; > + vap->va_fsid = VNOVAL; > + vap->va_rdev = VNOVAL; Shouldn't this be NODEV? NODEV is ((dev_t)-1) and VNOVAL is plain (-1), so their values are identical after assignment to va_rdev... > + > error = VOP_GETATTR(vp, vap, active_cred); > if (error) > return (error); > @@ -750,7 +761,10 @@ vn_stat(vp, sb, active_cred, file_cred, > sb->st_nlink = vap->va_nlink; > sb->st_uid = vap->va_uid; > sb->st_gid = vap->va_gid; > - sb->st_rdev = vap->va_rdev; > + if (vap->va_rdev == VNOVAL) > + sb->st_rdev = NODEV; > + else > + sb->st_rdev = vap->va_rdev; ... this change seems to have no effect on machines with 32-bit 2's complement ints and to be wrong on other machines. Assignment of VNOVAL to va_rdev changes its value from -1 to 0xFFFFFFFFU, so it can only compare equal to VNOVAL if int has the same size as dev_t or there is stronger magic. When the comparision is fixed to compare with the assigned value (dev_t)VNOVAL == (dev_t)(-1) == NODEV, it is clear that the change has no effect. Bruce _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
|
|
Re: birthtime initialization
by Jaakko Heinonen-2
::
Rate this Message:
On 2008-09-09, Bruce Evans wrote:
> > @@ -703,6 +703,17 @@ vn_stat(vp, sb, active_cred, file_cred, > > + vap->va_rdev = VNOVAL; > > Shouldn't this be NODEV? NODEV is ((dev_t)-1) and VNOVAL is plain (-1), > so their values are identical after assignment to va_rdev... Yes, it's probably better to use NODEV here... > > - sb->st_rdev = vap->va_rdev; > > + if (vap->va_rdev == VNOVAL) > > + sb->st_rdev = NODEV; > > + else > > + sb->st_rdev = vap->va_rdev; ...and leave out this change. > ... this change seems to have no effect on machines with 32-bit 2's complement > ints and to be wrong on other machines. Assignment of VNOVAL to va_rdev > changes its value from -1 to 0xFFFFFFFFU, so it can only compare equal > to VNOVAL if int has the same size as dev_t or there is stronger magic. > When the comparision is fixed to compare with the assigned value > (dev_t)VNOVAL == (dev_t)(-1) == NODEV, it is clear that the change has > no effect. Some file systems seem to have vap->va_rdev != VNOVAL comparisons in VOP_SETATTR(). Thanks. -- Jaakko _______________________________________________ freebsd-fs@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-fs To unsubscribe, send any mail to "freebsd-fs-unsubscribe@..." |
| Free embeddable forum powered by Nabble | Forum Help |
