From mboxrd@z Thu Jan 1 00:00:00 1970 From: Subject: Re: [PATCH] NFS server does not update mtime on setattr request Date: Wed, 7 Jun 2006 12:28:29 -0300 Message-ID: <01b801c68a47$07cec3e0$7d01010a@p3> References: <4485C3FE.5070504@redhat.com> <1149658707.27298.10.camel@localhost> <4486E662.5080900@redhat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0053717478==" Cc: Neil Brown , NFS List , Linux Kernel Mailing List Return-path: Received: from sc8-sf-list2-b.sourceforge.net ([10.3.1.8] helo=sc8-sf-list2.sourceforge.net) by sc8-sf-list2-new.sourceforge.net with esmtp (Exim 4.43) id 1Fnzxl-0005k5-BJ for nfs@lists.sourceforge.net; Wed, 07 Jun 2006 08:28:53 -0700 Received: from sc8-sf-mx1-b.sourceforge.net ([10.3.1.91] helo=mail.sourceforge.net) by sc8-sf-list2.sourceforge.net with esmtp (Exim 4.30) id 1Fnzxl-0008W2-3n for nfs@lists.sourceforge.net; Wed, 07 Jun 2006 08:28:53 -0700 Received: from hm397.locaweb.com.br ([200.234.205.160]) by mail.sourceforge.net with smtp (Exim 4.44) id 1Fnzxb-0007LK-J3 for nfs@lists.sourceforge.net; Wed, 07 Jun 2006 08:28:53 -0700 To: "Peter Staubach" , "Trond Myklebust" List-Id: "Discussion of NFS under Linux development, interoperability, and testing." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: nfs-bounces@lists.sourceforge.net Errors-To: nfs-bounces@lists.sourceforge.net This is a multi-part message in MIME format. --===============0053717478== Content-Type: multipart/alternative; boundary="----=_NextPart_000_01B5_01C68A2D.E1FCA470" This is a multi-part message in MIME format. ------=_NextPart_000_01B5_01C68A2D.E1FCA470 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Please, dont wright no more for me. Tanks. ----- Original Message -----=20 From: Peter Staubach=20 To: Trond Myklebust=20 Cc: Neil Brown ; NFS List ; Linux Kernel Mailing List=20 Sent: Wednesday, June 07, 2006 11:44 AM Subject: Re: [PATCH] NFS server does not update mtime on setattr = request Trond Myklebust wrote: >On Tue, 2006-06-06 at 14:05 -0400, Peter Staubach wrote: > > =20 > >>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. >> =20 >> > >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. > > =20 > 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. >> =20 >> > >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=20 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=20 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 - To unsubscribe from this list: send the line "unsubscribe = linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ --=20 No virus found in this incoming message. Checked by AVG Free Edition. Version: 7.1.394 / Virus Database: 268.8.2/357 - Release Date: = 6/6/2006 ------=_NextPart_000_01B5_01C68A2D.E1FCA470 Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
Please, dont wright no more for me. = Tanks.
----- Original Message -----
From:=20 Peter=20 Staubach
Cc: Neil Brown ; NFS=20 List ; Linux Kernel Mailing = List=20
Sent: Wednesday, June 07, 2006 = 11:44=20 AM
Subject: Re: [PATCH] NFS server = does not=20 update mtime on setattr request

Trond Myklebust wrote:

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

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

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

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

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

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

Neil, = can we=20 get these changes integrated, please?

   =20 Thanx...

       ps
-
To = unsubscribe=20 from this list: send the line "unsubscribe linux-kernel" in
the = body of a=20 message to majordomo@vger.kernel.orgMore=20 majordomo info at  http://vger.kernel.or= g/majordomo-info.html
Please=20 read the FAQ at  http://www.tux.org/lkml/


--=20
No virus found in this incoming message.
Checked by AVG Free=20 Edition.
Version: 7.1.394 / Virus Database: 268.8.2/357 - Release = Date:=20 6/6/2006

------=_NextPart_000_01B5_01C68A2D.E1FCA470-- --===============0053717478== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============0053717478== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ NFS maillist - NFS@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs --===============0053717478==--