On Mon, 1 Dec 2008 09:44:35 +0100
Nick Piggin <npiggin@suse.de> wrote:

quoted text
> 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...?
> 

But a concurrent write and read is, right?

Suppose we do a successful cifs_write_begin and set PageChecked. Another
thread then incurs a page fault and does a readpage before we copy the
data to the page. Won't we then call write_end with both PageChecked and
PageUptodate set?

That write will be fine, of course. PageChecked is still true though,
and I think that sets up the problem I was describing...

-- 
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html