On Thu, 11 Jun 2009 13:52:55 -0400
Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Thu, 28 May 2009 16:33:03 +1000
> NeilBrown <neilb@xxxxxxx> wrote:
>
> > Currently when we right a number to 'threads' in nfsdfs,
> > we take the nfsd_mutex, update the number of threads, then take the
> > mutex again to read the number of threads.
> >
> > Mostly this isn't a big deal. However if we are write '0', and
> > portmap happens to be dead, then we can get unpredictable behaviour.
> > If the nfsd threads all got killed quickly and the last thread is
> > waiting for portmap to respond, then the second time we take the mutex
> > we will block waiting for the last thread.
> > However if the nfsd threads didn't die quite that fast, then there
> > will be no contention when we try to take the mutex again.
> >
> > Unpredictability isn't fun, and waiting for the last thread to exit is
> > pointless, so avoid taking the lock twice.
> > To achieve this, get nfsd_svc return a non-negative number of active
> > threads when not returning a negative error.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
> >
> > fs/nfsd/nfsctl.c | 13 +++++++++----
> > fs/nfsd/nfssvc.c | 2 ++
> > 2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index af16849..1ddd4f9 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -207,10 +207,14 @@ static struct file_operations pool_stats_operations = {
> > static ssize_t write_svc(struct file *file, char *buf, size_t size)
> > {
> > struct nfsctl_svc *data;
> > + int err;
> > if (size < sizeof(*data))
> > return -EINVAL;
> > data = (struct nfsctl_svc*) buf;
> > - return nfsd_svc(data->svc_port, data->svc_nthreads);
> > + err = nfsd_svc(data->svc_port, data->svc_nthreads);
> > + if (err < 0)
> > + return err;
> > + return 0;
> > }
> >
> > /**
> > @@ -692,10 +696,11 @@ static ssize_t write_threads(struct file *file, char *buf, size_t size)
> > if (newthreads < 0)
> > return -EINVAL;
> > rv = nfsd_svc(NFS_PORT, newthreads);
> > - if (rv)
> > + if (rv < 0)
> > return rv;
> > - }
> > - sprintf(buf, "%d\n", nfsd_nrthreads());
> > + } else
> > + rv = nfsd_nrthreads();
> > + sprintf(buf, "%d\n", rv);
> > return strlen(buf);
> > }
> >
> > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> > index cbba4a9..df6c59e 100644
> > --- a/fs/nfsd/nfssvc.c
> > +++ b/fs/nfsd/nfssvc.c
> > @@ -413,6 +413,8 @@ nfsd_svc(unsigned short port, int nrservs)
> > goto failure;
> >
> > error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
> > + if (error == 0)
> > + error = nfsd_serv->sv_nrthreads - 1;
> ^^^^
> Why -1 here? I thought you wanted nfsd_svc to return the number of
> threads?
>
Ahh nm...I see why. sv_nrthreads is artificially high due to the
earlier svc_get. Hate the refcounting with this stuff -- blech...
> Also, looks like Bruce's tree has changed a bit recently and I don't
> think this patch will apply cleanly now. You may need to respin...
>
> > failure:
> > svc_destroy(nfsd_serv); /* Release server */
> > out:
> >
> >
> > --
> > 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
> >
>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
> --
> 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
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
