| Message ID | 1395849036-6907-1-git-send-email-steved@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mar 26, 2014, at 8:50, Steve Dickson <steved@redhat.com> wrote: > nfs4_run_open_task() puts a ref count on the nfs4_opendata > data pointer, then tries to allocate the task pointer. > If that task allocation fails nfs4_run_open_task() returns > leaving the ref count on the data pointer. > Hi Steve, That should not be the case. rpc_run_task() will always call nfs4_open_release() even if it returns an error. Cheers Trond
On 03/26/2014 01:25 PM, Trond Myklebust wrote: > > On Mar 26, 2014, at 8:50, Steve Dickson <steved@redhat.com> wrote: > >> nfs4_run_open_task() puts a ref count on the nfs4_opendata >> data pointer, then tries to allocate the task pointer. >> If that task allocation fails nfs4_run_open_task() returns >> leaving the ref count on the data pointer. >> > > Hi Steve, > > That should not be the case. rpc_run_task() will always call nfs4_open_release() even if it returns an error. How can rpc_run_task() run without a task pointer? and I think you need a task allocated pointer to even call nfs4_open_release() Here is what I'm seeing: If rpc_alloc_task() fails, rpc_new_task() returns ERR_PTR(-ENOMEM) In rpc_run_task() we do task = rpc_new_task(task_setup_data); if (IS_ERR(task)) goto out; : : : out: return task; In nfs4_run_open_task() we do kref_get(&data->kref); : : : task = rpc_run_task(&task_setup_data) If (IS_ERR(task)) return PTR_ERR(task); What am I missing? steved. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mar 26, 2014, at 10:48, Steve Dickson <SteveD@redhat.com> wrote: > > > On 03/26/2014 01:25 PM, Trond Myklebust wrote: >> >> On Mar 26, 2014, at 8:50, Steve Dickson <steved@redhat.com> wrote: >> >>> nfs4_run_open_task() puts a ref count on the nfs4_opendata >>> data pointer, then tries to allocate the task pointer. >>> If that task allocation fails nfs4_run_open_task() returns >>> leaving the ref count on the data pointer. >>> >> >> Hi Steve, >> >> That should not be the case. rpc_run_task() will always call nfs4_open_release() even if it returns an error. > How can rpc_run_task() run without a task pointer? and I think > you need a task allocated pointer to even call nfs4_open_release() No. nfs4_open_release() is called without an rpc task. This is how it is designed to work. > > Here is what I'm seeing: > > If rpc_alloc_task() fails, rpc_new_task() returns ERR_PTR(-ENOMEM) Yes, but before returning ERR_PTR(-ENOMEM), it calls rpc_release_calldata(setup_data->callback_ops, setup_data->callback_data); which again calls rpc_ops->rpc_release(calldata)
On 03/26/2014 02:39 PM, Trond Myklebust wrote: >> Here is what I'm seeing: >> >> If rpc_alloc_task() fails, rpc_new_task() returns ERR_PTR(-ENOMEM) > > Yes, but before returning ERR_PTR(-ENOMEM), it calls > > rpc_release_calldata(setup_data->callback_ops, setup_data->callback_data); > > which again calls rpc_ops->rpc_release(calldata) Ah... I did miss that call... sorry or the noise... steved. > > _________________________________ > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 2da6a69..a5a61ec 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1846,6 +1846,13 @@ static int nfs4_run_open_task(struct nfs4_opendata *data, int isrecover) }; int status; + /* + * Allocate the task before kref-ing the data + */ + task = rpc_run_task(&task_setup_data); + if (IS_ERR(task)) + return PTR_ERR(task); + nfs4_init_sequence(&o_arg->seq_args, &o_res->seq_res, 1); kref_get(&data->kref); data->rpc_done = 0; @@ -1856,16 +1863,13 @@ static int nfs4_run_open_task(struct nfs4_opendata *data, int isrecover) nfs4_set_sequence_privileged(&o_arg->seq_args); data->is_recover = 1; } - task = rpc_run_task(&task_setup_data); - if (IS_ERR(task)) - return PTR_ERR(task); - status = nfs4_wait_for_completion_rpc_task(task); - if (status != 0) { - data->cancelled = 1; - smp_wmb(); - } else - status = data->rpc_status; - rpc_put_task(task); + status = nfs4_wait_for_completion_rpc_task(task); + if (status != 0) { + data->cancelled = 1; + smp_wmb(); + } else + status = data->rpc_status; + rpc_put_task(task); return status; }
nfs4_run_open_task() puts a ref count on the nfs4_opendata data pointer, then tries to allocate the task pointer. If that task allocation fails nfs4_run_open_task() returns leaving the ref count on the data pointer. That extra ref count stops the data pointer from being freed in _nfs4_do_open() as expected. This patch reverse the order of the task allocation and ref count. The task is allocated before the ref count is done, allowing the allocation to fail (and return) without the ref count. Signed-off-by: Steve Dickson <steved@redhat.com> --- fs/nfs/nfs4proc.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)