On Wed, 26 Nov 2008 14:09:43 +0100 Nick Piggin <npiggin@xxxxxxx> 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@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
