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

[Dave Kleikamp]

On Fri, 2008-11-21 at 17:50 -0500, Jeff Layton wrote:
> On Fri, 21 Nov 2008 14:38:18 -0600
> "Steve French" <smfrench at gmail.com> wrote:
> 
> > Fix attached.
> > 
> > Shaggy/Jeff/Nick etc. do you want to review/ack it since it is late in the rc?
> > 
> 
> Talking with Steve on IRC, we thought it might be better to optimize
> away the read when possible. I think this patch should do it. We skip
> the read if the write starts past the current end of the file, or if
> the offset into the page of the beginning of the write is 0 and we're
> writing past the current end of the file. In those situations we just
> zero out the rest of the page.
> 
> Combined patch inlined below. I also took the liberty of adding a page
> pointer to make the code look a little cleaner.
> 
> Thoughts?
> 
> Signed-off-by: Jeff Layton <jlayton at redhat.com>
> Signed-off-by: Steve French <smfrench at us.ibm.com>
> ---
>  fs/cifs/file.c |   42 ++++++++++++++++++++++++++++++------------
>  1 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index b691b89..1d252b8 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2062,26 +2062,43 @@ static int cifs_write_begin(struct file *file, struct address_space *mapping,
>  {
>  	pgoff_t index = pos >> PAGE_CACHE_SHIFT;
>  	loff_t offset = pos & (PAGE_CACHE_SIZE - 1);
> +	loff_t page_start = pos & PAGE_MASK;
> +	loff_t i_size;
> +	struct page *page;
> +	char *buf;
> +	int rc = 0;
> 
>  	cFYI(1, ("write_begin from %lld len %d", (long long)pos, len));
> 
> -	*pagep = __grab_cache_page(mapping, index);
> -	if (!*pagep)
> -		return -ENOMEM;
> +	page = __grab_cache_page(mapping, index);
> +	if (!page) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> 
> -	if (PageUptodate(*pagep))
> -		return 0;
> +	if (PageUptodate(page))
> +		goto out;
> 
>  	/* If we are writing a full page it will be up to date,
>  	   no need to read from the server */
>  	if (len == PAGE_CACHE_SIZE && flags & AOP_FLAG_UNINTERRUPTIBLE)
> -		return 0;
> -
> -	if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> -		int rc;
> +		goto out;
> 
> +	i_size = i_size_read(page->mapping->host);
> +	if (pos >= i_size || ((offset == 0) && ((pos + len) >= i_size))) {

I don't think the (pos >= i_size) part of this test is correct.  We
would still need to preserve the data between the start of the page and
i_size.

The second part of the test makes sense.

> +		/*
> +		 * optimize away the read when the unwritten part of the page
> +		 * isn't expected to have any existing file data. Just zero
> +		 * out the unused parts.
> +		 */
> +		buf = kmap(page);
> +		memset(buf, 0, offset);
> +		memset(buf + offset + len, 0, PAGE_CACHE_SIZE - (offset + len));
> +		kunmap(page);
> +		SetPageUptodate(page);
> +	} else if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
>  		/* might as well read a page, it is fast enough */
> -		rc = cifs_readpage_worker(file, *pagep, &offset);
> +		cifs_readpage_worker(file, page, &page_start);
> 
>  		/* we do not need to pass errors back
>  		   e.g. if we do not have read access to the file
> @@ -2093,8 +2110,9 @@ static int cifs_write_begin(struct file *file, struct address_space *mapping,
>  		   racing with this read? In any case
>  		   this will be written out by write_end so is fine */
>  	}
> -
> -	return 0;
> +out:
> +	*pagep = page;
> +	return rc;
>  }
> 
>  const struct address_space_operations cifs_addr_ops = {
-- 
David Kleikamp
IBM Linux Technology Center