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

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

[Christoph Hellwig]

[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.

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.

Your use of the function is wrong no matter whether it takes a struct
file or struct inode.

> > so just update them directly for those callers.
> 
> That is difficult to do as VFS ->truncate() does not allow an error to be 
> returned.  And the setting must only happen on success for {f,}truncate().
> 
> Thus vmtruncate() always returns success even when ->truncate() failed, so 
> VFS _cannot_ be compliant with standard unless it is changed so that 
> ->truncate() can return error and vmtruncate() passes that upward to 
> ->setattr().  Otherwise you have to do the cmtime updates from 
> ->truncate() on success and not do them on failure.

so please submit a patch to allow an error return from ->truncate.

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

[Anton Altaparmakov]

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.

> Your use of the function is wrong no matter whether it takes a struct
> file or struct inode.
> 
> > > so just update them directly for those callers.
> > 
> > That is difficult to do as VFS ->truncate() does not allow an error to be 
> > returned.  And the setting must only happen on success for {f,}truncate().
> > 
> > Thus vmtruncate() always returns success even when ->truncate() failed, so 
> > VFS _cannot_ be compliant with standard unless it is changed so that 
> > ->truncate() can return error and vmtruncate() passes that upward to 
> > ->setattr().  Otherwise you have to do the cmtime updates from 
> > ->truncate() on success and not do them on failure.
> 
> so please submit a patch to allow an error return from ->truncate.

As I said I don't care about conforming to standards.  I just want to do 
same as all other Linux fs.  I have enough to do as it is without 
working on and submitting random patches...

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/

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

[Anton Altaparmakov]

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/

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

[Anton Altaparmakov]

A question...

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

How come inode_setattr() sets both c and mtime unconditionally but 
inode/file_update_time() makes the setting conditional on IS_NOCMTIME() 
being false?

It seems that either they should always be set or they should never be 
set when IS_NOCMTIME() is true, and not be set half the time and not half 
the other time, no?

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/

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

[Dave Kleikamp]

On Tue, 2005-11-08 at 10:41 +0000, Anton Altaparmakov wrote:
> How come inode_setattr() sets both c and mtime unconditionally but 
> inode/file_update_time() makes the setting conditional on IS_NOCMTIME() 
> being false?
> 
> It seems that either they should always be set or they should never be 
> set when IS_NOCMTIME() is true, and not be set half the time and not half 
> the other time, no?

IS_NOCMTIME() is useful to network file systems (nfs specifically) that
rely on the server setting the ctime & mtime when the inode is modified.
Of course, if the ctime & mtime are explicitly set by setattr, the
client will need to change these fields.

Local file systems don't set S_NOCMTIME, so you will get the behavior
you want.  (I don't know enough about fuse to see why it sets this
flag.)
-- 
David Kleikamp
IBM Linux Technology Center

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

[Trond Myklebust]

On Tue, 2005-11-08 at 09:17 -0600, Dave Kleikamp wrote:
> On Tue, 2005-11-08 at 10:41 +0000, Anton Altaparmakov wrote:
> > How come inode_setattr() sets both c and mtime unconditionally but 
> > inode/file_update_time() makes the setting conditional on IS_NOCMTIME() 
> > being false?
> > 
> > It seems that either they should always be set or they should never be 
> > set when IS_NOCMTIME() is true, and not be set half the time and not half 
> > the other time, no?
> 
> IS_NOCMTIME() is useful to network file systems (nfs specifically) that
> rely on the server setting the ctime & mtime when the inode is modified.
> Of course, if the ctime & mtime are explicitly set by setattr, the
> client will need to change these fields.

Right. IS_NOCMTIME was introduced to stop the _kernel_ from setting
ctime and mtime on those filesystems for which this is inappropriate.

It is not intended to prevent the user from doing so.

Cheers,
  Trond

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

[Anton Altaparmakov]

On Tue, 8 Nov 2005, Trond Myklebust wrote:

> On Tue, 2005-11-08 at 09:17 -0600, Dave Kleikamp wrote:
> > On Tue, 2005-11-08 at 10:41 +0000, Anton Altaparmakov wrote:
> > > How come inode_setattr() sets both c and mtime unconditionally but 
> > > inode/file_update_time() makes the setting conditional on IS_NOCMTIME() 
> > > being false?
> > > 
> > > It seems that either they should always be set or they should never be 
> > > set when IS_NOCMTIME() is true, and not be set half the time and not half 
> > > the other time, no?
> > 
> > IS_NOCMTIME() is useful to network file systems (nfs specifically) that
> > rely on the server setting the ctime & mtime when the inode is modified.
> > Of course, if the ctime & mtime are explicitly set by setattr, the
> > client will need to change these fields.
> 
> Right. IS_NOCMTIME was introduced to stop the _kernel_ from setting
> ctime and mtime on those filesystems for which this is inappropriate.
> 
> It is not intended to prevent the user from doing so.

Ok, thanks to both of you for explaining!

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/