Re: + switch-ntfs-to-touch_atime.patch added to -mm tree

[Anton Altaparmakov <aia21@cam.ac.uk>]

On Tue, 8 Nov 2005, Anton Altaparmakov wrote:
> On Tue, 8 Nov 2005, Christoph Hellwig wrote:
> > [ccing -fsdevel now because this is something of interest to the public]
> > 
> > On Mon, Nov 07, 2005 at 09:51:08PM +0000, Anton Altaparmakov wrote:
> > > What callers are those?  I am curious...  The SUS/Posix standard requires 
> > > it as I said in previous post (except for the no size change case but 
> > > as I also said in previous post the VFS does that wrong in same way as 
> > > ntfs and I just copied vfs)...
> > 
> > Besides the various ->setattr instaces which are supposed to set ctime if
> > ATTR_SIZE|ATTR_CTIME are set (which it is for sys_truncate or
> > sys_ftruncate but possibly not nfsd requests) generic_file_buffered_write()
> > calls vmtruncate and thus ->truncate without asking for ctime updates.
> 
> ntfs doesn't use generic_file_buffered_write() but yes the ntfs version 
> also does it (or will do if it doesn't yet - I had taken it out and 
> can't remember if I had put it back in yet)...
> 
> > The point here is to repeat that the nth time is that you are doing
> > things in ->truncate that you shouldn't do there.  Just move your
> > updates of the base inode to ->setattr, and update ->i_ctime directly
> > like all the other filesystems do instead of using inode_update_time
> > which contains an optimization that's only usefull for a codepath like
> > write where the ctime/mtime update happens very frequently.
> 
> Ok, will do.

Thinking about this some more I finally remembered why ntfs_truncate() 
sets m and ctime on the base inode.  It is because the base inode was 
modified, thus its m and ctime need to reflect this fact.  From the man 
page for stat(2):

[snip]
The field st_mtime is changed by file modifications, e.g. by mknod(2), 
truncate(2), utime(2)  and write(2) (of more than zero bytes).  Moreover, 
st_mtime of a directory is changed by the creation or deletion of files in 
that directory.  The st_mtime field is not changed for changes in owner, 
group, hard link count, or mode.

The field st_ctime is changed by writing or by setting inode information 
(i.e., owner, group, link count, mode, etc.).
[snip]

And ntfs truncate does both of the above to the base inode, so according 
to the above man page it should update the base inode m and ctime.

So what is wrong with that?

For other file systems you do not need to do this because they only have 
one inode.  It would be wrong to do it from ->setattr() because it has 
nothing to do with the base inode.  It is called to update time stamps on 
a specific inode, so we don't want it to change the time stamps on a 
different inode, too.

Although, having said that non-base inodes do not have a/m/c time on disk 
so perhaps you are right that I should be updating the a/m/c time of the 
base inode in addition to the inode itself in ->setattr().

But I still think that ntfs_truncate's modifications of m and ctime of the 
base inode are correct regardless of whether ->setattr() does it because 
ntfs_truncate() is (or at least will be) called from places other than 
->setattr() and it would be silly to have all those other place update the 
cmtimes themselves.

And even your example of file write calling vmtruncate().  I think it is 
correct even there to do an mctimes update of the base inode as the base 
inode was then modified so this should be reflected.

Now that I remembered the actual reason for doing the updates of the base 
inode in ntfs_truncate(), has that changed your mind?

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/