On Wed, Nov 26, 2008 at 11:37:58AM -0500, Jeff Layton wrote:
> On Wed, 26 Nov 2008 16:23:32 +0100
> Nick Piggin <npiggin@xxxxxxx> wrote:
> >
> > > > That seems pretty reasonable, although keep in mind that
> > > > AOP_FLAG_UNINTERRUPTIBLE is not going to be the common case (unless
> > > > you're running loop or nfsd or something on the filesystem).
> > > >
> > > > It would be really nice to figure out a way to avoid the reads in
> > > > the interruptible case as well.
> > >
> >
> > > > I can't remember the CIFS code very well, but in several of the new
> > > > aops conversions I did, I added something like a BUG_ON(!PageUptodate())
> > > > in the write_end methods to ensure I wasn't missing some key part of
> > > > the logic. It's entirely possible that cifs is almost ready to handle
> > > > a !uptodate page in write_end...
> > >
> > > Well, CIFS is "special". Rather than just updating the pagecache, we
> > > can fall back to doing a sync write instead. So I don't think we want
> > > to BUG if the page isn't up to date. It's not ideal, but I think it's a
> > > situation we can deal with if necessary.
> >
> > Yes, that would be better. That sync write fallback is quite clever I
> > think...
> >
>
> Since we're able to do the sync write fallback here, I think we can
> probably not bother with checking for AOP_FLAG_UNINTERRUPTIBLE at all.
> As long as we only set the page uptodate in write_end in the case that
> the write isn't short, then we'll be ok I think even when it's
> interruptible.
Yes, that would make sense. The AOP_FLAG_UNINTERRUPTIBLE is really just
a hint in case the filesystem can avoid some really expensive operation.
In reality, ~AOP_FLAG_UNINTERRUPTIBLE is going to be the most common
case for 99% of users, so having any tricky optimisations for it is
probably not worth the hassle.
> This does make the assumption that the interrupted case is somewhat
> rare...
It should be somewhat rare. The kernel prefaults the user memory before
it attempts to copy. For regular write(2), this means a short write is
almost never going to happen. For writev(2), we only fault in the first
iovec, so short writes could be much more common -- if this ever becomes
a problem then we could easily look at prefetching more than one iov.
> Thoughts on this patch?
Looks pretty good.
> ------------------[snip]-------------------
>
> From a8e5ea4d9859f590b5c04954e8e2a13023f690f5 Mon Sep 17 00:00:00 2001
> From: Jeff Layton <jlayton@xxxxxxxxxx>
> Date: Wed, 26 Nov 2008 11:32:09 -0500
> Subject: [PATCH] cifs: fix regression in cifs_write_begin/cifs_write_end
>
> The conversion to write_begin/write_end interfaces had a bug where we
> were passing a bad parameter to cifs_readpage_worker. Rather than
> passing the page offset of the start of the write, we needed to pass the
> offset of the beginning of the page. This was reliably showing up as
> data corruption in the fsx-linux test from LTP.
>
> It also became evident that this code was occasionally doing unnecessary
> read calls. Optimize those away by using the PG_checked flag to indicate
> that the unwritten part of the page has been initialized.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/cifs/file.c | 79 +++++++++++++++++++++++++++++++++++++++++---------------
> 1 files changed, 58 insertions(+), 21 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index b691b89..49d4a97 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1475,7 +1475,11 @@ 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 (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
> + if (PageChecked(page)) {
> + if (copied == len)
> + SetPageUptodate(page);
> + ClearPageChecked(page);
> + } else if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
> SetPageUptodate(page);
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.
Thanks,
Nick
--
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
