From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [linux-cifs-client] Re: fsx-linux failing with latest cifs-2.6 git tree Date: Wed, 26 Nov 2008 16:23:32 +0100 Message-ID: <20081126152332.GA23539@wotan.suse.de> References: <1227329229.29807.9.camel@norville.austin.ibm.com> <20081122103916.6d9fc4e6@tleilax.poochiereds.net> <1227385664.10953.4.camel@norville.austin.ibm.com> <20081123065715.029b6ceb@tleilax.poochiereds.net> <524f69650811231832v12a03252ycbf53d9b06f178f8@mail.gmail.com> <20081124061918.7376c1ce@tleilax.poochiereds.net> <524f69650811252004nfc7936bga8aa0d34ef487cd@mail.gmail.com> <20081126071146.1bdcd630@tleilax.poochiereds.net> <20081126130943.GF23649@wotan.suse.de> <20081126100857.63e4b45d@tleilax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Steve French , linux-fsdevel , pbadari@us.ibm.com, "linux-cifs-client@lists.samba.org" To: Jeff Layton Return-path: Received: from mail.suse.de ([195.135.220.2]:35485 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751960AbYKZPXf (ORCPT ); Wed, 26 Nov 2008 10:23:35 -0500 Content-Disposition: inline In-Reply-To: <20081126100857.63e4b45d@tleilax.poochiereds.net> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Nov 26, 2008 at 10:08:57AM -0500, Jeff Layton wrote: > On Wed, 26 Nov 2008 14:09:43 +0100 > Nick Piggin 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...