On Fri, 06 Jun 2008 09:35:51 +1000 Greg Banks <gnb@xxxxxxxxxxxxxxxxx> 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 <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
