Re: [linux-cifs-client] Re: fsx-linux failing with latest cifs-2.6 git tree

[Nick Piggin <npiggin@suse.de>]

On Wed, Nov 26, 2008 at 10:08:57AM -0500, Jeff Layton wrote:
> On Wed, 26 Nov 2008 14:09:43 +0100
> Nick Piggin <npiggin@suse.de> wrote:
> 
> 
> > As to why it can happen, because copy_from_user could take a page fault
> > on the app's source address (eg. to write(2)).
> 
> Yep, I figured this out after stumbling across the LWN article.
> 
> > You _might_ be OK there, but it's not a great idea to SetPageUptodate
> > first ;) Aside from the problem of the short-copy, SetPageUptodate
> > actually has a memory barrier in it to ensure the data stored into the
> > page to bring it uptodate is actually visible before the PageUptodate
> > flag is. Again, if you are doing DMAs rather than cache coherent stores
> > to initialise the page, maybe you can get away without that barrier...
> > But it's just bad practice.
> > 
> 
> Gotcha. I think the current patch takes care of this (we're using
> PageChecked to indicate that the uninitialized parts of the page were
> written to).
> 
> The problem I suppose is that we could end up getting a short write in
> write_end. I guess this means that we need to modify the patch a bit
> further and only set PageUptodate in write_end if copied == len.
> 
> > > Given that I now understand what AOP_FLAG_UNINTERRUPTIBLE is supposed
> > > to do, this patch is probably what we need. Running tests on it now.
> > 
> > That seems pretty reasonable, although keep in mind that
> > AOP_FLAG_UNINTERRUPTIBLE is not going to be the common case (unless
> > you're running loop or nfsd or something on the filesystem).
> > 
> > It would be really nice to figure out a way to avoid the reads in
> > the interruptible case as well.
> 
> True. For now though I think we need to start with slow and safe and see
> if we can optimize it further later...

Yes definitely. Thanks for cleaning up my mess!

 
> > I can't remember the CIFS code very well, but in several of the new
> > aops conversions I did, I added something like a BUG_ON(!PageUptodate())
> > in the write_end methods to ensure I wasn't missing some key part of
> > the logic. It's entirely possible that cifs is almost ready to handle
> > a !uptodate page in write_end...
> 
> Well, CIFS is "special". Rather than just updating the pagecache, we
> can fall back to doing a sync write instead. So I don't think we want
> to BUG if the page isn't up to date. It's not ideal, but I think it's a
> situation we can deal with if necessary.

Yes, that would be better. That sync write fallback is quite clever I
think...