Hi Roger,
Roger Willcocks wrote:
Timothy Shimmin wrote:
Hi Roger,
...
I don't like all these inconsistencies.
Take a look at the attached patch relative to the current cvs (it's a
bit big to put
inline). The basic problem is it's currently unclear when to set the
times from
va_atime etc. and when to set them to the current time. So I've used the
already
defined XFS_AT_UPDxTIME flags to indicate that a time should be set to
'now'
and XFS_AT_xTIME to mean set it using va_xtime. This seems to fit well with
the current code and I wonder if that's how it was meant to work in the
first
place.
Yeah, I've looked at this a few times now ;-) and this _seems_ like
a reasonable thing to do to me.
So patch:
ATTR_ATIME_SET => XFS_AT_ATIME (& set va_atime etc) (used to set to given time)
ATTR_ATIME => XFS_AT_UPDATIME (used to set to "now")
likewise for M variant.
Previously:
ATTR_ATIME_SET => ATTR_UTIME flag (used to set given time)
must expect ATTR_ATIME to be set too to get va_atime
ATTR_ATIME => XFS_AT_ATIME (& set va_atime) (used to set to "now")
a bit confusing since it can store va_atime even if
ATTR_ATIME_SET is not on
I've also removed the now redundant ATTR_UTIME flag and pulled
the null truncate to the top, which simplifies things.
So these changes of:
if (mask & (XFS_AT_ATIME|XFS_AT_MTIME)) {
if (!file_owner) {
- if ((flags & ATTR_UTIME) &&
- !capable(CAP_FOWNER)) {
+ if (!capable(CAP_FOWNER)) {
Where you take out ATTR_UTIME make sense since XFS_AT_ATIME et al,
now refer to the case where a given time is provided
instead of requiring ATTR_UTIME to be set.
One query: in both xfs_iops.c/xfs_vn_setattr and
xfs_dm.c/xfs_dm_set_fileattr the
ATIME branch sets the inode's atime directly.
xfs_vn_setattr()
if (ia_valid & ATTR_ATIME) {
vattr.va_mask |= XFS_AT_ATIME;
vattr.va_atime = attr->ia_atime;
inode->i_atime = attr->ia_atime;
}
xfs_dm_set_fileattr()
if (mask & DM_AT_ATIME) {
vat.va_mask |= XFS_AT_ATIME;
vat.va_atime.tv_sec = stat.fa_atime;
vat.va_atime.tv_nsec = 0;
inode->i_atime.tv_sec = stat.fa_atime;
}
Hmmm....
So this could change behavior for xfs_vn_setattr().
If previously we had ATTR_ATIME set but NOT ATTR_ATIME_SET,
then we would set inode->i_atime.
Now with the patch, in this case, we don't set inode->i_atime
at this point.
However, in this case we wouldn't want i_atime to be set to ia_atime
as we would want it to be set to "now" in xfs_ichgtime().
This is probably something
to do with
the comment above xfs_iops.c/xfs_ichgtime ('to make sure the access time
update
will take') but it could probably be handled better.
I'll need to look.
BTW, your locking looks wrong - it appears you don't unlock when the
file is non-zero size.
Oops...
I was also thinking of a read lock here.
And initializing quot vars to zero in variable definition at top.
This stuff really needs to be QA'ed well.
It would be too easy to get a regression in expected behavior.
Need to hunt out qa tests.
Thanks for the effort,
Tim.
|