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

[Steve French]

Don't we also have to add the check for
if (mapping->host->clientCanCacheRead) before we decide to skip doing
the cifs_readpage_worker ... otherwise we aren't 100% sure of the file
size on the server (although it is probably right since we just did
revalidate)

so something like

 if (mapping->host->clientCanCacheRead && (page_start >= i_size ||
(offset == 0 && (pos + len) >= i_size)))

On Fri, Nov 21, 2008 at 5:25 PM, Jeff Layton <jlayton at redhat.com> wrote:
> On Fri, 21 Nov 2008 23:02:48 +0000
> Dave Kleikamp <shaggy at linux.vnet.ibm.com> wrote:
>>
>> 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.
>>
>
> Thanks Dave, good catch.
>
> I think this is what we want. Skip the read if the entire page is
> beyond the end of file, or if the write will clobber all of the data
> that would be read in.
>
> 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..0c7ef2a 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 (page_start >= i_size || (offset == 0 && (pos + len) >= i_size)) {
> +               /*
> +                * 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 = {
> --
> 1.5.5.1
>
>



-- 
Thanks,

Steve