Re: Link performance over NFS degraded in RHEL5. -- was : Read/Write NFS I/O performance degraded by FLUSH_STABLE page flushing

NeilBrown
2009-06-16T00:51:06Z

On Tue, June 16, 2009 10:33 am, J. Bruce Fields wrote:
> On Tue, Jun 16, 2009 at 10:21:50AM +1000, NeilBrown wrote:
>> On Tue, June 16, 2009 9:08 am, J. Bruce Fields wrote:
>>
>> > + if (host_err >= 0 && stable)
>> > + wait_for_concurrent_writes(file, use_wgather, &host_err);
>> >
>>
>> Surely you want this to be:
>>
>> if (host_err >= 0 && stable && use_wgather)
>> host_err = wait_for_concurrent_writes(file);
>> as
>> - this is more readable
>> - setting last_ino and last_dev is pointless when !use_wgather
>
> Yep, thanks.
>
>> - we aren't interested in differentiation between non-negative values
>> of
>> host_err.
>
> Unfortunately, just below:
>
> if (host_err >= 0) {
> err = 0;
> *cnt = host_err;
> } else
> err = nfserrno(host_err);
>

Ahh.... that must be in code you haven't pushed out yet.
I don't see it in mainline or git.linux-nfs.org

> We could save that count earlier, e.g.:
>
> @@ -1014,6 +1013,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh
> *fhp,
> int host_err;
> int stable = *stablep;
> int use_wgather;
> + int bytes;
>
> #ifdef MSNFS
> err = nfserr_perm;
> @@ -1056,6 +1056,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh
> *fhp,
> set_fs(oldfs);
> if (host_err >= 0) {
> nfsdstats.io_write += host_err;
> + bytes = host_err;
> fsnotify_modify(file->f_path.dentry);

Or even

if (host_err >= 0) {
bytes = host_err;
nfsdstats.io_write += bytes
...

And if you did that in whatever patch move the assignment to
*cnt to the bottom of the function, it might be even more readable!

Thanks,
NeilBrown