[linux-cifs-client] Re: [PATCH 4/4] cifs: flush data on any setattr

[Jeff Layton]

On Thu, 26 Mar 2009 21:52:51 -0500
Steve French <smfrench at gmail.com> wrote:

> On Thu, Mar 26, 2009 at 12:35 PM, Jeff Layton <jlayton at redhat.com> wrote:
> > We already flush all the dirty pages for an inode before doing
> > ATTR_SIZE and ATTR_MTIME changes. There's another problem though -- if
> > we change the mode so that the file becomes read-only then we may not
> > be able to write data to it after a reconnect.
> >
> > Fix this by just going back to flushing all the dirty data on any
> > setattr call.
> > +        * 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?
> > +        */
> 
> Merged the patch into cifs-2.6.git, but Jeff brings up an interesting
> point about what really happens when we are shrinking the file size
> (truncate) here ... is it possible that the vfs will flush pages that
> are going to be thrown away
> 

I think so. If I follow the code correctly, cifs_set_file_size does:

flush the data (filemap_write_and_wait)
set the file size on the server (CIFSSMBSetFileSize, CIFSSMBSetEOF, ...) 
truncate the inode in memory (cifs_vmtruncate)

Perhaps it makes sense to reorder those operations, but that might make
it tough to handle errors. Another (better?) option would be to only
flush the data that's within the new size of the file.

A trivial optimization might be to cheat here and only skip the flushing
if the file is going to be truncated to 0 length (that's probably the
most common case anyway).

Now that I look at this though, I have to wonder...does the existing
code have a race? Is it possible for some writes to occur after we set
the file size on the server but before we truncate the pages off of the
inode?

-- 
Jeff Layton <jlayton at redhat.com>