On Sat, 26 Oct 2024, Jeff Layton wrote: > On Fri, 2024-10-25 at 13:31 +0000, Trond Myklebust wrote: > > On Fri, 2024-10-25 at 12:52 +0000, Chuck Lever III wrote: > > > > > > > > > > On Oct 24, 2024, at 11:00 PM, NeilBrown <neilb@xxxxxxx> wrote: > > > > > > > > I'm wondering about the request for a garbage-collected nfsd_file > > > > though. For NFSv3 that makes sense. For NFSv4 we would expect the > > > > file > > > > to already be open as a non-garbage-collected nfsd_file and opening > > > > it > > > > again seems wasteful. That doesn't need to be fixed for this patch > > > > and > > > > maybe doesn't need to be fixed at all, but it seemed worth > > > > highlighting. > > > > > > I don't think using a GC'd nfsd_file for LOCALIO is a bug. > > > > > > NFSv4 guarantees an OPEN to pin the nfsd_file, and a CLOSE > > > (or lease expiry) to release it. There's no desire for or > > > need for garbage collection. > > > > > > NFSv3 and LOCALIO use each nfsd_file for the life of one I/O > > > operation, and that nfsd_file is cached in the expectation > > > that another I/O operation on the same file will happen with > > > frequent temporarl proximity. Garbage collection is needed > > > for both cases. > > > > > > > No. There is a huge difference between the two. In the case of NFSv3, > > the server has no idea whether or not there is further need for the > > file (stateless), whereas in the localio case, the client is able to > > tell exactly when the application holds the file open or not > > (stateful). > > > > The only reason for jumping through all these hoops in the case of > > localio is the 'user may want to unmount' exception. > > > > Is there any reason why we couldn't add a notification for that, to > > give knfsd the opportunity to clear out any open file state? The > > current approach appears to be flat out optimising for the exception. > > > > > > No, and after discussing it with others here at the bake-a-thon, I > think that might be the best path forward here. At a high level: > > Add a callback to the client that runs just before calling > nfsd_file_cache_purge() in expkey_flush(). The client would be expected > to return all of its nfsd_files "soon" and switch back to normal > network I/O. After that point, it can try to get a new nfsd_file and > start up localio at that point. With that change too you can switch to > using non-GC'ed nfsd_files. The client can just hold them open and do > I/O to them at will. I don't think a "callback" is the best approach - certainly not in the sense of a function pointer that nfs gives to nfsd. Rather we want some protocol, mediated by nfs-local, which allows nfsd to invalidate a thing and nfs to know when it has been invalidated and to signal its release. I suspect the "thing" is an nfsd_file. I think a suitable protocol involves SRCU. There would be one SRCU state for all of nfsd. nfs gets a reference to an nfsd_file and holds it as long as it likes. Before accessing nf_file (or nf_inode) it gets srcu_read_lock() on nfsd and checks that the nfsd_file is still valid - probably if it is still hashed. If not it drops the reference. If it is valid it can safely use nf_file for an IO, and then srcu_read_unlock() to release it. nfsd_file_cache_purge() add a synchronize_srcu() call just before calling nfsd_file_dispose_list() We would need two different refcounts - on for localio to use which must be augmented with srcu and which doesn't prevent nfsd_file_cond_queue() from queuing for deletion, and the current one which does prevent queuing. The new refcount would be a kref and when when it reaches zero the nfsd_file is freed. So nfsd_file_slab_free() wouldn't free the nfsd_file but would kref_put() it. NeilBrown > > I think that's probably less brittle than trying to keep opaque inode > pointers around, and would probably mean better performance since you > won't be thrashing the filecache as much. > -- > Jeff Layton <jlayton@xxxxxxxxxx> > >
