Re: [PATCH v4 5/8] NFSD check stateids against copy stateids

[Olga Kornievskaia <olga.kornievskaia@gmail.com>]

On Thu, Aug 1, 2019 at 3:36 PM J. Bruce Fields <bfields@redhat.com> wrote:
>
> On Thu, Aug 01, 2019 at 02:24:04PM -0400, Olga Kornievskaia wrote:
> > i was just looking at close_lru and delegation_lru but I guess that's
> > not a list of delegation or open stateids but rather some complex of
> > not deleting the stateid right away but moving it to nfs4_ol_stateid
> > and the list on the nfsd_net. Are you looking for something similar
> > for the copy_notify state or can I just keep a global list of the
> > nfs4_client and add and delete of that (not move to the delete later)?
>
> A global list seems like it should work if the locking's OK.

I'm having issues taking a reference on a parent stateid and being
able to clean it. Let me try to explain.

Since I take a reference on the stateid, then during what would have
been the last put (due to say a close operation), stateid isn't
released. Now that stateid is sticking around. I personally would have
liked on what would have been a close and release of the stateid to
release the copy notify state(s) (which was being done before but
having a reference makes it hard? i want to count number of copy
notify states and if then somehow if the num_copies-1 is going to make
it 0, then decrement by num_copies (and the normal -1) but if it's not
the last reference then it shouldn't be decremented.

Now say no fancy logic happens on close so we have these stateids left
over . What to do on unmount? It will error with err_client_busy since
there are non-zero copy notify states and only after a lease period it
will release the resources (when the close of the file should have
removed any copy notify state)?

Question: would it be acceptable to do something like this on freeing
of the parent stateid?

@@ -896,8 +931,12 @@ static void block_delegations(struct knfsd_fh *fh)
        might_lock(&clp->cl_lock);

        if (!refcount_dec_and_lock(&s->sc_count, &clp->cl_lock)) {
-               wake_up_all(&close_wq);
-               return;
+               if (!refcount_sub_and_test_checked(s->sc_cp_list_size,
+                               &s->sc_count)) {
+                       refcount_add_checked(s->sc_cp_list_size, &s->sc_count);
+                       wake_up_all(&close_wq);
+                       return;
+               }
        }
        idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
        spin_unlock(&clp->cl_lock);

then free the copy notify stateids associated with stateid.

Laundromat would still be checking the copy_notify stateids for
anything that's been not active for a while (but not closed).





>
> --b.