[PATCH] NFS server does not update mtime on setattr request

[PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

Hi.

Attached aer two patches to address two halves of a problem where NFS does
not update the mtime when an existing file is opened with O_TRUNC.  The
mtime does not get updated when the file is already zero length, although
it should.

On the NFS client side, there was an optimization added which attempted
to avoid an over the wire call if the size of the file was not going to
change.  This would be great, except for the side effect of the mtime
on the file needing to change anyway.  The solution is just to issue the
over the wire call anyway, which, as a side effect, updates the mtime and
ctime fields.

On the NFS server side, there was a change to the routine, inode_setattr(),
which now relies upon the caller to set the ATTR_MTIME and ATTR_CTIME
flags in ia_valid in addition to the ATTR_SIZE.  Previously, this routine
would force these bits on if the size of the file was not changing.  Now,
this routine relies upon the caller to specify all of the fields which need
to be updated.

Comments?

    Thanx...

       ps

Signed-off-by: Peter Staubach <staubach@redhat.com>

[-- Attachment #2: nfs_client_mtime.devel --]
[-- Type: text/plain, Size: 466 bytes --]

--- linux-2.6.16.x86_64/fs/nfs/inode.c.org
+++ linux-2.6.16.x86_64/fs/nfs/inode.c
@@ -947,11 +947,6 @@ nfs_setattr(struct dentry *dentry, struc
 
 	nfs_inc_stats(inode, NFSIOS_VFSSETATTR);
 
-	if (attr->ia_valid & ATTR_SIZE) {
-		if (!S_ISREG(inode->i_mode) || attr->ia_size == i_size_read(inode))
-			attr->ia_valid &= ~ATTR_SIZE;
-	}
-
 	/* Optimization: if the end result is no change, don't RPC */
 	attr->ia_valid &= NFS_VALID_ATTRS;
 	if (attr->ia_valid == 0)

[-- Attachment #3: nfs_server_mtime.devel --]
[-- Type: text/plain, Size: 371 bytes --]

--- linux-2.6.16.x86_64/fs/nfsd/vfs.c.org
+++ linux-2.6.16.x86_64/fs/nfsd/vfs.c
@@ -327,6 +327,11 @@ nfsd_setattr(struct svc_rqst *rqstp, str
 			goto out_nfserr;
 		}
 		DQUOT_INIT(inode);
+		/*
+		 * Make sure that the mtime changes even if the file
+		 * size doesn't actually change.
+		 */
+		iap->ia_valid |= ATTR_MTIME | ATTR_CTIME;
 	}
 
 	imode = inode->i_mode;

Re: [PATCH] NFS server does not update mtime on setattr request

[Trond Myklebust]

On Tue, 2006-06-06 at 14:05 -0400, Peter Staubach wrote:

> On the NFS client side, there was an optimization added which attempted
> to avoid an over the wire call if the size of the file was not going to
> change.  This would be great, except for the side effect of the mtime
> on the file needing to change anyway.  The solution is just to issue the
> over the wire call anyway, which, as a side effect, updates the mtime and
> ctime fields.

Vetoed!

The current code gets it quite right: if someone calls open(O_TRUNC),
then may_open() calls do_truncate() with the ATTR_MTIME|ATTR_CTIME flags
set. That will cause the client to do the right thing _regardless_ of
the size optimisation.

> On the NFS server side, there was a change to the routine, inode_setattr(),
> which now relies upon the caller to set the ATTR_MTIME and ATTR_CTIME
> flags in ia_valid in addition to the ATTR_SIZE.  Previously, this routine
> would force these bits on if the size of the file was not changing.  Now,
> this routine relies upon the caller to specify all of the fields which need
> to be updated.

Also wrong.

This change causes the server to do entirely the wrong thing for
truncate()/ftruncate() calls: in the SuSv3 spec, a call that fails to
change the file length is supposed to leave the file entirely unchanged:
that includes mtime/ctime as well as suid/sgid bits.

Cheers,
  Trond

Re: [PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

Trond Myklebust wrote:

>On Tue, 2006-06-06 at 14:05 -0400, Peter Staubach wrote:
>
>  
>
>>On the NFS client side, there was an optimization added which attempted
>>to avoid an over the wire call if the size of the file was not going to
>>change.  This would be great, except for the side effect of the mtime
>>on the file needing to change anyway.  The solution is just to issue the
>>over the wire call anyway, which, as a side effect, updates the mtime and
>>ctime fields.
>>    
>>
>
>Vetoed!
>
>The current code gets it quite right: if someone calls open(O_TRUNC),
>then may_open() calls do_truncate() with the ATTR_MTIME|ATTR_CTIME flags
>set. That will cause the client to do the right thing _regardless_ of
>the size optimisation.
>
>  
>

You are right.  My testing originally showed something different, but
testing again shows the correct semantics.

I think that the conservative thing to do though, since an over the wire
call is being made anyway, is to remove the optimization and retain the
size change.  The server is already going to have such a check in it
anyway and issuing the SETATTR with the size change in it may reduce
some races.

>>On the NFS server side, there was a change to the routine, inode_setattr(),
>>which now relies upon the caller to set the ATTR_MTIME and ATTR_CTIME
>>flags in ia_valid in addition to the ATTR_SIZE.  Previously, this routine
>>would force these bits on if the size of the file was not changing.  Now,
>>this routine relies upon the caller to specify all of the fields which need
>>to be updated.
>>    
>>
>
>Also wrong.
>
>This change causes the server to do entirely the wrong thing for
>truncate()/ftruncate() calls: in the SuSv3 spec, a call that fails to
>change the file length is supposed to leave the file entirely unchanged:
>that includes mtime/ctime as well as suid/sgid bits.
>

I saw that wording too and assumed what I think that you assumed.  I assumed
that that meant that if the new size is equal to the old size, then nothing
should be changed.  However, that does not seem to be how those words are to
be interpreted.  They are to be interpreted as "if the new length of the 
file
can be successfully set, then the mtime/ctime should be changed".  It does
not matter if the new length was the same as the old length or not.  Linux
implements this semantic, as you pointed out above regarding the client side
changes with the passing of ATTR_MTIME|ATTR_CTIME to do_truncate().  SunOS
also implements this semantic.

Therefore, I believe that this patch should stand because it modifies 
the NFS
server to do the right thing and indeed, to match the current semantics of
do_truncate().  The older version of do_truncate() would have set ATTR_MTIME
and ATTR_CTIME itself if the file size was not actually changing.  Clients
such as SunOS and older versions of Linux will require this change in order
to properly interoperate with a new enough Linux NFS server.

Neil, can we get these changes integrated, please?

    Thanx...

       ps

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[J. Bruce Fields]

On Wed, Jun 07, 2006 at 10:44:50AM -0400, Peter Staubach wrote:
> I saw that wording too and assumed what I think that you assumed.  I
> assumed that that meant that if the new size is equal to the old size,
> then nothing should be changed.  However, that does not seem to be how
> those words are to be interpreted.  They are to be interpreted as "if
> the new length of the file can be successfully set, then the
> mtime/ctime should be changed".

What's the basis for that interpretation?  The language seems extremely
clear:

	"On successful completion, if the file size is changed, these
	functions will mark for update the st_ctime and st_mtime fields
	of the file, and if the file is a regular file, the S_ISUID and
	S_ISGID bits of the file mode may be cleared."

Why are you concerned about this?  Do you have an actual application
that breaks?

--b.

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

J. Bruce Fields wrote:

>On Wed, Jun 07, 2006 at 10:44:50AM -0400, Peter Staubach wrote:
>  
>
>>I saw that wording too and assumed what I think that you assumed.  I
>>assumed that that meant that if the new size is equal to the old size,
>>then nothing should be changed.  However, that does not seem to be how
>>those words are to be interpreted.  They are to be interpreted as "if
>>the new length of the file can be successfully set, then the
>>mtime/ctime should be changed".
>>    
>>
>
>What's the basis for that interpretation?  The language seems extremely
>clear:
>
>	"On successful completion, if the file size is changed, these
>	functions will mark for update the st_ctime and st_mtime fields
>	of the file, and if the file is a regular file, the S_ISUID and
>	S_ISGID bits of the file mode may be cleared."
>
>Why are you concerned about this?  Do you have an actual application
>that breaks?
>

Yes, there is a customer who is quite unhappy that the semantics over Linux
client NFS are different than those of BSD, Solaris, and local file system
access on Linux itself.  The basis for my work is based on a bugzilla from
this customer.

My interpretation is based on looking at the local behavior on Linux, which
changes mtime/ctime even if the file size does not change, and SunOS, which
changes mtime/ctime even if the file size does not change and is very
heavily SUSv3 compliant.

In this case, "changed" does not mean "made different".  It simply means
that the file size is set to the new value.

I would have chosen different words or a different interpretation too,
but all of the evidence suggests that the semantics are as I stated.

    Thanx...

       ps

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Trond Myklebust]

On Wed, 2006-06-07 at 11:26 -0400, Peter Staubach wrote:
> J. Bruce Fields wrote:
> 
> >On Wed, Jun 07, 2006 at 10:44:50AM -0400, Peter Staubach wrote:
> >  
> >
> >>I saw that wording too and assumed what I think that you assumed.  I
> >>assumed that that meant that if the new size is equal to the old size,
> >>then nothing should be changed.  However, that does not seem to be how
> >>those words are to be interpreted.  They are to be interpreted as "if
> >>the new length of the file can be successfully set, then the
> >>mtime/ctime should be changed".
> >>    
> >>
> >
> >What's the basis for that interpretation?  The language seems extremely
> >clear:
> >
> >	"On successful completion, if the file size is changed, these
> >	functions will mark for update the st_ctime and st_mtime fields
> >	of the file, and if the file is a regular file, the S_ISUID and
> >	S_ISGID bits of the file mode may be cleared."
> >
> >Why are you concerned about this?  Do you have an actual application
> >that breaks?
> >
> 
> Yes, there is a customer who is quite unhappy that the semantics over Linux
> client NFS are different than those of BSD, Solaris, and local file system
> access on Linux itself.  The basis for my work is based on a bugzilla from
> this customer.
> 
> My interpretation is based on looking at the local behavior on Linux, which
> changes mtime/ctime even if the file size does not change, and SunOS, which
> changes mtime/ctime even if the file size does not change and is very
> heavily SUSv3 compliant.
> 
> In this case, "changed" does not mean "made different".  It simply means
> that the file size is set to the new value.
> 
> I would have chosen different words or a different interpretation too,
> but all of the evidence suggests that the semantics are as I stated.

We've already fixed this to be SuSv3 compliant for both create and
truncate. Your "safe" suggestion would break truncate again. That is why
it is being vetoed.

Cheers,
  Trond

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[J. Bruce Fields]

On Wed, Jun 07, 2006 at 11:26:24AM -0400, Peter Staubach wrote:
> J. Bruce Fields wrote:
> >What's the basis for that interpretation?  The language seems extremely
> >clear:
> >
> >	"On successful completion, if the file size is changed, these
> >	functions will mark for update the st_ctime and st_mtime fields
> >	of the file, and if the file is a regular file, the S_ISUID and
> >	S_ISGID bits of the file mode may be cleared."
> >
> >Why are you concerned about this?  Do you have an actual application
> >that breaks?
> 
> Yes, there is a customer who is quite unhappy that the semantics over Linux
> client NFS are different than those of BSD, Solaris, and local file system
> access on Linux itself.  The basis for my work is based on a bugzilla from
> this customer.

OK; just out of curiosity, what's the url/bug number/whatever?

> My interpretation is based on looking at the local behavior on Linux, which
> changes mtime/ctime even if the file size does not change, and SunOS, which
> changes mtime/ctime even if the file size does not change and is very
> heavily SUSv3 compliant.

Fair enough.

> In this case, "changed" does not mean "made different".  It simply means
> that the file size is set to the new value.

That's ridiculous, though; that's just not what "changed" means, and
that renders the "if" clause redundant.  Better just to say "SUS is
wrong, and this is what everybody actually does...."

--b.

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

Trond Myklebust wrote:

>On Wed, 2006-06-07 at 11:26 -0400, Peter Staubach wrote:
>  
>
>>J. Bruce Fields wrote:
>>
>>    
>>
>>>On Wed, Jun 07, 2006 at 10:44:50AM -0400, Peter Staubach wrote:
>>> 
>>>
>>>      
>>>
>>>>I saw that wording too and assumed what I think that you assumed.  I
>>>>assumed that that meant that if the new size is equal to the old size,
>>>>then nothing should be changed.  However, that does not seem to be how
>>>>those words are to be interpreted.  They are to be interpreted as "if
>>>>the new length of the file can be successfully set, then the
>>>>mtime/ctime should be changed".
>>>>   
>>>>
>>>>        
>>>>
>>>What's the basis for that interpretation?  The language seems extremely
>>>clear:
>>>
>>>	"On successful completion, if the file size is changed, these
>>>	functions will mark for update the st_ctime and st_mtime fields
>>>	of the file, and if the file is a regular file, the S_ISUID and
>>>	S_ISGID bits of the file mode may be cleared."
>>>
>>>Why are you concerned about this?  Do you have an actual application
>>>that breaks?
>>>
>>>      
>>>
>>Yes, there is a customer who is quite unhappy that the semantics over Linux
>>client NFS are different than those of BSD, Solaris, and local file system
>>access on Linux itself.  The basis for my work is based on a bugzilla from
>>this customer.
>>
>>My interpretation is based on looking at the local behavior on Linux, which
>>changes mtime/ctime even if the file size does not change, and SunOS, which
>>changes mtime/ctime even if the file size does not change and is very
>>heavily SUSv3 compliant.
>>
>>In this case, "changed" does not mean "made different".  It simply means
>>that the file size is set to the new value.
>>
>>I would have chosen different words or a different interpretation too,
>>but all of the evidence suggests that the semantics are as I stated.
>>    
>>
>
>We've already fixed this to be SuSv3 compliant for both create and
>truncate. Your "safe" suggestion would break truncate again. That is why
>it is being vetoed.
>

You are speaking of the client side changes?  I don't have a problem with
not taking them.

I am curious about how this would break truncate?

    Thanx...

       ps

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

J. Bruce Fields wrote:

>On Wed, Jun 07, 2006 at 11:26:24AM -0400, Peter Staubach wrote:
>  
>
>>J. Bruce Fields wrote:
>>    
>>
>>>What's the basis for that interpretation?  The language seems extremely
>>>clear:
>>>
>>>	"On successful completion, if the file size is changed, these
>>>	functions will mark for update the st_ctime and st_mtime fields
>>>	of the file, and if the file is a regular file, the S_ISUID and
>>>	S_ISGID bits of the file mode may be cleared."
>>>
>>>Why are you concerned about this?  Do you have an actual application
>>>that breaks?
>>>      
>>>
>>Yes, there is a customer who is quite unhappy that the semantics over Linux
>>client NFS are different than those of BSD, Solaris, and local file system
>>access on Linux itself.  The basis for my work is based on a bugzilla from
>>this customer.
>>    
>>
>
>OK; just out of curiosity, what's the url/bug number/whatever?
>
>  
>

The Red Hat BZ number is 193621.  The description is that when zero length
files are copied, even over an existing zero length file, the mtime on the
target file does not change.

       ps

>>My interpretation is based on looking at the local behavior on Linux, which
>>changes mtime/ctime even if the file size does not change, and SunOS, which
>>changes mtime/ctime even if the file size does not change and is very
>>heavily SUSv3 compliant.
>>    
>>
>
>Fair enough.
>
>  
>
>>In this case, "changed" does not mean "made different".  It simply means
>>that the file size is set to the new value.
>>    
>>
>
>That's ridiculous, though; that's just not what "changed" means, and
>that renders the "if" clause redundant.  Better just to say "SUS is
>wrong, and this is what everybody actually does...."
>

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[J. Bruce Fields]

On Wed, Jun 07, 2006 at 11:50:31AM -0400, Peter Staubach wrote:
> The Red Hat BZ number is 193621.

"You are not authorized to access bug #193621", it tells me....

> The description is that when zero length files are copied, even over
> an existing zero length file, the mtime on the target file does not
> change.

Is the server-side patch sufficient on its own?

--b.

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

J. Bruce Fields wrote:

>On Wed, Jun 07, 2006 at 11:50:31AM -0400, Peter Staubach wrote:
>  
>
>>The Red Hat BZ number is 193621.
>>    
>>
>
>"You are not authorized to access bug #193621", it tells me....
>
>  
>

Hmm.  That ones seems to be restricted for some reason.  I think that
this happens when we get escalated bugzillas from customers.


>>The description is that when zero length files are copied, even over
>>an existing zero length file, the mtime on the target file does not
>>change.
>>    
>>
>
>Is the server-side patch sufficient on its own?
>  
>

The server side patch isn't quite sufficient on its own.  A RHEL-4 patch
is also required for the client side.  I could construct the RHEL-4 patch
so that it alone would be sufficient to address the particular problem
that that customer is having, but that isn't the entire situation.
Non-Linux clients would still have a problem with the current upstream
Linux server.  For example, in my testing, a Solaris 10 client mounting
an FC-5 server fails.  When running the attached script, the mtime on
the file, bar, should change by about 1 minute, 3 times.

    Thanx...

       ps

[-- Attachment #2: repo --]
[-- Type: text/plain, Size: 265 bytes --]

#!/bin/sh

rm -f foo bar

set -x

touch foo

cp foo bar

stat --format="%n %y" foo bar

sleep 60

cp foo bar

stat --format="%n %y" foo bar

sleep 60

cp foo bar

stat --format="%n %y" foo bar

sleep 60

rm foo

touch foo

cp foo bar

stat --format="%n %y" foo bar

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Trond Myklebust]

On Wed, 2006-06-07 at 11:44 -0400, Peter Staubach wrote:

> I am curious about how this would break truncate?

According to SuSv43, truncate should result in changes to
mtime/ctime/suid/sgid if and only if the file size changes. The
combination of disabling the client caching and always setting
mtime/ctime on the server will therefore clearly break truncate.

Cheers,
  Trond

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

Trond Myklebust wrote:

>On Wed, 2006-06-07 at 11:44 -0400, Peter Staubach wrote:
>
>  
>
>>I am curious about how this would break truncate?
>>    
>>
>
>According to SuSv43, truncate should result in changes to
>mtime/ctime/suid/sgid if and only if the file size changes. The
>combination of disabling the client caching and always setting
>mtime/ctime on the server will therefore clearly break truncate.
>

Okay, I see that.

Someone should probably alert the Solaris folks that they might have a 
bug in
their NFS clients.  I suspect that they are only sending over the size 
element
in some of the over the wire SETATTR calls when they really should be 
sending
the size and mtime elements.  This might head off a potential customer issue
where they blaim Linux instead of Solaris.

    Thanx...

       ps

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Neil Brown]

On Wednesday June 7, staubach@redhat.com wrote:
> 
> Neil, can we get these changes integrated, please?

Nope.
The discussion has already gone on from here, so I might be covering
old ground, but there seem to be further mentions of still needing the
server patch, so just to be sure it is covered....

My reading of SUS says that 
  open(O_TRUNC) of an empty file does not update the modify time
  truncate() of an empty file does update the modify time

So the server has to be able to support this distinction without being
able to directly know what API call was made on the client.
The patch you suggest makes it impossible to support that distinction.

Possibly the server could make a distinction between when nfsd_setattr
is called directly, and when it is called via nfsd_create{,_v3}.  I
would be more open to a patch that makes a distinction there.  However
I think that it would be best for the client to be explicit about what
it is doing by setting the right attr flags.

NeilBrown

Re: [NFS] [PATCH] NFS server does not update mtime on setattr request

[Peter Staubach]

Neil Brown wrote:

>On Wednesday June 7, staubach@redhat.com wrote:
>  
>
>>Neil, can we get these changes integrated, please?
>>    
>>
>
>Nope.
>The discussion has already gone on from here, so I might be covering
>old ground, but there seem to be further mentions of still needing the
>server patch, so just to be sure it is covered....
>
>My reading of SUS says that 
>  open(O_TRUNC) of an empty file does not update the modify time
>  truncate() of an empty file does update the modify time
>
>  
>

These seem backwards.

>So the server has to be able to support this distinction without being
>able to directly know what API call was made on the client.
>The patch you suggest makes it impossible to support that distinction.
>
>Possibly the server could make a distinction between when nfsd_setattr
>is called directly, and when it is called via nfsd_create{,_v3}.  I
>would be more open to a patch that makes a distinction there.  However
>I think that it would be best for the client to be explicit about what
>it is doing by setting the right attr flags.
>

Yup, agreed.  I do think that we are going to have some interoperability
problems with some existing clients in the marketplace though.  Perhaps
we can get some of those folks to look at their clients and make them a
little more explicit about what bits needs to be changed, as opposed to
assuming that the server would just do what the client expected.

    Thanx...

       ps