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.
--
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
