On Mon, Mar 20 2017, Olga Kornievskaia wrote:
> Since open_owner is no longer removed then the resources are not
> freed. They will not be freed until unmount (or reboot recovery).
I don't follow your reasoning.
When the last uses for a state owner is dropped by
nfs4_put_state_owner() (e.g. when the last open file using it is
closed), nfs4_put_state_owner() will add the state to
server->state_owners_lru.
nfs4_gc_state_owners() is called periodically (every time any file is
opened I think) and it will call nfs4_remove_state_owner_locked()
on any state which is sufficiently old enough (hasn't been used in
'lease-time'), and will then call nfs4_free_state_owner().
These two will release all resources.
Does that explanation fit with your understanding?
Thanks,
NeilBrown
>
> There are (buggy) servers out there that might be forcing the client
> to keep creating new open_owners. So if they are no longer released,
> can't the client get into trouble here?
>
> On Sun, Dec 18, 2016 at 7:48 PM, NeilBrown <neilb@xxxxxxxx> wrote:
>>
>> When an NFS4ERR_BAD_SEQID is received the open-owner is removed from
>> the ->state_owners rbtree so that it will no longer be used.
>>
>> If any stateids attached to this open-owner are still in use, and if a
>> request using one gets an NFS4ERR_BAD_STATEID reply, this can for bad.
>>
>> The state is marked as needing recovery and the nfs4_state_manager()
>> is scheduled to clean up. nfs4_state_manager() finds states to be
>> recovered by walking the state_owners rbtree. As the open-owner is
>> not in the rbtree, the bad state is not found so nfs4_state_manager()
>> completes having done nothing. The request is then retried, with a
>> predicatable result (indefinite retries).
>>
>> If the stateid is for a delegation, this open_owner will be used
>> to open files when the delegation is returned. For that to work,
>> a new open-owner needs to be presented to the server.
>>
>> This patch changes NFS4ERR_BAD_SEQID handling to leave the open-owner
>> in the rbtree but updates the 'create_time' so it looks like a new
>> open-owner. With this the indefinite retries no longer happen.
>>
>> Signed-off-by: NeilBrown <neilb@xxxxxxxx>
>> ---
>>
>> Hi Trond,
>> It appears this one got lost too.
>> I've added a comment as I thought an explanation was needed, and
>> renamed the function from "drop" to "reset".
>>
>> Thanks,
>> NeilBrown
>>
>> fs/nfs/nfs4state.c | 29 +++++++++++++----------------
>> 1 file changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index cf869802ff23..1d152f4470cd 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -494,21 +494,18 @@ nfs4_alloc_state_owner(struct nfs_server *server,
>> }
>>
>> static void
>> -nfs4_drop_state_owner(struct nfs4_state_owner *sp)
>> -{
>> - struct rb_node *rb_node = &sp->so_server_node;
>> -
>> - if (!RB_EMPTY_NODE(rb_node)) {
>> - struct nfs_server *server = sp->so_server;
>> - struct nfs_client *clp = server->nfs_client;
>> -
>> - spin_lock(&clp->cl_lock);
>> - if (!RB_EMPTY_NODE(rb_node)) {
>> - rb_erase(rb_node, &server->state_owners);
>> - RB_CLEAR_NODE(rb_node);
>> - }
>> - spin_unlock(&clp->cl_lock);
>> - }
>> +nfs4_reset_state_owner(struct nfs4_state_owner *sp)
>> +{
>> + /* This state_owner is no longer usable, but must
>> + * remain in place so that state recovery can find it
>> + * and the opens associated with it.
>> + * It may also be used for new 'open' request to
>> + * return a delegation to the server.
>> + * So update the 'create_time' so that it looks like
>> + * a new state_owner. This will cause the server to
>> + * request an OPEN_CONFIRM to start a new sequence.
>> + */
>> + sp->so_seqid.create_time = ktime_get();
>> }
>>
>> static void nfs4_free_state_owner(struct nfs4_state_owner *sp)
>> @@ -1113,7 +1110,7 @@ void nfs_increment_open_seqid(int status, struct nfs_seqid *seqid)
>>
>> sp = container_of(seqid->sequence, struct nfs4_state_owner, so_seqid);
>> if (status == -NFS4ERR_BAD_SEQID)
>> - nfs4_drop_state_owner(sp);
>> + nfs4_reset_state_owner(sp);
>> if (!nfs4_has_session(sp->so_server->nfs_client))
>> nfs_increment_seqid(status, seqid);
>> }
>> --
>> 2.11.0
>>
Attachment:
signature.asc
Description: PGP signature
