On Mon, 1 Dec 2008 09:44:35 +0100
Nick Piggin <npiggin@xxxxxxx> wrote:
> 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@xxxxxxxxx> wrote:
> >
> > > On Fri, Nov 28, 2008 at 6:18 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
