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