On 23 Apr 2025, at 18:04, Benjamin Coddington wrote:
> On 23 Apr 2025, at 16:35, Jeff Layton wrote:
>
>> On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
>>> A follow-up patch intends to signal the success or failure of FREE_STATEID
>>> by modifying the nfs4_stateid's type which requires the const qualifier for
>>> the nfs4_stateid to be dropped. Since it will no longer safe to operate
>>> directly on shared stateid objects in this path, ensure that callers send a
>>> copy.
>>>
>>> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
>>> ---
>>> fs/nfs/nfs4proc.c | 12 +++++++-----
>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 6e95db6c17e9..bfb9e980d662 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -2990,6 +2990,7 @@ static void nfs41_delegation_recover_stateid(struct nfs4_state *state)
>>> static int nfs41_check_expired_locks(struct nfs4_state *state)
>>> {
>>> int status, ret = NFS_OK;
>>> + nfs4_stateid stateid;
>>> struct nfs4_lock_state *lsp, *prev = NULL;
>>> struct nfs_server *server = NFS_SERVER(state->inode);
>>>
>>> @@ -3007,9 +3008,9 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>>> nfs4_put_lock_state(prev);
>>> prev = lsp;
>>>
>>> + nfs4_stateid_copy(&stateid, &lsp->ls_stateid);
>>> status = nfs41_test_and_free_expired_stateid(server,
>>> - &lsp->ls_stateid,
>>> - cred);
>>> + &stateid, cred);
>>> trace_nfs4_test_lock_stateid(state, lsp, status);
>>> if (status == -NFS4ERR_EXPIRED ||
>>> status == -NFS4ERR_BAD_STATEID) {
>>> @@ -3042,17 +3043,18 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>>> static int nfs41_check_open_stateid(struct nfs4_state *state)
>>> {
>>> struct nfs_server *server = NFS_SERVER(state->inode);
>>> - nfs4_stateid *stateid = &state->open_stateid;
>>> + nfs4_stateid stateid;
>>> const struct cred *cred = state->owner->so_cred;
>>> int status;
>>>
>>> if (test_bit(NFS_OPEN_STATE, &state->flags) == 0)
>>> return -NFS4ERR_BAD_STATEID;
>>> - status = nfs41_test_and_free_expired_stateid(server, stateid, cred);
>>> + nfs4_stateid_copy(&stateid, &state->open_stateid);
>>> + status = nfs41_test_and_free_expired_stateid(server, &stateid, cred);
>>> trace_nfs4_test_open_stateid(state, NULL, status);
>>> if (status == -NFS4ERR_EXPIRED || status == -NFS4ERR_BAD_STATEID) {
>>> nfs_state_clear_open_state_flags(state);
>>> - stateid->type = NFS4_INVALID_STATEID_TYPE;
>>> + state->open_stateid.type = NFS4_INVALID_STATEID_TYPE;
>>> return status;
>>> }
>>> if (nfs_open_stateid_recover_openmode(state))
>>
>> I don't know that you really need to do this. In the cases where you
>> end up setting the type to FREED, you will also return NFS4ERR_EXPIRED,
>> which will make the callers set the type to INVALID.
>>
>> There will be a brief window where the type will be set to FREED, but
>> that should be no big deal.
>
> Definitely playing it safe here, I think I have to worry about concurrency -
> who else might be simultaneously looking at that nfs4_state->open_stateid.type?
> I really don't want to introduce a regression because we knew we weren't
> modifying that stateid before.
>
> I suppose I can dive into an audit and see if that can actually happen.
After looking theoretically there shouldn't be a race in either of these two
paths since we'll only be here doing reclaim/recovery and operating on the
state sequentially. State recovery will have drained and blocked parallel
opens.
I'll send just the 2nd patch in another version.
Ben
