From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Steve French" Subject: Re: [linux-cifs-client] Re: fsx-linux failing with latest cifs-2.6 git tree Date: Sun, 30 Nov 2008 15:44:21 -0600 Message-ID: <524f69650811301344n4316bbcam86530e11ef6ca620@mail.gmail.com> References: <1227385664.10953.4.camel@norville.austin.ibm.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> <20081126152332.GA23539@wotan.suse.de> <20081126113758.66d27c67@tleilax.poochiereds.net> <20081127083330.GB28285@wotan.suse.de> <20081128071845.020907f1@tupile.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: "Nick Piggin" , linux-fsdevel , pbadari@us.ibm.com, "linux-cifs-client@lists.samba.org" To: "Jeff Layton" Return-path: Received: from ey-out-2122.google.com ([74.125.78.25]:18164 "EHLO ey-out-2122.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753083AbYK3VoX (ORCPT ); Sun, 30 Nov 2008 16:44:23 -0500 Received: by ey-out-2122.google.com with SMTP id 6so921803eyi.37 for ; Sun, 30 Nov 2008 13:44:21 -0800 (PST) In-Reply-To: <20081128071845.020907f1@tupile.poochiereds.net> Content-Disposition: inline Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, Nov 28, 2008 at 6:18 AM, Jeff Layton wrote: >> One minor thing -- you could do the !PageUptodate check first? If the >> page is already uptodate, then everything is much simpler I think? (and >> PageChecked should not be set). >> >> if (!PageUptodate(page)) { >> if (PageChecked(page)) { >> if (copied == len) >> SetPageUptodate(page); >> ClearPageChecked(page); >> } else if (copied == PAGE_CACHE_SIZE) >> SetPageUptodate(page); >> } >> >> I don't know if you think that's better or not, but I really like to >> make it clear that this is the !PageUptodate logic, and we never try >> to SetPageUptodate on an already uptodate page. >> >> But I guess it is just a matter of style. So go with whatever you like >> best. > --------------[snip]--------------- > Subject: [PATCH] cifs: clean up conditionals in cifs_write_end > > Make it clear that the conditionals at the start of cifs_write_end are > just for the situation when the page is not uptodate. > > Signed-off-by: Jeff Layton > --- > fs/cifs/file.c | 12 +++++++----- > 1 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index f0a81e6..202a20f 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -1475,12 +1475,14 @@ static int cifs_write_end(struct file *file, struct address_space *mapping, > cFYI(1, ("write_end for page %p from pos %lld with %d bytes", > page, pos, copied)); > > - if (PageChecked(page)) { > - if (copied == len) > + if (!PageUptodate(page)) { > + if (PageChecked(page)) { > + if (copied == len) > + SetPageUptodate(page); > + ClearPageChecked(page); > + } else if (copied == PAGE_CACHE_SIZE) > SetPageUptodate(page); > - ClearPageChecked(page); > - } else if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE) > - SetPageUptodate(page); > + } > > if (!PageUptodate(page)) { > char *page_data; Jeff and I just talked about his patch above, and decided not to make his minor change above. Moving PageUptodate check earlier would complicate things in one way ... if PageChecked were ever set at the same time as PageUptodate then PageChecked would stay set. That is probably not an issue but that is clearer with the original. -- Thanks, Steve