Re: [PATCH 2/6] ceph: hold extra reference to r_parent over life of request

[Xiubo Li <xiubli@redhat.com>]

On 2020/1/9 21:33, Jeff Layton wrote:
> On Thu, 2020-01-09 at 21:16 +0800, Xiubo Li wrote:
>> On 2020/1/9 19:20, Jeff Layton wrote:
>>> On Thu, 2020-01-09 at 10:05 +0800, Xiubo Li wrote:
>>>> On 2020/1/6 23:35, Jeff Layton wrote:
>>>>> Currently, we just assume that it will stick around by virtue of the
>>>>> submitter's reference, but later patches will allow the syscall to
>>>>> return early and we can't rely on that reference at that point.
>>>>>
>>>>> Take an extra reference to the inode when setting r_parent and release
>>>>> it when releasing the request.
>>>>>
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>> ---
>>>>>     fs/ceph/mds_client.c | 8 ++++++--
>>>>>     1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>>> index 94cce2ab92c4..b7122f682678 100644
>>>>> --- a/fs/ceph/mds_client.c
>>>>> +++ b/fs/ceph/mds_client.c
>>>>> @@ -708,8 +708,10 @@ void ceph_mdsc_release_request(struct kref *kref)
>>>>>     		/* avoid calling iput_final() in mds dispatch threads */
>>>>>     		ceph_async_iput(req->r_inode);
>>>>>     	}
>>>>> -	if (req->r_parent)
>>>>> +	if (req->r_parent) {
>>>>>     		ceph_put_cap_refs(ceph_inode(req->r_parent), CEPH_CAP_PIN);
>>>>> +		ceph_async_iput(req->r_parent);
>>>>> +	}
>>>>>     	ceph_async_iput(req->r_target_inode);
>>>>>     	if (req->r_dentry)
>>>>>     		dput(req->r_dentry);
>>>>> @@ -2706,8 +2708,10 @@ int ceph_mdsc_submit_request(struct ceph_mds_client *mdsc, struct inode *dir,
>>>>>     	/* take CAP_PIN refs for r_inode, r_parent, r_old_dentry */
>>>>>     	if (req->r_inode)
>>>>>     		ceph_get_cap_refs(ceph_inode(req->r_inode), CEPH_CAP_PIN);
>>>>> -	if (req->r_parent)
>>>>> +	if (req->r_parent) {
>>>>>     		ceph_get_cap_refs(ceph_inode(req->r_parent), CEPH_CAP_PIN);
>>>>> +		ihold(req->r_parent);
>>>>> +	}
>>>> This might also fix another issue when the mdsc request is timedout and
>>>> returns to the vfs, then the r_parent maybe released in vfs. And then if
>>>> we reference it again in mdsc handle_reply() -->
>>>> ceph_mdsc_release_request(),  some unknown issues may happen later ??
>>>>
>>> AIUI, when a timeout occurs, the req is unhashed such that handle_reply
>>> can't find it. So, I doubt this affects that one way or another.
>> If my understanding is correct, such as for rmdir(), the logic will be :
>>
>> req = ceph_mdsc_create_request()      //  ref == 1
>>
>> ceph_mdsc_do_request(req) -->
>>
>>           ceph_mdsc_submit_request(req) -->
>>
>>                   __register_request(req) // ref == 2
>>
>>           ceph_mdsc_wait_request(req)  // If timedout
>>
>> ceph_mdsc_put_request(req)  // ref == 1
>>
>> Then in handled_reply(), only when we get a safe reply it will call
>> __unregister_request(req), then the ref could be 0.
>>
>> Though it will ihold()/ceph_async_iput() the req->r_unsafe_dir(=
>> req->r_parent) , but the _iput() will be called just before we reference
>> the req->r_parent in the _relase_request(). And the _iput() here may
>> call the iput_final().
>>
> I take it back, I think you're right. This likely would fix that issue
> up. I'll plan to add a note about that to the changelog before I merge
> it. Should we mark this for stable in light of that?

Yeah, right :-)