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

[Nick Piggin <npiggin@suse.de>]

On Sun, Nov 30, 2008 at 05:17:34PM -0500, Jeff Layton wrote:
> On Sun, 30 Nov 2008 15:44:21 -0600
> "Steve French" <smfrench@gmail.com> wrote:
> 
> > On Fri, Nov 28, 2008 at 6:18 AM, Jeff Layton <jlayton@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@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.

Oh? I admittedly haven't looked at the source code after applying
your latest patch, but I thought it should not be possible to have
a leaking PageChecked. The page is under the page lock the whole
time, so a concurrent write should not be an issue...?