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

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Jun 16, 2009 at 10:50:57AM +1000, NeilBrown wrote:
> 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

Whoops--actually, it's the opposite problem: a bugfix patch that went
upstream removed this, and I didn't merge that back into my for-2.6.31
branch.  OK, time to do that, and then this is all much simpler....
Thanks for calling my attention to that!

--b.

> 
> > 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
> 
>