On Mon, 2024-09-09 at 15:06 +1000, NeilBrown wrote:
> The pair of bloom filtered used by delegation_blocked() was intended to
> block delegations on given filehandles for between 30 and 60 seconds. A
> new filehandle would be recorded in the "new" bit set. That would then
> be switch to the "old" bit set between 0 and 30 seconds later, and it
> would remain as the "old" bit set for 30 seconds.
>
Since we're on the subject...
60s seems like an awfully long time to block delegations on an inode.
Recalls generally don't take more than a few seconds when things are
functioning properly.
Should we swap the bloom filters more often?
> Unfortunately the code intended to clear the old bit set once it reached
> 30 seconds old, preparing it to be the next new bit set, instead cleared
> the *new* bit set before switching it to be the old bit set. This means
> that the "old" bit set is always empty and delegations are blocked
> between 0 and 30 seconds.
>
> This patch updates bd->new before clearing the set with that index,
> instead of afterwards.
>
> Reported-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 6282cd565553 ("NFSD: Don't hand out delegations for 30 seconds after recalling them.")
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4313addbe756..6f18c1a7af2e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1078,7 +1078,8 @@ static void nfs4_free_deleg(struct nfs4_stid *stid)
> * When a delegation is recalled, the filehandle is stored in the "new"
> * filter.
> * Every 30 seconds we swap the filters and clear the "new" one,
> - * unless both are empty of course.
> + * unless both are empty of course. This results in delegations for a
> + * given filehandle being blocked for between 30 and 60 seconds.
> *
> * Each filter is 256 bits. We hash the filehandle to 32bit and use the
> * low 3 bytes as hash-table indices.
> @@ -1107,9 +1108,9 @@ static int delegation_blocked(struct knfsd_fh *fh)
> if (ktime_get_seconds() - bd->swap_time > 30) {
> bd->entries -= bd->old_entries;
> bd->old_entries = bd->entries;
> + bd->new = 1-bd->new;
> memset(bd->set[bd->new], 0,
> sizeof(bd->set[0]));
> - bd->new = 1-bd->new;
> bd->swap_time = ktime_get_seconds();
> }
> spin_unlock(&blocked_delegations_lock);
--
Jeff Layton <jlayton@xxxxxxxxxx>
