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
