From owner-freebsd-fs@FreeBSD.ORG Tue Sep 9 18:46:13 2008 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E08301065679 for ; Tue, 9 Sep 2008 18:46:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx08.syd.optusnet.com.au (fallbackmx08.syd.optusnet.com.au [211.29.132.10]) by mx1.freebsd.org (Postfix) with ESMTP id 769638FC22 for ; Tue, 9 Sep 2008 18:46:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by fallbackmx08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m89BAMae015106 for ; Tue, 9 Sep 2008 21:10:22 +1000 Received: from besplex.bde.org (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m89BAHw1015432 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 9 Sep 2008 21:10:18 +1000 Date: Tue, 9 Sep 2008 21:10:17 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jaakko Heinonen In-Reply-To: <20080903160736.GA12587@a91-153-122-179.elisa-laajakaista.fi> Message-ID: <20080909204354.Q3089@besplex.bde.org> 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> <20080724000618.Q16961@besplex.bde.org> <20080725072314.GA807@a91-153-120-204.elisa-laajakaista.fi> <20080725192315.D27178@delplex.bde.org> <20080903160736.GA12587@a91-153-122-179.elisa-laajakaista.fi> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-fs@freebsd.org Subject: Re: birthtime initialization X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Sep 2008 18:46:14 -0000 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