Date: Tue, 5 Dec 2006 01:50:09 GMT From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/106255: [msdosfs] : correct setting of archive flag Message-ID: <200612050150.kB51o9v6069493@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/106255; it has been noted by GNATS.
From: Bruce Evans <bde@zeta.org.au>
To: Rene Ladan <r.c.ladan@gmail.com>
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200612050150.kB51o9v6069493>