Re: [PATCH RFC] nfs: ensure cached data is correct before using delegation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 17, 2014 at 5:04 PM, Scott Mayhew <smayhew@xxxxxxxxxx> wrote:
>
> No, I don't think it's right to ignore NFS_INO_INVALID_DATA, and
> originally I was testing a fix similar to this:
>
> ---8<---
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 3ee5af4..98ff061 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -934,12 +934,14 @@ static bool nfs_write_pageuptodate(struct page *page, struct inode *inode)
>
>         if (nfs_have_delegated_attributes(inode))
>                 goto out;
> -       if (nfsi->cache_validity & (NFS_INO_INVALID_DATA|NFS_INO_REVAL_PAGECACHE))
> +       if (nfsi->cache_validity & NFS_INO_REVAL_PAGECACHE)
>                 return false;
>         smp_rmb();
>         if (test_bit(NFS_INO_INVALIDATING, &nfsi->flags))
>                 return false;
>  out:
> +       if (nfsi->cache_validity & NFS_INO_INVALID_DATA)
> +               return false;
>         return PageUptodate(page) != 0;
>  }
> ---8<---
>
>
> However,
>
> 1) it wasn't really keeping with the spirit of commit 8d197a56 (NFS:
> Always trust the PageUptodate flag when we have a delegation), and
>
> 2) one of my test programs (used to test commit c7559663 (NFS: Allow
> nfs_updatepage to extend a write under additional circumstances)))
> started performing poorly again, doing tons of sub page-sized writes
> intead of a handful of wsize'd writes.
>
> I did some more digging and I think I see 2 areas that could be
> improved.
>
> The first would be to clear NFS_INO_INVALID_DATA if we've just
> truncated the inode to 0 bytes -- after all, if we've just unmapped
> all the pages from the inode's address space then isn't our data
> consisitent?:

Hi Scott,

What say we rather just don't set/clear NFS_INO_INVALID_DATA if
mapping->nrpages == 0? That should catch the above case, plus a couple
of other potential false positives.

> The second thing I noticed is that we're constantly invalidating our
> cache due to the change attribute changing on the server.  But if we
> have a write delegation then the change attribute changing must be the
> result of *our* changes, in which case we should be able to just silently
> update the change attribute on our side without invalidating our caches:

Ack. That's a bug in the current code.

Cheers
  Trond


-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux