On Fri, Feb 6, 2015 at 9:57 AM, Peng Tao <tao.peng@xxxxxxxxxxxxxxx> wrote:
> On Fri, Feb 6, 2015 at 9:45 AM, Peng Tao <tao.peng@xxxxxxxxxxxxxxx> wrote:
>> On Fri, Feb 6, 2015 at 6:37 AM, Trond Myklebust
>> <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>>> If we have to do a return-on-close in the delegreturn code, then
>>> we must ensure that the inode and super block remain referenced.
>>>
> ah, a second thought. I looked for call sites of nfs_sb_active() and
> it gets called at five places in current tree:
> alloc_nfs_open_context, nfs4_opendata_alloc, nfs4_do_close,
> nfs_do_call_unlink, nfs_do_call_unlink
>
> So it appears that sb is activated while any file keeps opened and
> between unlink calls. Then it looks that we are allowed to keep
> delegations after sb is released? Maybe the best way to fix the sb
> reference part is to pin sb when getting the first delegation.
>
err, that cannot be working at all... by pinning super block, we
prevent umount from happening. and we are returning delegations while
shutting down nfs_server and evicting inode. I see your point that the
patch actually intends to deal with race between async delegation
return vs. nfs_server shutting down and inode eviction.
oops! sorry for the noise... you patch is definitely the way we want to go.
Cheers,
Tao
> Cheers,
> Tao
>
>> looks good. One nit is that maybe it's better to reuse the two helpers
>> in your 2ed patch.
>>
>> Reviewed-by: Peng Tao <tao.peng@xxxxxxxxxxxxxxx>
>>> Cc: Peng Tao <tao.peng@xxxxxxxxxxxxxxx>
>>> Cc: stable@xxxxxxxxxxxxxxx # 3.17.x
>>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>> ---
>>> fs/nfs/nfs4proc.c | 19 ++++++++++++++-----
>>> 1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index cd4295d84d54..b803c1d363e7 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -5175,9 +5175,16 @@ static void nfs4_delegreturn_done(struct rpc_task *task, void *calldata)
>>> static void nfs4_delegreturn_release(void *calldata)
>>> {
>>> struct nfs4_delegreturndata *data = calldata;
>>> + struct inode *inode = data->inode;
>>> +
>>> + if (inode) {
>>> + struct super_block *sb = inode->i_sb;
>>>
>>> - if (data->roc)
>>> - pnfs_roc_release(data->inode);
>>> + if (data->roc)
>>> + pnfs_roc_release(inode);
>>> + iput(inode);
>>> + nfs_sb_deactive(sb);
>>> + }
>>> kfree(calldata);
>>> }
>>>
>>> @@ -5234,9 +5241,11 @@ static int _nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, co
>>> nfs_fattr_init(data->res.fattr);
>>> data->timestamp = jiffies;
>>> data->rpc_status = 0;
>>> - data->inode = inode;
>>> - data->roc = list_empty(&NFS_I(inode)->open_files) ?
>>> - pnfs_roc(inode) : false;
>>> + data->inode = igrab(inode);
>>> + if (data->inode) {
>>> + nfs_sb_active(inode->i_sb);
>>> + data->roc = nfs4_roc(inode);
>>> + }
>>>
>>> task_setup_data.callback_data = data;
>>> msg.rpc_argp = &data->args;
>>> --
>>> 2.1.0
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
