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

[Steve French]

Looks fine to me, you can add in the acked-by ... if Dave or Nick
agree and it tests out ok then I will push to cifs-2.6.git within next
day or so.

On Sun, Nov 23, 2008 at 5:57 AM, Jeff Layton <jlayton at redhat.com> wrote:
> On Sat, 22 Nov 2008 14:27:44 -0600
> Dave Kleikamp <shaggy at linux.vnet.ibm.com> wrote:
>
>> Just took a real quick look, but I have one comment.  I'd leave the page
>> alone if it's already PageUptodate.  I wouldn't bother setting
>> PageChecked and let cifs_write_end leave it alone when its already
>> uptodate.
>
> Ok, it makes sense not to flip page bits unless we need to. How's this
> instead? It also switches the code to use zero_user_segments() instead
> of calling kmap/memset directly.
>
> I've dropped Steve's SoB for now since this has morphed quite a bit from
> his original patch. Steve, feel free to add it back if you approve.
>
> --------------------[snip]--------------------
>
> From da222d5125ec4095f10e9a4c984b6bd6b0cacf18 Mon Sep 17 00:00:00 2001
> From: Jeff Layton <jlayton at redhat.com>
> Date: Sun, 23 Nov 2008 06:54:42 -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 |   68 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index b691b89..9114ea5 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1475,7 +1475,10 @@ 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)) {
> +               SetPageUptodate(page);
> +               ClearPageChecked(page);
> +       } else if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
>                SetPageUptodate(page);
>
>        if (!PageUptodate(page)) {
> @@ -2062,39 +2065,68 @@ 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 inode *inode;
> +       struct page *page;
> +       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;
>
> -               /* might as well read a page, it is fast enough */
> -               rc = cifs_readpage_worker(file, *pagep, &offset);
> +       /*
> +        * optimize away the read when we have an oplock, and we're not
> +        * expecting to use any of the data we'd be reading in. That is, when
> +        * the page lies beyond the EOF, or straddles the EOF and the write
> +        * would cover all of the existing data in the file.
> +        */
> +       inode = page->mapping->host;
> +       if (CIFS_I(inode)->clientCanCacheRead) {
> +               i_size = i_size_read(inode);
> +               if (page_start >= i_size ||
> +                   (offset == 0 && (pos + len) >= i_size)) {
> +                       zero_user_segments(page, 0, offset, offset + len,
> +                                          PAGE_CACHE_SIZE);
> +                       /*
> +                        * PageChecked means that the parts of the page to
> +                        * which we're not writing are considered up to date.
> +                        * Once the write data is copied to the page, it can
> +                        * be set uptodate.
> +                        */
> +                       SetPageChecked(page);
> +                       goto out;
> +               }
> +       }
>
> -               /* we do not need to pass errors back
> -                  e.g. if we do not have read access to the file
> -                  because cifs_write_end will attempt synchronous writes
> -                  -- shaggy */
> +       if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> +               /*
> +                * might as well read a page, it is fast enough. If we get
> +                * an error, we don't need to return it. cifs_write_end will
> +                * do a sync write instead since PG_uptodate isn't set.
> +                */
> +               cifs_readpage_worker(file, page, &page_start);
>        } else {
>                /* we could try using another file handle if there is one -
>                   but how would we lock it to prevent close of that handle
>                   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