The check for whether or not we sent an RPC call in nfs40_sequence_done
is insufficient to decide whether or not we are holding a session slot,
and thus should not be used to decide when to free that slot.
This patch replaces the RPC_WAS_SENT() test with the correct test for
whether or not slot == NULL.
Cc: Chuck Lever <[email protected]>
Cc: [email protected] # 3.12+
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ae00c3ed733f..493e9cce1f11 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
struct nfs4_slot *slot = res->sr_slot;
struct nfs4_slot_table *tbl;
- if (!RPC_WAS_SENT(task))
+ if (slot == NULL)
goto out;
tbl = slot->table;
--
1.8.5.3
On Jan 30, 2014, at 12:03, Trond Myklebust <[email protected]> wrote:
>
> On Jan 30, 2014, at 12:02, Chuck Lever <[email protected]> wrote:
>
>>
>> On Jan 30, 2014, at 8:55 AM, Trond Myklebust <[email protected]> wrote:
>>
>>>
>>> On Jan 30, 2014, at 11:49, Chuck Lever <[email protected]> wrote:
>>>
>>>>
>>>> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <[email protected]> wrote:
>>>>
>>>>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>>>>> is insufficient to decide whether or not we are holding a session slot,
>>>>> and thus should not be used to decide when to free that slot.
>>>>>
>>>>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>>>>> whether or not slot == NULL.
>>>>>
>>>>> Cc: Chuck Lever <[email protected]>
>>>>> Cc: [email protected] # 3.12+
>>>>> Signed-off-by: Trond Myklebust <[email protected]>
>>>>> ---
>>>>> fs/nfs/nfs4proc.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>> index ae00c3ed733f..493e9cce1f11 100644
>>>>> --- a/fs/nfs/nfs4proc.c
>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>>>>> struct nfs4_slot *slot = res->sr_slot;
>>>>> struct nfs4_slot_table *tbl;
>>>>>
>>>>> - if (!RPC_WAS_SENT(task))
>>>>> + if (slot == NULL)
>>>>> goto out;
>>>>
>>>> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not. This patch should clean that up?
>>>
>>> Unfortunately we can?t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(
>>
>> Move the slot == NULL check to all nfs40_sequence_done() call sites? <shrug>
>
> No...
We could move the entire function minus the slot == NULL test into a helper that could be called by nfs4_sequence_done, but I?m not convinced that particular micro-optimisation is really worth it.
--
Trond Myklebust
Linux NFS client maintainer
On Jan 30, 2014, at 11:49, Chuck Lever <[email protected]> wrote:
>
> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <[email protected]> wrote:
>
>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>> is insufficient to decide whether or not we are holding a session slot,
>> and thus should not be used to decide when to free that slot.
>>
>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>> whether or not slot == NULL.
>>
>> Cc: Chuck Lever <[email protected]>
>> Cc: [email protected] # 3.12+
>> Signed-off-by: Trond Myklebust <[email protected]>
>> ---
>> fs/nfs/nfs4proc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index ae00c3ed733f..493e9cce1f11 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>> struct nfs4_slot *slot = res->sr_slot;
>> struct nfs4_slot_table *tbl;
>>
>> - if (!RPC_WAS_SENT(task))
>> + if (slot == NULL)
>> goto out;
>
> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not. This patch should clean that up?
Unfortunately we can?t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(
--
Trond Myklebust
Linux NFS client maintainer
It is now completely safe to call nfs41_sequence_free_slot with a NULL
slot.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4filelayout.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 20a56fa271bd..12c8132ad408 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -336,8 +336,7 @@ static void filelayout_read_call_done(struct rpc_task *task, void *data)
if (test_bit(NFS_IOHDR_REDO, &rdata->header->flags) &&
task->tk_status == 0) {
- if (rdata->res.seq_res.sr_slot != NULL)
- nfs41_sequence_done(task, &rdata->res.seq_res);
+ nfs41_sequence_done(task, &rdata->res.seq_res);
return;
}
@@ -446,8 +445,7 @@ static void filelayout_write_call_done(struct rpc_task *task, void *data)
if (test_bit(NFS_IOHDR_REDO, &wdata->header->flags) &&
task->tk_status == 0) {
- if (wdata->res.seq_res.sr_slot != NULL)
- nfs41_sequence_done(task, &wdata->res.seq_res);
+ nfs41_sequence_done(task, &wdata->res.seq_res);
return;
}
--
1.8.5.3
On Jan 30, 2014, at 12:02, Chuck Lever <[email protected]> wrote:
>
> On Jan 30, 2014, at 8:55 AM, Trond Myklebust <[email protected]> wrote:
>
>>
>> On Jan 30, 2014, at 11:49, Chuck Lever <[email protected]> wrote:
>>
>>>
>>> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <[email protected]> wrote:
>>>
>>>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>>>> is insufficient to decide whether or not we are holding a session slot,
>>>> and thus should not be used to decide when to free that slot.
>>>>
>>>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>>>> whether or not slot == NULL.
>>>>
>>>> Cc: Chuck Lever <[email protected]>
>>>> Cc: [email protected] # 3.12+
>>>> Signed-off-by: Trond Myklebust <[email protected]>
>>>> ---
>>>> fs/nfs/nfs4proc.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index ae00c3ed733f..493e9cce1f11 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>>>> struct nfs4_slot *slot = res->sr_slot;
>>>> struct nfs4_slot_table *tbl;
>>>>
>>>> - if (!RPC_WAS_SENT(task))
>>>> + if (slot == NULL)
>>>> goto out;
>>>
>>> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not. This patch should clean that up?
>>
>> Unfortunately we can?t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(
>
> Move the slot == NULL check to all nfs40_sequence_done() call sites? <shrug>
No...
--
Trond Myklebust
Linux NFS client maintainer
Move the test for res->sr_slot == NULL out of the nfs41_sequence_free_slot
helper and into the main function for efficiency.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 493e9cce1f11..42da6af77587 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -559,15 +559,10 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
{
struct nfs4_session *session;
struct nfs4_slot_table *tbl;
+ struct nfs4_slot *slot = res->sr_slot;
bool send_new_highest_used_slotid = false;
- if (!res->sr_slot) {
- /* just wake up the next guy waiting since
- * we may have not consumed a slot after all */
- dprintk("%s: No slot\n", __func__);
- return;
- }
- tbl = res->sr_slot->table;
+ tbl = slot->table;
session = tbl->session;
spin_lock(&tbl->slot_tbl_lock);
@@ -577,11 +572,11 @@ static void nfs41_sequence_free_slot(struct nfs4_sequence_res *res)
if (tbl->highest_used_slotid > tbl->target_highest_slotid)
send_new_highest_used_slotid = true;
- if (nfs41_wake_and_assign_slot(tbl, res->sr_slot)) {
+ if (nfs41_wake_and_assign_slot(tbl, slot)) {
send_new_highest_used_slotid = false;
goto out_unlock;
}
- nfs4_free_slot(tbl, res->sr_slot);
+ nfs4_free_slot(tbl, slot);
if (tbl->highest_used_slotid != NFS4_NO_SLOT)
send_new_highest_used_slotid = false;
@@ -595,16 +590,17 @@ out_unlock:
int nfs41_sequence_done(struct rpc_task *task, struct nfs4_sequence_res *res)
{
struct nfs4_session *session;
- struct nfs4_slot *slot;
+ struct nfs4_slot *slot = res->sr_slot;
struct nfs_client *clp;
bool interrupted = false;
int ret = 1;
+ if (slot == NULL)
+ goto out_noaction;
/* don't increment the sequence number if the task wasn't sent */
if (!RPC_WAS_SENT(task))
goto out;
- slot = res->sr_slot;
session = slot->table->session;
if (slot->interrupted) {
@@ -679,6 +675,7 @@ out:
/* The session may be reset by one of the error handlers. */
dprintk("%s: Error %d free the slot \n", __func__, res->sr_status);
nfs41_sequence_free_slot(res);
+out_noaction:
return ret;
retry_nowait:
if (rpc_restart_call_prepare(task)) {
--
1.8.5.3
On Jan 29, 2014, at 9:36 AM, Trond Myklebust <[email protected]> wrote:
> The check for whether or not we sent an RPC call in nfs40_sequence_done
> is insufficient to decide whether or not we are holding a session slot,
> and thus should not be used to decide when to free that slot.
>
> This patch replaces the RPC_WAS_SENT() test with the correct test for
> whether or not slot == NULL.
>
> Cc: Chuck Lever <[email protected]>
> Cc: [email protected] # 3.12+
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ae00c3ed733f..493e9cce1f11 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
> struct nfs4_slot *slot = res->sr_slot;
> struct nfs4_slot_table *tbl;
>
> - if (!RPC_WAS_SENT(task))
> + if (slot == NULL)
> goto out;
When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not. This patch should clean that up?
>
> tbl = slot->table;
> --
> 1.8.5.3
>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Jan 30, 2014, at 8:55 AM, Trond Myklebust <[email protected]> wrote:
>
> On Jan 30, 2014, at 11:49, Chuck Lever <[email protected]> wrote:
>
>>
>> On Jan 29, 2014, at 9:36 AM, Trond Myklebust <[email protected]> wrote:
>>
>>> The check for whether or not we sent an RPC call in nfs40_sequence_done
>>> is insufficient to decide whether or not we are holding a session slot,
>>> and thus should not be used to decide when to free that slot.
>>>
>>> This patch replaces the RPC_WAS_SENT() test with the correct test for
>>> whether or not slot == NULL.
>>>
>>> Cc: Chuck Lever <[email protected]>
>>> Cc: [email protected] # 3.12+
>>> Signed-off-by: Trond Myklebust <[email protected]>
>>> ---
>>> fs/nfs/nfs4proc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index ae00c3ed733f..493e9cce1f11 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -539,7 +539,7 @@ static int nfs40_sequence_done(struct rpc_task *task,
>>> struct nfs4_slot *slot = res->sr_slot;
>>> struct nfs4_slot_table *tbl;
>>>
>>> - if (!RPC_WAS_SENT(task))
>>> + if (slot == NULL)
>>> goto out;
>>
>> When CONFIG_NFS_V4_1 is enabled, nfs4_sequence_done() already does the slot == NULL test, though the other nfs40_sequence_done() call sites do not. This patch should clean that up?
>
> Unfortunately we can?t touch nfs4_sequence_done: it wants to test for whether or not a NFSv4.1 session exists, which requires it to look at slot->table->session. :-(
Move the slot == NULL check to all nfs40_sequence_done() call sites? <shrug>
If not, can we get a code comment that explains why that check is done twice in the common path?
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com