[linux-cifs-client] Re: fsx-linux failing with latest cifs-2.6 git tree

[Jeff Layton]

On Sun, 30 Nov 2008 15:44:21 -0600
"Steve French" <smfrench at gmail.com> wrote:

> On Fri, Nov 28, 2008 at 6:18 AM, Jeff Layton <jlayton at redhat.com> 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 <jlayton at redhat.com>
> > ---
> >  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.
> 

I think it actually is a problem. Suppose PageChecked is never cleared
like you say, we flush the page and then do a partial page write again.
We do a readpage this time and it fails, but the copy of data to the
page works. Now we hit cifs_write_end and PageChecked is set, but
the unwritten parts of the page actually aren't up to date. Data
corruption ensues...

I agree that we should drop that patch. We might be able to make
cifs_write_end more efficient, but we'll need to be more careful
with PageChecked.

-- 
Jeff Layton <jlayton at redhat.com>