Re: set last write time = fsync ?

Re: set last write time = fsync ?

[Steve French]

On Thu, Mar 13, 2008 at 6:10 AM, Jeff Layton <jlayton@redhat.com> wrote:
>
> On Wed, 12 Mar 2008 22:54:50 -0500
>  "Steve French" <smfrench@gmail.com> wrote:
>
>  > kukks noticed that "cp -p source-file /cifs-mnt" would not set the
>  > last write time to the previous time
>  >
>  > It looks like a straightforward caching sideffect - "cp -p" does
>  >
>  > open
>  > write
>  > set time stamps (utimensat)
>  > set uid/gid (fchown32)
>  > set acl (setxattr(system.posix_acl_access))
>  >
>  > but the write is cached until close (which does a flush just before
>  > the close) - so the final write resets the time which was set
>  > correctly earlier
>  >
>  Interesting -- this seems to be "mtime" week for me. I've been working
>  on some NFS cache invalidation issues that involve out-of-order mtime
>  updates...
>
>  Actually, this problem looks like a regression caused by the changes
>  in this patch:
>
>  commit cea218054ad277d6c126890213afde07b4eb1602
>  Author: Jeff Layton <jlayton@redhat.com>
>  Date:   Tue Nov 20 23:19:03 2007 +0000
>
>     [CIFS] Fix potential data corruption when writing out cached dirty pages
>
>
>  ...before that patch we flushed the cache on every setattr call. We now
>  just do that on size changes, but it sounds like we need to reconsider
>  this.
>
>  Trying to follow Lustre's model sounds tricky and I'm not sure it would
>  work anyway since the server is going to update the mtime whenever the
>  write comes in. Consider:
>
>  Write --> server sets mtime to current time
>  Setattr --> server sets mtime for utimes() call
>  Write --> server resets mtime to current time
>
>  ...I'm not sure that the client has much control over this. If we don't
>  want to flush the cache for a setattr, there are two options:
>
>  1) delay the setattr until all of the pending writes have been flushed
>  (tricky and not really what we want)
>
>  2) queue up another setattr call after the last write has completed
>  (also tricky, and possibly problematic)
>
>  There could be other problems lurking here too. What happens to the
>  pending writes when we change the ownership and mode to something where
>  we no longer have permission to write to the file? I suppose they just
>  error out and that's not good either.
>
>  For the record, NFS seems to flush the cache too on all setattr calls.
>
>  Jeff Layton <jlayton@redhat.com>

Here is a patch to fix the cp -p problem:

Kukks/Jeff,
coments?  Unless there are objections I want to add this to the tree
quickly (I also need to add Q's memleak fix - but want to try it
against more servers to see if there is a better way to do that)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index af42262..e57e5c4 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1420,11 +1420,10 @@ int cifs_setattr(struct dentry *direntry,
struct iattr *attrs)
 	}
 	cifsInode = CIFS_I(direntry->d_inode);

-	/* BB check if we need to refresh inode from server now ? BB */
-
-	if (attrs->ia_valid & ATTR_SIZE) {
+	if ((attrs->ia_valid & ATTR_MTIME) || (attrs->ia_valid & ATTR_SIZE)) {
 		/*
-		   Flush data before changing file size on server. If the
+		   Flush data before changing file size or changing the last
+		   write time of the file on the server. If the
 		   flush returns error, store it to report later and continue.
 		   BB: This should be smarter. Why bother flushing pages that
 		   will be truncated anyway? Also, should we error out here if
@@ -1435,7 +1434,9 @@ int cifs_setattr(struct dentry *direntry, struct
iattr *attrs)
 			CIFS_I(direntry->d_inode)->write_behind_rc = rc;
 			rc = 0;
 		}
+	}

+	if (attrs->ia_valid & ATTR_SIZE) {
 		/* To avoid spurious oplock breaks from server, in the case of
 		   inodes that we already have open, avoid doing path based
 		   setting of file size if we can do it by handle.



-- 
Thanks,

Steve

Re: set last write time = fsync ?

[Jeff Layton]

On Thu, 13 Mar 2008 21:32:32 -0500
"Steve French" <smfrench@gmail.com> wrote:

> On Thu, Mar 13, 2008 at 6:10 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >
> > On Wed, 12 Mar 2008 22:54:50 -0500
> >  "Steve French" <smfrench@gmail.com> wrote:
> >
> >  > kukks noticed that "cp -p source-file /cifs-mnt" would not set the
> >  > last write time to the previous time
> >  >
> >  > It looks like a straightforward caching sideffect - "cp -p" does
> >  >
> >  > open
> >  > write
> >  > set time stamps (utimensat)
> >  > set uid/gid (fchown32)
> >  > set acl (setxattr(system.posix_acl_access))
> >  >
> >  > but the write is cached until close (which does a flush just before
> >  > the close) - so the final write resets the time which was set
> >  > correctly earlier
> >  >
> >  Interesting -- this seems to be "mtime" week for me. I've been working
> >  on some NFS cache invalidation issues that involve out-of-order mtime
> >  updates...
> >
> >  Actually, this problem looks like a regression caused by the changes
> >  in this patch:
> >
> >  commit cea218054ad277d6c126890213afde07b4eb1602
> >  Author: Jeff Layton <jlayton@redhat.com>
> >  Date:   Tue Nov 20 23:19:03 2007 +0000
> >
> >     [CIFS] Fix potential data corruption when writing out cached dirty pages
> >
> >
> >  ...before that patch we flushed the cache on every setattr call. We now
> >  just do that on size changes, but it sounds like we need to reconsider
> >  this.
> >
> >  Trying to follow Lustre's model sounds tricky and I'm not sure it would
> >  work anyway since the server is going to update the mtime whenever the
> >  write comes in. Consider:
> >
> >  Write --> server sets mtime to current time
> >  Setattr --> server sets mtime for utimes() call
> >  Write --> server resets mtime to current time
> >
> >  ...I'm not sure that the client has much control over this. If we don't
> >  want to flush the cache for a setattr, there are two options:
> >
> >  1) delay the setattr until all of the pending writes have been flushed
> >  (tricky and not really what we want)
> >
> >  2) queue up another setattr call after the last write has completed
> >  (also tricky, and possibly problematic)
> >
> >  There could be other problems lurking here too. What happens to the
> >  pending writes when we change the ownership and mode to something where
> >  we no longer have permission to write to the file? I suppose they just
> >  error out and that's not good either.
> >
> >  For the record, NFS seems to flush the cache too on all setattr calls.
> >
> >  Jeff Layton <jlayton@redhat.com>
> 
> Here is a patch to fix the cp -p problem:
> 
> Kukks/Jeff,
> coments?  Unless there are objections I want to add this to the tree
> quickly (I also need to add Q's memleak fix - but want to try it
> against more servers to see if there is a better way to do that)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index af42262..e57e5c4 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1420,11 +1420,10 @@ int cifs_setattr(struct dentry *direntry,
> struct iattr *attrs)
>  	}
>  	cifsInode = CIFS_I(direntry->d_inode);
> 
> -	/* BB check if we need to refresh inode from server now ? BB */
> -
> -	if (attrs->ia_valid & ATTR_SIZE) {
> +	if ((attrs->ia_valid & ATTR_MTIME) || (attrs->ia_valid & ATTR_SIZE)) {
>  		/*
> -		   Flush data before changing file size on server. If the
> +		   Flush data before changing file size or changing the last
> +		   write time of the file on the server. If the
>  		   flush returns error, store it to report later and continue.
>  		   BB: This should be smarter. Why bother flushing pages that
>  		   will be truncated anyway? Also, should we error out here if
> @@ -1435,7 +1434,9 @@ int cifs_setattr(struct dentry *direntry, struct
> iattr *attrs)
>  			CIFS_I(direntry->d_inode)->write_behind_rc = rc;
>  			rc = 0;
>  		}
> +	}
> 
> +	if (attrs->ia_valid & ATTR_SIZE) {
>  		/* To avoid spurious oplock breaks from server, in the case of
>  		   inodes that we already have open, avoid doing path based
>  		   setting of file size if we can do it by handle.
> 
> 
> 

That should fix Kukks' problem, but still leaves us vulnerable to the
other problems. For instance:

Changing UID/GID or mode could cause later writes to fail.

What happens if we still have outstanding writes, but we've set
ATTR_READONLY on the file on the server?

What about the atime? Pending writes will clobber it.

I think the only thing we can really do is flush on every setattr call.
The only situation I can see where we don't need to flush is possibly
where we're *only* changing the ctime, and it's probably not worth it
to make that a special case.

How about this patch instead?

Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index af42262..8bcbfff 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1420,22 +1420,20 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 	}
 	cifsInode = CIFS_I(direntry->d_inode);
 
-	/* BB check if we need to refresh inode from server now ? BB */
+	/*
+	   Flush data before changing attributes on server. If the
+	   flush returns error, store it to report later and continue.
+	   BB: This should be smarter. Why bother flushing pages that
+	   will be truncated anyway? Also, what should we do if the
+	   the flush returns error?
+	 */
+	rc = filemap_write_and_wait(direntry->d_inode->i_mapping);
+	if (rc != 0) {
+		CIFS_I(direntry->d_inode)->write_behind_rc = rc;
+		rc = 0;
+	}
 
 	if (attrs->ia_valid & ATTR_SIZE) {
-		/*
-		   Flush data before changing file size on server. If the
-		   flush returns error, store it to report later and continue.
-		   BB: This should be smarter. Why bother flushing pages that
-		   will be truncated anyway? Also, should we error out here if
-		   the flush returns error?
-		 */
-		rc = filemap_write_and_wait(direntry->d_inode->i_mapping);
-		if (rc != 0) {
-			CIFS_I(direntry->d_inode)->write_behind_rc = rc;
-			rc = 0;
-		}
-
 		/* To avoid spurious oplock breaks from server, in the case of
 		   inodes that we already have open, avoid doing path based
 		   setting of file size if we can do it by handle.

Re: set last write time = fsync ?

[Steve French]

I don't worry about flushing atime (anyone crazy enough to do that
would pay a huge performance penalty).
Access is usually checked on open right ... so once a file is open
even if the file becomes read-only, the writes, even cached writes
continue.

On Fri, Mar 14, 2008 at 5:40 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Thu, 13 Mar 2008 21:32:32 -0500
>
>
> "Steve French" <smfrench@gmail.com> wrote:
>
>  > On Thu, Mar 13, 2008 at 6:10 AM, Jeff Layton <jlayton@redhat.com> wrote:
>  > >
>  > > On Wed, 12 Mar 2008 22:54:50 -0500
>  > >  "Steve French" <smfrench@gmail.com> wrote:
>  > >
>  > >  > kukks noticed that "cp -p source-file /cifs-mnt" would not set the
>  > >  > last write time to the previous time
>  > >  >
>  > >  > It looks like a straightforward caching sideffect - "cp -p" does
>  > >  >
>  > >  > open
>  > >  > write
>  > >  > set time stamps (utimensat)
>  > >  > set uid/gid (fchown32)
>  > >  > set acl (setxattr(system.posix_acl_access))
>  > >  >
>  > >  > but the write is cached until close (which does a flush just before
>  > >  > the close) - so the final write resets the time which was set
>  > >  > correctly earlier
>  > >  >
>  > >  Interesting -- this seems to be "mtime" week for me. I've been working
>  > >  on some NFS cache invalidation issues that involve out-of-order mtime
>  > >  updates...
>  > >
>  > >  Actually, this problem looks like a regression caused by the changes
>  > >  in this patch:
>  > >
>  > >  commit cea218054ad277d6c126890213afde07b4eb1602
>  > >  Author: Jeff Layton <jlayton@redhat.com>
>  > >  Date:   Tue Nov 20 23:19:03 2007 +0000
>  > >
>  > >     [CIFS] Fix potential data corruption when writing out cached dirty pages
>  > >
>  > >
>  > >  ...before that patch we flushed the cache on every setattr call. We now
>  > >  just do that on size changes, but it sounds like we need to reconsider
>  > >  this.
>  > >
>  > >  Trying to follow Lustre's model sounds tricky and I'm not sure it would
>  > >  work anyway since the server is going to update the mtime whenever the
>  > >  write comes in. Consider:
>  > >
>  > >  Write --> server sets mtime to current time
>  > >  Setattr --> server sets mtime for utimes() call
>  > >  Write --> server resets mtime to current time
>  > >
>  > >  ...I'm not sure that the client has much control over this. If we don't
>  > >  want to flush the cache for a setattr, there are two options:
>  > >
>  > >  1) delay the setattr until all of the pending writes have been flushed
>  > >  (tricky and not really what we want)
>  > >
>  > >  2) queue up another setattr call after the last write has completed
>  > >  (also tricky, and possibly problematic)
>  > >
>  > >  There could be other problems lurking here too. What happens to the
>  > >  pending writes when we change the ownership and mode to something where
>  > >  we no longer have permission to write to the file? I suppose they just
>  > >  error out and that's not good either.
>  > >
>  > >  For the record, NFS seems to flush the cache too on all setattr calls.
>  > >
>  > >  Jeff Layton <jlayton@redhat.com>
>  >
>  > Here is a patch to fix the cp -p problem:
>  >
>  > Kukks/Jeff,
>  > coments?  Unless there are objections I want to add this to the tree
>  > quickly (I also need to add Q's memleak fix - but want to try it
>  > against more servers to see if there is a better way to do that)
>  >
>  > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>  > index af42262..e57e5c4 100644
>  > --- a/fs/cifs/inode.c
>  > +++ b/fs/cifs/inode.c
>  > @@ -1420,11 +1420,10 @@ int cifs_setattr(struct dentry *direntry,
>  > struct iattr *attrs)
>  >       }
>  >       cifsInode = CIFS_I(direntry->d_inode);
>  >
>  > -     /* BB check if we need to refresh inode from server now ? BB */
>  > -
>  > -     if (attrs->ia_valid & ATTR_SIZE) {
>  > +     if ((attrs->ia_valid & ATTR_MTIME) || (attrs->ia_valid & ATTR_SIZE)) {
>  >               /*
>  > -                Flush data before changing file size on server. If the
>  > +                Flush data before changing file size or changing the last
>  > +                write time of the file on the server. If the
>  >                  flush returns error, store it to report later and continue.
>  >                  BB: This should be smarter. Why bother flushing pages that
>  >                  will be truncated anyway? Also, should we error out here if
>  > @@ -1435,7 +1434,9 @@ int cifs_setattr(struct dentry *direntry, struct
>  > iattr *attrs)
>  >                       CIFS_I(direntry->d_inode)->write_behind_rc = rc;
>  >                       rc = 0;
>  >               }
>  > +     }
>  >
>  > +     if (attrs->ia_valid & ATTR_SIZE) {
>  >               /* To avoid spurious oplock breaks from server, in the case of
>  >                  inodes that we already have open, avoid doing path based
>  >                  setting of file size if we can do it by handle.
>  >
>  >
>  >
>
>  That should fix Kukks' problem, but still leaves us vulnerable to the
>  other problems. For instance:
>
>  Changing UID/GID or mode could cause later writes to fail.
>
>  What happens if we still have outstanding writes, but we've set
>  ATTR_READONLY on the file on the server?
>
>  What about the atime? Pending writes will clobber it.
>
>  I think the only thing we can really do is flush on every setattr call.
>  The only situation I can see where we don't need to flush is possibly
>  where we're *only* changing the ctime, and it's probably not worth it
>  to make that a special case.
>
>  How about this patch instead?
>
>  Signed-off-by: Jeff Layton <jlayton@redhat.com>
>
>
>  diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>  index af42262..8bcbfff 100644
>
> --- a/fs/cifs/inode.c
>  +++ b/fs/cifs/inode.c
>  @@ -1420,22 +1420,20 @@ int cifs_setattr(struct dentry *direntry, struct iattr *attrs)
>
>         }
>         cifsInode = CIFS_I(direntry->d_inode);
>
>  -       /* BB check if we need to refresh inode from server now ? BB */
>  +       /*
>  +          Flush data before changing attributes on server. If the
>
> +          flush returns error, store it to report later and continue.
>  +          BB: This should be smarter. Why bother flushing pages that
>  +          will be truncated anyway? Also, what should we do if the
>  +          the flush returns error?
>  +        */
>  +       rc = filemap_write_and_wait(direntry->d_inode->i_mapping);
>  +       if (rc != 0) {
>  +               CIFS_I(direntry->d_inode)->write_behind_rc = rc;
>
> +               rc = 0;
>  +       }
>
>         if (attrs->ia_valid & ATTR_SIZE) {
>  -               /*
>
> -                  Flush data before changing file size on server. If the
>  -                  flush returns error, store it to report later and continue.
>
> -                  BB: This should be smarter. Why bother flushing pages that
>  -                  will be truncated anyway? Also, should we error out here if
>  -                  the flush returns error?
>  -                */
>  -               rc = filemap_write_and_wait(direntry->d_inode->i_mapping);
>  -               if (rc != 0) {
>  -                       CIFS_I(direntry->d_inode)->write_behind_rc = rc;
>  -                       rc = 0;
>  -               }
>
>
> -
>                 /* To avoid spurious oplock breaks from server, in the case of
>                    inodes that we already have open, avoid doing path based
>                    setting of file size if we can do it by handle.
>



-- 
Thanks,

Steve

Re: set last write time = fsync ?

[Jeff Layton]

On Fri, 14 Mar 2008 11:16:41 -0500
"Steve French" <smfrench@gmail.com> wrote:

> I don't worry about flushing atime (anyone crazy enough to do that
> would pay a huge performance penalty).
> Access is usually checked on open right ... so once a file is open
> even if the file becomes read-only, the writes, even cached writes
> continue.
> 

Ahh, you're correct. I've been doing a lot of NFS work lately and was
thinking stateless... :-)

That patch should be OK then, though I think if someone is purposefully
setting the atime we should take care not to clobber it. We're not
going to be going through this codepath on every atime update, are we?
Just on utimes() type calls, correct? If so, doing a flush on atime
updates might be reasonable as well...

-- 
Jeff Layton <jlayton@redhat.com>

Re: set last write time = fsync ?

[Steve French]

On Fri, Mar 14, 2008 at 11:55 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Fri, 14 Mar 2008 11:16:41 -0500
>
> "Steve French" <smfrench@gmail.com> wrote:
>
>
> > I don't worry about flushing atime (anyone crazy enough to do that
>  > would pay a huge performance penalty).
>  > Access is usually checked on open right ... so once a file is open
>  > even if the file becomes read-only, the writes, even cached writes
>  > continue.
>  >
>
>  Ahh, you're correct. I've been doing a lot of NFS work lately and was
>  thinking stateless... :-)
>
>  That patch should be OK then, though I think if someone is purposefully
>  setting the atime we should take care not to clobber it. We're not
>  going to be going through this codepath on every atime update, are we?
>  Just on utimes() type calls, correct? If so, doing a flush on atime
>  updates might be reasonable as well...
>
> Jeff Layton <jlayton@redhat.com>
>

I don't think we need to flush before setting (just) atime.
If the problem with timestamps is delayed writes getting written out
on close ... won't close update the atime anyway?


-- 
Thanks,

Steve

Re: set last write time = fsync ?

[Jeff Layton]

On Fri, 14 Mar 2008 14:19:06 -0500
"Steve French" <smfrench@gmail.com> wrote:

> On Fri, Mar 14, 2008 at 11:55 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Fri, 14 Mar 2008 11:16:41 -0500
> >
> > "Steve French" <smfrench@gmail.com> wrote:
> >
> >
> > > I don't worry about flushing atime (anyone crazy enough to do that
> >  > would pay a huge performance penalty).
> >  > Access is usually checked on open right ... so once a file is open
> >  > even if the file becomes read-only, the writes, even cached writes
> >  > continue.
> >  >
> >
> >  Ahh, you're correct. I've been doing a lot of NFS work lately and was
> >  thinking stateless... :-)
> >
> >  That patch should be OK then, though I think if someone is purposefully
> >  setting the atime we should take care not to clobber it. We're not
> >  going to be going through this codepath on every atime update, are we?
> >  Just on utimes() type calls, correct? If so, doing a flush on atime
> >  updates might be reasonable as well...
> >
> > Jeff Layton <jlayton@redhat.com>
> >
> 
> I don't think we need to flush before setting (just) atime.
> If the problem with timestamps is delayed writes getting written out
> on close ... won't close update the atime anyway?
> 
> 
Consider that an app like tar might do something like this:

open()
write()
write()
write()
close()
utimes()

The app would likely set the mtime too, but I'm not sure we should make
that assumption. The question is -- should we allow that utimes() call
to be clobbered by writes lingering around after the close() returns?

IMO, we shouldn't. The situation in this case is really the same for
atime and mtime. Both would be clobbered by a delayed write, so we
should really treat them the same...

-- 
Jeff Layton <jlayton@redhat.com>

Re: set last write time = fsync ?

[Steve French]

On Fri, Mar 14, 2008 at 3:35 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Fri, 14 Mar 2008 14:19:06 -0500
>
> "Steve French" <smfrench@gmail.com> wrote:
>
>  > On Fri, Mar 14, 2008 at 11:55 AM, Jeff Layton <jlayton@redhat.com> wrote:
>  > > On Fri, 14 Mar 2008 11:16:41 -0500
>  > >
>  > > "Steve French" <smfrench@gmail.com> wrote:
>  > >
>  > >
>  > > > I don't worry about flushing atime (anyone crazy enough to do that
>  > >  > would pay a huge performance penalty).
>  > >  > Access is usually checked on open right ... so once a file is open
>  > >  > even if the file becomes read-only, the writes, even cached writes
>  > >  > continue.
>  > >  >
>  > >
>  > >  Ahh, you're correct. I've been doing a lot of NFS work lately and was
>  > >  thinking stateless... :-)
>  > >
>  > >  That patch should be OK then, though I think if someone is purposefully
>  > >  setting the atime we should take care not to clobber it. We're not
>  > >  going to be going through this codepath on every atime update, are we?
>  > >  Just on utimes() type calls, correct? If so, doing a flush on atime
>  > >  updates might be reasonable as well...
>  > >
>  > > Jeff Layton <jlayton@redhat.com>
>  > >
>  >
>  > I don't think we need to flush before setting (just) atime.
>  > If the problem with timestamps is delayed writes getting written out
>  > on close ... won't close update the atime anyway?
>  >
>  >
>  Consider that an app like tar might do something like this:
>
>  open()
>  write()
>  write()
>  write()
>  close()
>  utimes()
>
>  The app would likely set the mtime too, but I'm not sure we should make
>  that assumption. The question is -- should we allow that utimes() call
>  to be clobbered by writes lingering around after the close() returns?

There can't be writes lingering around after the close ... filp_close does
a flush before calling fput.


-- 
Thanks,

Steve

Re: set last write time = fsync ?

[Jeff Layton]

On Fri, 14 Mar 2008 16:38:27 -0500
"Steve French" <smfrench@gmail.com> wrote:

> On Fri, Mar 14, 2008 at 3:35 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Fri, 14 Mar 2008 14:19:06 -0500
> >
> > "Steve French" <smfrench@gmail.com> wrote:
> >
> >  > On Fri, Mar 14, 2008 at 11:55 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >  > > On Fri, 14 Mar 2008 11:16:41 -0500
> >  > >
> >  > > "Steve French" <smfrench@gmail.com> wrote:
> >  > >
> >  > >
> >  > > > I don't worry about flushing atime (anyone crazy enough to do that
> >  > >  > would pay a huge performance penalty).
> >  > >  > Access is usually checked on open right ... so once a file is open
> >  > >  > even if the file becomes read-only, the writes, even cached writes
> >  > >  > continue.
> >  > >  >
> >  > >
> >  > >  Ahh, you're correct. I've been doing a lot of NFS work lately and was
> >  > >  thinking stateless... :-)
> >  > >
> >  > >  That patch should be OK then, though I think if someone is purposefully
> >  > >  setting the atime we should take care not to clobber it. We're not
> >  > >  going to be going through this codepath on every atime update, are we?
> >  > >  Just on utimes() type calls, correct? If so, doing a flush on atime
> >  > >  updates might be reasonable as well...
> >  > >
> >  > > Jeff Layton <jlayton@redhat.com>
> >  > >
> >  >
> >  > I don't think we need to flush before setting (just) atime.
> >  > If the problem with timestamps is delayed writes getting written out
> >  > on close ... won't close update the atime anyway?
> >  >
> >  >
> >  Consider that an app like tar might do something like this:
> >
> >  open()
> >  write()
> >  write()
> >  write()
> >  close()
> >  utimes()
> >
> >  The app would likely set the mtime too, but I'm not sure we should make
> >  that assumption. The question is -- should we allow that utimes() call
> >  to be clobbered by writes lingering around after the close() returns?
> 
> There can't be writes lingering around after the close ... filp_close does
> a flush before calling fput.
> 
> 

Right, but we don't do filemap_fdatawait() on flush so I suppose we're
not guaranteed to actually have all the writes out on the wire before
the close occurs.

IIRC, the current writepages implementation in cifs I think does
effectively wait until all the writes have completed before returning,
so a filemap_fdatawait wouldn't really do make any difference.

Anyway, after looking back over the original problem, I think I'm
convinced that your original patch is OK.

ACK

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: Re: set last write time = fsync ?

[simo]

On Fri, 2008-03-14 at 11:16 -0500, Steve French wrote:
> I don't worry about flushing atime (anyone crazy enough to do that
> would pay a huge performance penalty).
> Access is usually checked on open right ... so once a file is open
> even if the file becomes read-only, the writes, even cached writes
> continue.

Is this true even in the case you loose the connection with the server
and need to re-establish it ?

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo@samba.org>
Senior Software Engineer at Red Hat Inc. <ssorce@redhat.com>

Re: [linux-cifs-client] Re: set last write time = fsync ?

[Steve French]

On Sat, Mar 15, 2008 at 9:51 AM, simo <idra@samba.org> wrote:
>
>  On Fri, 2008-03-14 at 11:16 -0500, Steve French wrote:
>  > Access is usually checked on open right ... so once a file is open
>  > even if the file becomes read-only, the writes, even cached writes
>  > continue.
>
>  Is this true even in the case you loose the connection with the server
>  and need to re-establish it ?
>
>  Simo Sorce
>  Samba Team GPL Compliance Officer <simo@samba.org>
>  Senior Software Engineer at Red Hat Inc. <ssorce@redhat.com>

If you lose the connection with the server, the eventual reconnection could
fail (changed password, deleted user) or reopen could fail (ACL on
the file changed) but there isn't much we can do about that except
aggressively flush write data (for write behind data) and do whole file
caching on the client (for read ahead data)  which hurt performance
even worse - but cifs does have a mount option to write through
(forcedirectio) rather than cache writes if desired.



-- 
Thanks,

Steve