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

[Nick Piggin]

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 at suse.de> 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 at redhat.com>
> 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 at redhat.com>
> ---
>  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