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

Jeff Layton jlayton at redhat.com
Wed Nov 26 15:08:57 GMT 2008


On Wed, 26 Nov 2008 14:09:43 +0100
Nick Piggin <npiggin at 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...

> 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.

Steve, I think I may have (at least) one more respin coming your way...
-- 
Jeff Layton <jlayton at redhat.com>


More information about the linux-cifs-client mailing list