On Fri, 06 Jun 2008 09:35:51 +1000
Greg Banks <gnb-cP1dWloDopni96+mSzHFpQC/[email protected]> wrote:
> J. Bruce Fields wrote:
> > On Thu, Jun 05, 2008 at 10:47:37AM +1000, Greg Banks wrote:
> >
> >> I notice there's no change to write_leasetime(). That calls
> >> nfs4_reset_lease(), which seems to want to only be called when nfsd is
> >> not started (according to my reading of the comment preceding the
> >> function), and which uses the BKL to protect the variable
> >> user_lease_time. Perhaps that path should be changed to use the new
> >> nfsd_mutex also?
> >>
> >
> > write_leasetime() just results in setting the user_lease_time, which is
> > copied (once, on server startup) to lease_time, which is what is
> > actually used by the running server.
> Yes, understood.
> > So I don't think there's a race
> > here (and the existing BKL use in write_leasetime() isn't really
> > needed).
> >
> The race is pretty harmless. The only read of user_lease_time happens
> during the critical section currently guarded by the BKL in nfsd_svc();
> the only write of the variable is not guarded by that lock. If you call
> write_leasetime() many times with different values during nfsd startup,
> the actual value used is not predictable and you can't tell from
> userspace whether your write() succeeded in changing the behaviour of
> the server or not. Also, you can do write_leasetime() after nfsd
> startup without useful effect; other parameters which can't be changed
> from /proc after the service is running will fail the write() with EBUSY.
>
The race there is pretty harmless, but fixing it sounds like it'll be low
impact. I don't expect that the nfsd_mutex will be highly contended,
so adding some locking around these interfaces really shouldn't harm
anything.
Also, as you point out, many of these interfaces should return -EBUSY if
nfsd is up (to be consistent with how write_versions behaves), and we'll
need proper locking to do that.
I've got a new patchset that should hopefully address these problems and
some of your other comments that I'll post in a little while...
Cheers,
--
Jeff Layton <[email protected]>