From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [linux-cifs-client] Re: fsx-linux failing with latest cifs-2.6 git tree Date: Wed, 26 Nov 2008 10:08:57 -0500 Message-ID: <20081126100857.63e4b45d@tleilax.poochiereds.net> References: <20081121205151.1e3a09fe@tleilax.poochiereds.net> <524f69650811211802x74c0cadaided4c09f6c9e790e@mail.gmail.com> <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> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Steve French , linux-fsdevel , pbadari@us.ibm.com, "linux-cifs-client@lists.samba.org" To: Nick Piggin Return-path: Received: from mx2.redhat.com ([66.187.237.31]:54986 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753404AbYKZPJG (ORCPT ); Wed, 26 Nov 2008 10:09:06 -0500 In-Reply-To: <20081126130943.GF23649@wotan.suse.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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... > 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