Re: NFS client latencies

Trond Myklebust
2005-03-31T11:58:46Z

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

to den 31.03.2005 Klokka 09:30 (+0200) skreiv Ingo Molnar:

> > And presumably that list-based code rarely hits the worst-case, else
> > it would have been changed by now.
>
> that was my other point in a previous mail: most write benchmarks do
> continuous append writes, and CPU overhead easily gets lost in
> network
> latency.

Well. Most _good_ write benchmarks go through a variety of scenarios.

The problem with the radix-tree is that in most cases you are swapping a
linear scan through only the dirty pages (most of which you want to try
to add to the list anyway) for a linear scan through the entire list of
pages.
Damn! I've forgotten an extra _obvious_ optimization that might help
here... See the attached patch.

> Also, considering that a good portion of the NFS client's code is still
> running under the BKL one would assume if the BKL hurts performance it
> would have been changed by now? ;-)

Funny you should mention that... Chuck happens to have spent the last
fortnight working on removing our BKL-addiction. ;-)

Cheers,
Trond

--
Trond Myklebust <trond.myklebust@fys.uio.no>

[-- Attachment #2: Type: text/plain, Size: 1722 bytes --]

fs/nfs/write.c | 28 +++++++++++++++++-----------
1 files changed, 17 insertions(+), 11 deletions(-)

Index: linux-2.6.12-rc1/fs/nfs/write.c
===================================================================
--- linux-2.6.12-rc1.orig/fs/nfs/write.c
+++ linux-2.6.12-rc1/fs/nfs/write.c
@@ -539,12 +539,15 @@ static int
nfs_scan_dirty(struct inode *inode, struct list_head *dst, unsigned long idx_start, unsigned int npages)
{
struct nfs_inode *nfsi = NFS_I(inode);
- int res;
- res = nfs_scan_lock_dirty(nfsi, dst, idx_start, npages);
- nfsi->ndirty -= res;
- sub_page_state(nr_dirty,res);
- if ((nfsi->ndirty == 0) != list_empty(&nfsi->dirty))
- printk(KERN_ERR "NFS: desynchronized value of nfs_i.ndirty.\n");
+ int res = 0;
+
+ if (nfsi->ndirty != 0) {
+ res = nfs_scan_lock_dirty(nfsi, dst, idx_start, npages);
+ nfsi->ndirty -= res;
+ sub_page_state(nr_dirty,res);
+ if ((nfsi->ndirty == 0) != list_empty(&nfsi->dirty))
+ printk(KERN_ERR "NFS: desynchronized value of nfs_i.ndirty.\n");
+ }
return res;
}

@@ -563,11 +566,14 @@ static int
nfs_scan_commit(struct inode *inode, struct list_head *dst, unsigned long idx_start, unsigned int npages)
{
struct nfs_inode *nfsi = NFS_I(inode);
- int res;
- res = nfs_scan_list(&nfsi->commit, dst, idx_start, npages);
- nfsi->ncommit -= res;
- if ((nfsi->ncommit == 0) != list_empty(&nfsi->commit))
- printk(KERN_ERR "NFS: desynchronized value of nfs_i.ncommit.\n");
+ int res;
+
+ if (nfsi->ncommit != 0) {
+ res = nfs_scan_list(&nfsi->commit, dst, idx_start, npages);
+ nfsi->ncommit -= res;
+ if ((nfsi->ncommit == 0) != list_empty(&nfsi->commit))
+ printk(KERN_ERR "NFS: desynchronized value of nfs_i.ncommit.\n");
+ }
return res;
}
#endif