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

[Jeff Layton]

On Thu, 26 Mar 2009 15:32:44 -0700 (PDT)
Linus Torvalds <torvalds at linux-foundation.org> wrote:

> 
> 
> On Thu, 26 Mar 2009, Jeff Layton wrote:
> > On Thu, 26 Mar 2009 15:10:49 -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.
> > > 
> > > Do we have any idea of how badly this would hurt performance?
> > > 
> > > Changing the mode or owner may be rare enough ... but I don't have any
> > > data to prove that.
> > > 
> > > Changing the file's mode on the fly while writing could cause problems
> > > for file systems other than cifs too, not sure we want to hurt
> > > performance for a corner case of a bad application.  I don't this
> > > posix specifies that chmod does an implied fsync
> > > 
> > 
> > (cc'ing Linus since he brought this to our attention and might have
> > input)
> > 
> > No, posix doesn't specify that chmod does an implied fsync, but if
> > that's what's required to ensure that we don't break writeback across a
> > reconnect then I think it's the right thing to do. We can try to
> > optimize it away on some setattr cases, but they're probably not worth
> > it.
> 
> Yes, I don't care about anything but obvious correctness.
> 
> Right now, CIFS is _broken_. Real programs have seen real breakage. I 
> don't see why Steve argues against the patch on some specious grounds of 
> it "not being necessary", when it clearly is, and there clearly is a CIFS 
> bug there.
> 
> Performance is totally irrelevant. It doesn't matter at all, when the 
> alternative is "lost data".
> 

I suspect that there is another bug for the reporter that's consistently
causing a reconnect of the socket after the fchmod. That problem could
even be server-side (hard to know without a wire capture and/or some
debug logs).

The current code should work just fine as long as we don't incur a
reconnect and have to reclaim the open files between the fchmod and the
writes. The problem is that we can never 100% predict when we'll need
to reconnect, so we have to be very careful about how we handle changes
that might make us unable to reclaim the open.

On the performance side, we can always go back and optimize the flush
out of some of setattr calls when we *know* that they won't cause a
problem, but the list of those cases is starting to look pretty small.

-- 
Jeff Layton <jlayton at redhat.com>