On Fri, May 22, 2020 at 3:31 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote: > > On 2020/5/21 16:45, Yan, Zheng wrote: > > On Thu, May 21, 2020 at 3:39 PM <xiubli@xxxxxxxxxx> wrote: > >> From: Xiubo Li <xiubli@xxxxxxxxxx> > >> > >> In the ceph_check_caps() it may call the session lock/unlock stuff. > >> > >> There have some deadlock cases, like: > >> handle_forward() > >> ... > >> mutex_lock(&mdsc->mutex) > >> ... > >> ceph_mdsc_put_request() > >> --> ceph_mdsc_release_request() > >> --> ceph_put_cap_request() > >> --> ceph_put_cap_refs() > >> --> ceph_check_caps() > >> ... > >> mutex_unlock(&mdsc->mutex) > > For this case, it's better to call ceph_mdsc_put_request() after > > unlock mdsc->mutex > > Hi Zheng Yan, Jeff > > For this case there at least have 6 places, for some cases we can call > ceph_mdsc_put_request() after unlock mdsc->mutex very easily, but for > the others like: > > cleanup_session_requests() > > --> mutex_lock(&mdsc->mutex); > > --> __unregister_request() > > --> ceph_mdsc_put_request() ===> will call session lock/unlock pair > > --> mutex_unlock(&mdsc->mutex); > > There also has some more complicated cases, such as in handle_session(do > the mdsc->mutex lock/unlock pair) --> __wake_requests() --> > __do_request() --> __unregister_request() --> ceph_mdsc_put_request(). > > Maybe using the ceph_async_check_caps() is a better choice here, any idea ? > I think it's better to put_cap_refs async (only for ceph_mdsc_release_dir_caps) instead of async check_caps. > Thanks > > BRs > > Xiubo > > > >> And also there maybe has some double session lock cases, like: > >> > >> send_mds_reconnect() > >> ... > >> mutex_lock(&session->s_mutex); > >> ... > >> --> replay_unsafe_requests() > >> --> ceph_mdsc_release_dir_caps() > >> --> ceph_put_cap_refs() > >> --> ceph_check_caps() > >> ... > >> mutex_unlock(&session->s_mutex); > > There is no point to check_caps() and send cap message while > > reconnecting caps. So I think it's better to just skip calling > > ceph_check_caps() for this case. > > > > Regards > > Yan, Zheng > > > >> URL: https://tracker.ceph.com/issues/45635 > >> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx> > >> --- > >> fs/ceph/caps.c | 2 +- > >> fs/ceph/inode.c | 10 ++++++++++ > >> fs/ceph/super.h | 12 ++++++++++++ > >> 3 files changed, 23 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > >> index 27c2e60..08194c4 100644 > >> --- a/fs/ceph/caps.c > >> +++ b/fs/ceph/caps.c > >> @@ -3073,7 +3073,7 @@ void ceph_put_cap_refs(struct ceph_inode_info *ci, int had) > >> last ? " last" : "", put ? " put" : ""); > >> > >> if (last) > >> - ceph_check_caps(ci, 0, NULL); > >> + ceph_async_check_caps(ci); > >> else if (flushsnaps) > >> ceph_flush_snaps(ci, NULL); > >> if (wake) > >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > >> index 357c937..84a61d4 100644 > >> --- a/fs/ceph/inode.c > >> +++ b/fs/ceph/inode.c > >> @@ -35,6 +35,7 @@ > >> static const struct inode_operations ceph_symlink_iops; > >> > >> static void ceph_inode_work(struct work_struct *work); > >> +static void ceph_check_caps_work(struct work_struct *work); > >> > >> /* > >> * find or create an inode, given the ceph ino number > >> @@ -518,6 +519,7 @@ struct inode *ceph_alloc_inode(struct super_block *sb) > >> INIT_LIST_HEAD(&ci->i_snap_flush_item); > >> > >> INIT_WORK(&ci->i_work, ceph_inode_work); > >> + INIT_WORK(&ci->check_caps_work, ceph_check_caps_work); > >> ci->i_work_mask = 0; > >> memset(&ci->i_btime, '\0', sizeof(ci->i_btime)); > >> > >> @@ -2012,6 +2014,14 @@ static void ceph_inode_work(struct work_struct *work) > >> iput(inode); > >> } > >> > >> +static void ceph_check_caps_work(struct work_struct *work) > >> +{ > >> + struct ceph_inode_info *ci = container_of(work, struct ceph_inode_info, > >> + check_caps_work); > >> + > >> + ceph_check_caps(ci, 0, NULL); > >> +} > >> + > >> /* > >> * symlinks > >> */ > >> diff --git a/fs/ceph/super.h b/fs/ceph/super.h > >> index 226f19c..96d0e41 100644 > >> --- a/fs/ceph/super.h > >> +++ b/fs/ceph/super.h > >> @@ -421,6 +421,8 @@ struct ceph_inode_info { > >> struct timespec64 i_btime; > >> struct timespec64 i_snap_btime; > >> > >> + struct work_struct check_caps_work; > >> + > >> struct work_struct i_work; > >> unsigned long i_work_mask; > >> > >> @@ -1102,6 +1104,16 @@ extern void ceph_flush_snaps(struct ceph_inode_info *ci, > >> extern bool __ceph_should_report_size(struct ceph_inode_info *ci); > >> extern void ceph_check_caps(struct ceph_inode_info *ci, int flags, > >> struct ceph_mds_session *session); > >> +static void inline > >> +ceph_async_check_caps(struct ceph_inode_info *ci) > >> +{ > >> + struct inode *inode = &ci->vfs_inode; > >> + > >> + /* It's okay if queue_work fails */ > >> + queue_work(ceph_inode_to_client(inode)->inode_wq, > >> + &ceph_inode(inode)->check_caps_work); > >> +} > >> + > >> extern void ceph_check_delayed_caps(struct ceph_mds_client *mdsc); > >> extern void ceph_flush_dirty_caps(struct ceph_mds_client *mdsc); > >> extern int ceph_drop_caps_for_unlink(struct inode *inode); > >> -- > >> 1.8.3.1 > >> >
