From owner-freebsd-bugs@FreeBSD.ORG Tue Dec 5 01:50:11 2006 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4864016A597 for ; Tue, 5 Dec 2006 01:50:11 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [69.147.83.40]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5C52D43CA2 for ; Tue, 5 Dec 2006 01:49:34 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id kB51o9f7069496 for ; Tue, 5 Dec 2006 01:50:09 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.4/8.13.4/Submit) id kB51o9v6069493; Tue, 5 Dec 2006 01:50:09 GMT (envelope-from gnats) Date: Tue, 5 Dec 2006 01:50:09 GMT Message-Id: <200612050150.kB51o9v6069493@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Bruce Evans Cc: Subject: Re: kern/106255: [msdosfs] : correct setting of archive flag X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Bruce Evans List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Dec 2006 01:50:11 -0000 The following reply was made to PR kern/106255; it has been noted by GNATS. From: Bruce Evans To: Rene Ladan Cc: freebsd-gnats-submit@FreeBSD.org, freebsd-bugs@FreeBSD.org Subject: Re: kern/106255: [msdosfs] : correct setting of archive flag Date: Tue, 5 Dec 2006 12:44:36 +1100 (EST) On Mon, 4 Dec 2006, Rene Ladan wrote: > I've attached a new patch which > * fixes reporting of the flag (as in the previous patch) > * sets the archive flag in DETIMES() when the file is created > * cleans up the logic of not supporting setting the archive flag on > directories (chunk 3) > * does not set the flag when (vap->va_atime.tv_sec != VNOVAL) or > (vap->va_mode != VNOVAL) in msdosfs_setattr() The first fix is the one that most needs some directory logic (see a previous reply). Wasn't the archive flag already set in SF_ARCHIVED? I will enclose my old patches for DETIMES() at the end. I had noticed that the archive flag shouldn't be set for null changes to the times. Also, checking for null changes to times is a good optimization in general since it avoids some disk writes. ffs should do it too. The disk writes are delayed so many of them get coalesced, but even 1 is expensive. The file time granularity of 1-2 seconds is much longer than when CPUs were thousands of times slower. Of course, this optimization becomes null for ffs if you use the vfs.timestamp_precision sysctl to set a very small file time granularity. > I think that only userland tools should send a 'clear request', as the > flag only needs to be cleared when the file is backed up. The kernel > cannot know when a file has been backed up. Except it should be a set request of SF_ARCHIVED. % --- msdosfs_vnops.c.orig Sun Dec 3 20:45:24 2006 % +++ msdosfs_vnops.c Mon Dec 4 12:43:37 2006 % @@ -354,7 +354,7 @@ % vap->va_birthtime.tv_nsec = 0; % } % vap->va_flags = 0; % - if ((dep->de_Attributes & ATTR_ARCHIVE) == 0) % + if (dep->de_Attributes & ATTR_ARCHIVE) % vap->va_flags |= SF_ARCHIVED; % vap->va_gen = 0; % vap->va_blocksize = pmp->pm_bpcluster; Needs to do nothing for directories and not change for non-directories. % @@ -431,12 +431,13 @@ % if (error) % return (error); % } % - if (vap->va_flags & ~SF_ARCHIVED) % - return EOPNOTSUPP; Removing this is just a bug. It is needed to reject attempts to set flags other than SF_ARCHIVED. All such flags are unsupported. % if (vap->va_flags & SF_ARCHIVED) % dep->de_Attributes &= ~ATTR_ARCHIVE; I think the error return for the directory case belongs here, although this is inconsistent with the reversal of the flags. The archive bit just doesn't exist for directories so attempts to set SF_ARCHIVED == clear ATTR_ARCHIVE are nonsense. More importantly, the raw bit with the same value as ATTR_ARCHIVE might have a different meaning for directories, so it shouldn't be cleared blindly. % - else if (!(dep->de_Attributes & ATTR_DIRECTORY)) % - dep->de_Attributes |= ATTR_ARCHIVE; % + else % + if (dep->de_Attributes & ATTR_DIRECTORY) % + return EOPNOTSUPP; % + else % + dep->de_Attributes |= ATTR_ARCHIVE; This has some indentation errors. The multiple reversals make this error handling very confusing and wrong, especially for directories: - ATTR_ARCHIVE should be clear initially. Reversing the reversal in setattr gives SF_ARCHIVED clear for the wrong reason. - now if we cp -p the direcctory, then we don't try to set SF_ARCHIVED so we reach the above. SF_ARCHIVED clear is interpreted as a request to set ATTR_ARCHIVE and rejected. Untested fixes: if (dep->de_Attributes & ATTR_DIRECTORY) { if (vap->va_flags & SF_ARCHIVED) return (EOPNOTSUPP); } else { if (vap->va_flags & SF_ARCHIVED) { dep->de_Attributes &= ~ATTR_ARCHIVE; else dep->de_Attributes |= ATTR_ARCHIVE; } % dep->de_flag |= DE_MODIFIED; % } % % @@ -506,8 +507,9 @@ % dep->de_flag &= ~DE_UPDATE; % timespec2fattime(&vap->va_mtime, 0, % &dep->de_MDate, &dep->de_MTime, NULL); % + dep->de_Attributes |= ATTR_ARCHIVE; % + /* only set archive flag when file has changed */ Various style bugs in the comment. % } % - dep->de_Attributes |= ATTR_ARCHIVE; % dep->de_flag |= DE_MODIFIED; Now it doesn't set the archive flag when the atime changes but the mtime doesn't change. Is this intentional? This change has no effect since all callers set both times and we don't check for null changes to the times. Checking for null changes here is not worth it as an optimization, but probably right for correctness (so as not to set the archive flag for null changes). We don't handle null changes for setting attributes in general. Msdosfs doesn't have many attributes so the only other problem cases are: - null changes to the archive bit itself. Not detecting these is only a pessimization. It doesn't mess up the archive bit itself. - null changes to the mode. These force the archive bit on (for directories only). I once made some minor improvements for null changes to attributes in ffs. IIRC, they were for chown(), and the result is the following very delicate handling of null changes: - permission is required for even null changes of the uid - no extra permission is required for null changes of the gid (non-null changes require certain group permission). This is what I changed. - even null changes must update the inode change time (POSIX spec). Similarly for other null changes to attributes. Thus changes to attributes can only end up as null if the inode change time update is null. msdosfs doesn't have uids, gids or inode change times, so it cannot be a POSIX file system and has more possible null changes to attributes. % } % } % @@ -531,7 +533,6 @@ % dep->de_Attributes &= ~ATTR_READONLY; % else % dep->de_Attributes |= ATTR_READONLY; % - dep->de_Attributes |= ATTR_ARCHIVE; % dep->de_flag |= DE_MODIFIED; % } % } This is for chmod(). I don't think this is right. Changes to the mode should be backed up. % --- denode.h.orig Thu Oct 26 11:21:07 2006 % +++ denode.h Mon Dec 4 12:35:00 2006 % @@ -239,6 +239,7 @@ % timespec2fattime((cre), 0, &(dep)->de_CDate, \ % &(dep)->de_CTime, &(dep)->de_CHun); \ % (dep)->de_flag |= DE_MODIFIED; \ % + (dep)->de_Attributes |= ATTR_ARCHIVE; \ % } \ % (dep)->de_flag &= ~(DE_UPDATE | DE_CREATE | DE_ACCESS); \ % } while (0) Hmm, if the setting is forced in DETIMES() then it might cover changes in msdosfs_setattr(), like updating the inode change time does in ffs. but the time here is only the creation time -- the archive flag needs to be set here but it won't cover all changes like the inode change time does. Here is my old patch for DETIMES(): % Index: denode.h % =================================================================== % RCS file: /home/ncvs/src/sys/fs/msdosfs/denode.h,v % retrieving revision 1.27 % diff -u -2 -r1.27 denode.h % --- denode.h 26 Dec 2003 17:24:37 -0000 1.27 % +++ denode.h 27 Dec 2003 07:55:25 -0000 % @@ -220,4 +220,5 @@ % #define DETIMES(dep, acc, mod, cre) do { \ % if ((dep)->de_flag & DE_UPDATE) { \ % + /* XXX missing optimization for no-change case. */ \ % (dep)->de_flag |= DE_MODIFIED; \ % unix2dostime((mod), &(dep)->de_MDate, &(dep)->de_MTime, \ % @@ -236,13 +237,16 @@ % (dep)->de_flag |= DE_MODIFIED; \ % (dep)->de_ADate = adate; \ % + /* XXX intentionally don't set ATTR_ARCHIVE. */ \ % } \ % } \ % if ((dep)->de_flag & DE_CREATE) { \ % + /* XXX missing optimization for no-change case. */ \ % unix2dostime((cre), &(dep)->de_CDate, &(dep)->de_CTime, \ % &(dep)->de_CHun); \ % - (dep)->de_flag |= DE_MODIFIED; \ % + (dep)->de_flag |= DE_MODIFIED; \ % + (dep)->de_Attributes |= ATTR_ARCHIVE; \ % } \ % (dep)->de_flag &= ~(DE_UPDATE | DE_CREATE | DE_ACCESS); \ % -} while (0); % +} while (0) % % /* It is the same as your patch except for comments and an identation fix. I didn't understand DE_CREATE when I wrote it. DE_CREATE is always set to gether with DE_UPDATE, and unless there is a bug it is only set when the file is created (it is quite different from an inode change time). Thus the above change is null (ATTR_ARCHIVE has already been set since DE_UPDATE is set). Bruce