On Sat, Feb 17, 2024 at 10:00:59AM -0800, Dai Ngo wrote: > When the number of granted delegation becomes large, there are some > undesire effects happen on the NFS server. Besides the consumption > of system resources, the number of entries on the linked lists of the > file cache can grow significantly. > > When this condition happens, the NFS performace grounds to a halt as > reported here [1]. That was a v5.15.131 kernel. The LRU problems were addressed in v6.2. This doesn't seem like a clean rationale for adding this reaper behavior in, say, v6.9. > This patch attempts to alleviate this problem by asking the clients to > voluntarily return any unused delegations when the number of delegation > reaches 3/4 of the max_delegations by sending OP_CB_RECALL_ANY to all > clients that hold delegations. I don't have a strong sense of how big max_delegations can get. Is there evidence that CB_RECALL_ANY has enough impact, reliably, to reduce the size of the filecache? More below. > [1] https://lore.kernel.org/all/PH0PR14MB5493F59229B802B871407F9CAA442@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx> > --- > fs/nfsd/nfs4state.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index fdc95bfbfbb6..5fb83853533f 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -130,6 +130,7 @@ static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops; > static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops; > > static struct workqueue_struct *laundry_wq; > +static void deleg_reaper(struct nfsd_net *nn); > > int nfsd4_create_laundry_wq(void) > { > @@ -696,6 +697,9 @@ static struct nfsd_file *find_any_file_locked(struct nfs4_file *f) > static atomic_long_t num_delegations; > unsigned long max_delegations; > > +/* threshold to trigger deleg_reaper */ > +static unsigned long delegations_soft_limit; > + > /* > * Open owner state (share locks) > */ > @@ -6466,6 +6470,7 @@ nfs4_laundromat(struct nfsd_net *nn) > struct nfs4_cpntf_state *cps; > copy_stateid_t *cps_t; > int i; > + long n; > > if (clients_still_reclaiming(nn)) { > lt.new_timeo = 0; > @@ -6550,6 +6555,9 @@ nfs4_laundromat(struct nfsd_net *nn) > /* service the server-to-server copy delayed unmount list */ > nfsd4_ssc_expire_umount(nn); > #endif > + n = atomic_long_inc_return(&num_delegations); I don't think you want to modify the number of delegations here. atomic_long_read() instead? > + if (n > delegations_soft_limit) This introduces a mixed-sign comparison: n is a long, but delegations_soft_limit is an unsigned long. I'm always suspicious about whether an atomic counter can underflow, and then we have real problems when there are mixed-sign comparisons. But I'm also wondering if, instead, this logic should look directly at the length of the filecache LRU. > + deleg_reaper(nn); > out: > return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT); > } > @@ -8482,6 +8490,7 @@ set_max_delegations(void) > * giving a worst case usage of about 6% of memory. > */ > max_delegations = nr_free_buffer_pages() >> (20 - 2 - PAGE_SHIFT); > + delegations_soft_limit = (max_delegations / 4) * 3; I don't see a strong reason to keep delegations_soft_limit as a separate variable. > } > > static int nfs4_state_create_net(struct net *net) > -- > 2.39.3 > -- Chuck Lever
