> On 2 Feb 2017, at 19:26, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Thu, 2017-02-02 at 16:06 +0800, Yan, Zheng wrote:
>>> On 1 Feb 2017, at 19:49, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>>
>>> ...and simplify the handling of it in ceph_fill_trace.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>> ---
>>> fs/ceph/inode.c | 17 +++++++++--------
>>> fs/ceph/mds_client.c | 8 ++++----
>>> fs/ceph/mds_client.h | 2 +-
>>> 3 files changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>> index 4926265f4223..bd2e94a78057 100644
>>> --- a/fs/ceph/inode.c
>>> +++ b/fs/ceph/inode.c
>>> @@ -1233,8 +1233,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req,
>>>
>>> err = fill_inode(in, req->r_locked_page, &rinfo->targeti, NULL,
>>> session, req->r_request_started,
>>> - (!req->r_aborted && rinfo->head->result == 0) ?
>>> - req->r_fmode : -1,
>>> + (!test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
>>> + rinfo->head->result == 0) ? req->r_fmode : -1,
>>> &req->r_caps_reservation);
>>> if (err < 0) {
>>> pr_err("fill_inode badness %p %llx.%llx\n",
>>> @@ -1243,12 +1243,14 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req,
>>> }
>>> }
>>>
>>> + if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
>>> + goto done;
>>> +
>>
>> This seems wrong. If we got the reply, we should update inode’s caps even request is aborted.
>> Otherwise, MDS can hang at revoking the dropped caps.
>>
>
> FWIW, I hit that exact problem in an earlier iteration of this patchset
> until I figured out what was going on. The caps do get updated with this
> patch. That gets done in fill_inode, and that's called even when the
> call was aborted.
>
> None of the "move the flag into r_req_flags" patches materially change
> the behavior of the code. In this patch, we're just moving both of the
> subsequent !req->r_aborted flag checks into this one spot.
I think you are right. Thank you for explanation
Yan, Zheng
>
>>> /*
>>> * ignore null lease/binding on snapdir ENOENT, or else we
>>> * will have trouble splicing in the virtual snapdir later
>>> */
>>> - if (rinfo->head->is_dentry && !req->r_aborted &&
>>> - req->r_locked_dir &&
>>> + if (rinfo->head->is_dentry && req->r_locked_dir &&
>>> (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
>>> fsc->mount_options->snapdir_name,
>>> req->r_dentry->d_name.len))) {
>>> @@ -1351,9 +1353,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req,
>>> update_dentry_lease(dn, rinfo->dlease, session,
>>> req->r_request_started);
>>> dout(" final dn %p\n", dn);
>>> - } else if (!req->r_aborted &&
>>> - (req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||
>>> - req->r_op == CEPH_MDS_OP_MKSNAP)) {
>>> + } else if (req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||
>>> + req->r_op == CEPH_MDS_OP_MKSNAP) {
>>> struct dentry *dn = req->r_dentry;
>>> struct inode *dir = req->r_locked_dir;
>>>
>>> @@ -1478,7 +1479,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>> u32 fpos_offset;
>>> struct ceph_readdir_cache_control cache_ctl = {};
>>>
>>> - if (req->r_aborted)
>>> + if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
>>> return readdir_prepopulate_inodes_only(req, session);
>>>
>>> if (rinfo->hash_order && req->r_path2) {
>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>> index 1f2ef02832d9..a5156b6a0aed 100644
>>> --- a/fs/ceph/mds_client.c
>>> +++ b/fs/ceph/mds_client.c
>>> @@ -2115,7 +2115,7 @@ static int __do_request(struct ceph_mds_client *mdsc,
>>> int err = 0;
>>>
>>> if (req->r_err || req->r_got_result) {
>>> - if (req->r_aborted)
>>> + if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
>>> __unregister_request(mdsc, req);
>>> goto out;
>>> }
>>> @@ -2331,7 +2331,7 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
>>> */
>>> mutex_lock(&req->r_fill_mutex);
>>> req->r_err = err;
>>> - req->r_aborted = true;
>>> + set_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags);
>>> mutex_unlock(&req->r_fill_mutex);
>>>
>>> if (req->r_locked_dir &&
>>> @@ -2538,7 +2538,7 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>>> }
>>> out_err:
>>> mutex_lock(&mdsc->mutex);
>>> - if (!req->r_aborted) {
>>> + if (!test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) {
>>> if (err) {
>>> req->r_err = err;
>>> } else {
>>> @@ -2587,7 +2587,7 @@ static void handle_forward(struct ceph_mds_client *mdsc,
>>> goto out; /* dup reply? */
>>> }
>>>
>>> - if (req->r_aborted) {
>>> + if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) {
>>> dout("forward tid %llu aborted, unregistering\n", tid);
>>> __unregister_request(mdsc, req);
>>> } else if (fwd_seq <= req->r_num_fwd) {
>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>> index a58cacccc986..3da20955d5e6 100644
>>> --- a/fs/ceph/mds_client.h
>>> +++ b/fs/ceph/mds_client.h
>>> @@ -206,6 +206,7 @@ struct ceph_mds_request {
>>> struct inode *r_target_inode; /* resulting inode */
>>>
>>> #define CEPH_MDS_R_DIRECT_IS_HASH (1) /* r_direct_hash is valid */
>>> +#define CEPH_MDS_R_ABORTED (2) /* call was aborted */
>>> unsigned long r_req_flags;
>>>
>>> struct mutex r_fill_mutex;
>>> @@ -236,7 +237,6 @@ struct ceph_mds_request {
>>> struct ceph_mds_reply_info_parsed r_reply_info;
>>> struct page *r_locked_page;
>>> int r_err;
>>> - bool r_aborted;
>>>
>>> unsigned long r_timeout; /* optional. jiffies, 0 is "wait forever" */
>>> unsigned long r_started; /* start time to measure timeout against */
>>> --
>>> 2.9.3
>>>
>>
>>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
