* [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() @ 2020-04-28 13:13 Wu Bo 2020-04-28 14:48 ` Jeff Layton 0 siblings, 1 reply; 6+ messages in thread From: Wu Bo @ 2020-04-28 13:13 UTC (permalink / raw) To: jlayton, sage, idryomov Cc: ceph-devel, linux-kernel, liuzhiqiang26, linfeilong, wubo40 if the ceph_mdsc_open_export_target_session() return fails, should add a lock to avoid twice unlocking. Because the lock will be released at the retry or out_unlock tag. -- v1 -> v2: add spin_lock(&ci->i_ceph_lock) before goto out_unlock tag. Signed-off-by: Wu Bo <wubo40@huawei.com> --- fs/ceph/caps.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 185db76..414c0e2 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -3731,22 +3731,25 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, /* open target session */ tsession = ceph_mdsc_open_export_target_session(mdsc, target); - if (!IS_ERR(tsession)) { - if (mds > target) { - mutex_lock(&session->s_mutex); - mutex_lock_nested(&tsession->s_mutex, - SINGLE_DEPTH_NESTING); - } else { - mutex_lock(&tsession->s_mutex); - mutex_lock_nested(&session->s_mutex, - SINGLE_DEPTH_NESTING); - } - new_cap = ceph_get_cap(mdsc, NULL); - } else { + if (IS_ERR(tsession)) { WARN_ON(1); tsession = NULL; target = -1; + mutex_lock(&session->s_mutex); + spin_lock(&ci->i_ceph_lock); + goto out_unlock; + } + + if (mds > target) { + mutex_lock(&session->s_mutex); + mutex_lock_nested(&tsession->s_mutex, + SINGLE_DEPTH_NESTING); + } else { + mutex_lock(&tsession->s_mutex); + mutex_lock_nested(&session->s_mutex, + SINGLE_DEPTH_NESTING); } + new_cap = ceph_get_cap(mdsc, NULL); goto retry; out_unlock: -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() 2020-04-28 13:13 [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() Wu Bo @ 2020-04-28 14:48 ` Jeff Layton 2020-04-29 0:46 ` Wu Bo 0 siblings, 1 reply; 6+ messages in thread From: Jeff Layton @ 2020-04-28 14:48 UTC (permalink / raw) To: Wu Bo, sage, idryomov; +Cc: ceph-devel, linux-kernel, liuzhiqiang26, linfeilong On Tue, 2020-04-28 at 21:13 +0800, Wu Bo wrote: > if the ceph_mdsc_open_export_target_session() return fails, > should add a lock to avoid twice unlocking. > Because the lock will be released at the retry or out_unlock tag. > The problem looks real, but... > -- > v1 -> v2: > add spin_lock(&ci->i_ceph_lock) before goto out_unlock tag. > > Signed-off-by: Wu Bo <wubo40@huawei.com> > --- > fs/ceph/caps.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 185db76..414c0e2 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -3731,22 +3731,25 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, > > /* open target session */ > tsession = ceph_mdsc_open_export_target_session(mdsc, target); > - if (!IS_ERR(tsession)) { > - if (mds > target) { > - mutex_lock(&session->s_mutex); > - mutex_lock_nested(&tsession->s_mutex, > - SINGLE_DEPTH_NESTING); > - } else { > - mutex_lock(&tsession->s_mutex); > - mutex_lock_nested(&session->s_mutex, > - SINGLE_DEPTH_NESTING); > - } > - new_cap = ceph_get_cap(mdsc, NULL); > - } else { > + if (IS_ERR(tsession)) { > WARN_ON(1); > tsession = NULL; > target = -1; > + mutex_lock(&session->s_mutex); > + spin_lock(&ci->i_ceph_lock); > + goto out_unlock; Why did you make this case goto out_unlock instead of retrying as it did before? > + } > + > + if (mds > target) { > + mutex_lock(&session->s_mutex); > + mutex_lock_nested(&tsession->s_mutex, > + SINGLE_DEPTH_NESTING); > + } else { > + mutex_lock(&tsession->s_mutex); > + mutex_lock_nested(&session->s_mutex, > + SINGLE_DEPTH_NESTING); > } > + new_cap = ceph_get_cap(mdsc, NULL); > goto retry; > > out_unlock: -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() 2020-04-28 14:48 ` Jeff Layton @ 2020-04-29 0:46 ` Wu Bo 2020-04-29 15:31 ` Jeff Layton 2020-04-30 2:50 ` Yan, Zheng 0 siblings, 2 replies; 6+ messages in thread From: Wu Bo @ 2020-04-29 0:46 UTC (permalink / raw) To: Jeff Layton, sage, idryomov Cc: ceph-devel, linux-kernel, liuzhiqiang26, linfeilong On 2020/4/28 22:48, Jeff Layton wrote: > On Tue, 2020-04-28 at 21:13 +0800, Wu Bo wrote: >> if the ceph_mdsc_open_export_target_session() return fails, >> should add a lock to avoid twice unlocking. >> Because the lock will be released at the retry or out_unlock tag. >> > > The problem looks real, but... > >> -- >> v1 -> v2: >> add spin_lock(&ci->i_ceph_lock) before goto out_unlock tag. >> >> Signed-off-by: Wu Bo <wubo40@huawei.com> >> --- >> fs/ceph/caps.c | 27 +++++++++++++++------------ >> 1 file changed, 15 insertions(+), 12 deletions(-) >> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >> index 185db76..414c0e2 100644 >> --- a/fs/ceph/caps.c >> +++ b/fs/ceph/caps.c >> @@ -3731,22 +3731,25 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, >> >> /* open target session */ >> tsession = ceph_mdsc_open_export_target_session(mdsc, target); >> - if (!IS_ERR(tsession)) { >> - if (mds > target) { >> - mutex_lock(&session->s_mutex); >> - mutex_lock_nested(&tsession->s_mutex, >> - SINGLE_DEPTH_NESTING); >> - } else { >> - mutex_lock(&tsession->s_mutex); >> - mutex_lock_nested(&session->s_mutex, >> - SINGLE_DEPTH_NESTING); >> - } >> - new_cap = ceph_get_cap(mdsc, NULL); >> - } else { >> + if (IS_ERR(tsession)) { >> WARN_ON(1); >> tsession = NULL; >> target = -1; >> + mutex_lock(&session->s_mutex); >> + spin_lock(&ci->i_ceph_lock); >> + goto out_unlock; > > Why did you make this case goto out_unlock instead of retrying as it did > before? > If the problem occurs, target = -1, and goto retry lable, you need to call __get_cap_for_mds() or even call __ceph_remove_cap(), and then jump to out_unlock lable. All I think is unnecessary, goto out_unlock instead of retrying directly. Thanks. Wu Bo >> + } >> + >> + if (mds > target) { >> + mutex_lock(&session->s_mutex); >> + mutex_lock_nested(&tsession->s_mutex, >> + SINGLE_DEPTH_NESTING); >> + } else { >> + mutex_lock(&tsession->s_mutex); >> + mutex_lock_nested(&session->s_mutex, >> + SINGLE_DEPTH_NESTING); >> } >> + new_cap = ceph_get_cap(mdsc, NULL); >> goto retry; >> >> out_unlock: > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() 2020-04-29 0:46 ` Wu Bo @ 2020-04-29 15:31 ` Jeff Layton 2020-04-30 2:50 ` Yan, Zheng 1 sibling, 0 replies; 6+ messages in thread From: Jeff Layton @ 2020-04-29 15:31 UTC (permalink / raw) To: Wu Bo, sage, idryomov, Yan, Zheng Cc: ceph-devel, linux-kernel, liuzhiqiang26, linfeilong On Wed, 2020-04-29 at 08:46 +0800, Wu Bo wrote: > On 2020/4/28 22:48, Jeff Layton wrote: > > On Tue, 2020-04-28 at 21:13 +0800, Wu Bo wrote: > > > if the ceph_mdsc_open_export_target_session() return fails, > > > should add a lock to avoid twice unlocking. > > > Because the lock will be released at the retry or out_unlock tag. > > > > > > > The problem looks real, but... > > > > > -- > > > v1 -> v2: > > > add spin_lock(&ci->i_ceph_lock) before goto out_unlock tag. > > > > > > Signed-off-by: Wu Bo <wubo40@huawei.com> > > > --- > > > fs/ceph/caps.c | 27 +++++++++++++++------------ > > > 1 file changed, 15 insertions(+), 12 deletions(-) > > > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > > index 185db76..414c0e2 100644 > > > --- a/fs/ceph/caps.c > > > +++ b/fs/ceph/caps.c > > > @@ -3731,22 +3731,25 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, > > > > > > /* open target session */ > > > tsession = ceph_mdsc_open_export_target_session(mdsc, target); > > > - if (!IS_ERR(tsession)) { > > > - if (mds > target) { > > > - mutex_lock(&session->s_mutex); > > > - mutex_lock_nested(&tsession->s_mutex, > > > - SINGLE_DEPTH_NESTING); > > > - } else { > > > - mutex_lock(&tsession->s_mutex); > > > - mutex_lock_nested(&session->s_mutex, > > > - SINGLE_DEPTH_NESTING); > > > - } > > > - new_cap = ceph_get_cap(mdsc, NULL); > > > - } else { > > > + if (IS_ERR(tsession)) { > > > WARN_ON(1); > > > tsession = NULL; > > > target = -1; > > > + mutex_lock(&session->s_mutex); > > > + spin_lock(&ci->i_ceph_lock); Rather than taking the spinlock here, it'd be nicer to set a new label above the mutex (out_unlock_mutex or something) and jump to that. > > > + goto out_unlock; > > > > Why did you make this case goto out_unlock instead of retrying as it did > > before? > > > > If the problem occurs, target = -1, and goto retry lable, you need to > call __get_cap_for_mds() or even call __ceph_remove_cap(), and then jump > to out_unlock lable. All I think is unnecessary, goto out_unlock instead > of retrying directly. > (cc'ing Zheng since he understands the IMPORT/EXPORT code better than I) I'm not quite convinced. It certainly looks like this was done deliberately before, and that the expectation is that the cap be removed in this case. If we do want to make this change, then at the very least the changelog needs to spell out why this safe and desirable. > > > + } > > > + > > > + if (mds > target) { > > > + mutex_lock(&session->s_mutex); > > > + mutex_lock_nested(&tsession->s_mutex, > > > + SINGLE_DEPTH_NESTING); > > > + } else { > > > + mutex_lock(&tsession->s_mutex); > > > + mutex_lock_nested(&session->s_mutex, > > > + SINGLE_DEPTH_NESTING); > > > } > > > + new_cap = ceph_get_cap(mdsc, NULL); > > > goto retry; > > > > > > out_unlock: > > -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() 2020-04-29 0:46 ` Wu Bo 2020-04-29 15:31 ` Jeff Layton @ 2020-04-30 2:50 ` Yan, Zheng 2020-04-30 4:31 ` Wu Bo 1 sibling, 1 reply; 6+ messages in thread From: Yan, Zheng @ 2020-04-30 2:50 UTC (permalink / raw) To: Wu Bo Cc: Jeff Layton, Sage Weil, Ilya Dryomov, ceph-devel, Linux Kernel Mailing List, liuzhiqiang26, linfeilong On Wed, Apr 29, 2020 at 8:49 AM Wu Bo <wubo40@huawei.com> wrote: > > On 2020/4/28 22:48, Jeff Layton wrote: > > On Tue, 2020-04-28 at 21:13 +0800, Wu Bo wrote: > >> if the ceph_mdsc_open_export_target_session() return fails, > >> should add a lock to avoid twice unlocking. > >> Because the lock will be released at the retry or out_unlock tag. > >> > > > > The problem looks real, but... > > > >> -- > >> v1 -> v2: > >> add spin_lock(&ci->i_ceph_lock) before goto out_unlock tag. > >> > >> Signed-off-by: Wu Bo <wubo40@huawei.com> > >> --- > >> fs/ceph/caps.c | 27 +++++++++++++++------------ > >> 1 file changed, 15 insertions(+), 12 deletions(-) > >> > >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > >> index 185db76..414c0e2 100644 > >> --- a/fs/ceph/caps.c > >> +++ b/fs/ceph/caps.c > >> @@ -3731,22 +3731,25 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, > >> > >> /* open target session */ > >> tsession = ceph_mdsc_open_export_target_session(mdsc, target); > >> - if (!IS_ERR(tsession)) { > >> - if (mds > target) { > >> - mutex_lock(&session->s_mutex); > >> - mutex_lock_nested(&tsession->s_mutex, > >> - SINGLE_DEPTH_NESTING); > >> - } else { > >> - mutex_lock(&tsession->s_mutex); > >> - mutex_lock_nested(&session->s_mutex, > >> - SINGLE_DEPTH_NESTING); > >> - } > >> - new_cap = ceph_get_cap(mdsc, NULL); > >> - } else { > >> + if (IS_ERR(tsession)) { > >> WARN_ON(1); > >> tsession = NULL; > >> target = -1; > >> + mutex_lock(&session->s_mutex); > >> + spin_lock(&ci->i_ceph_lock); > >> + goto out_unlock; > > > > Why did you make this case goto out_unlock instead of retrying as it did > > before? > > > > If the problem occurs, target = -1, and goto retry lable, you need to > call __get_cap_for_mds() or even call __ceph_remove_cap(), and then jump > to out_unlock lable. All I think is unnecessary, goto out_unlock instead > of retrying directly. > __ceph_remove_cap() must be called even if opening target session failed. I think adding a mutex_lock(&session->s_mutex) to the IS_ERR(tsession) block should be enough. > Thanks. > Wu Bo > > >> + } > >> + > >> + if (mds > target) { > >> + mutex_lock(&session->s_mutex); > >> + mutex_lock_nested(&tsession->s_mutex, > >> + SINGLE_DEPTH_NESTING); > >> + } else { > >> + mutex_lock(&tsession->s_mutex); > >> + mutex_lock_nested(&session->s_mutex, > >> + SINGLE_DEPTH_NESTING); > >> } > >> + new_cap = ceph_get_cap(mdsc, NULL); > >> goto retry; > >> > >> out_unlock: > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() 2020-04-30 2:50 ` Yan, Zheng @ 2020-04-30 4:31 ` Wu Bo 0 siblings, 0 replies; 6+ messages in thread From: Wu Bo @ 2020-04-30 4:31 UTC (permalink / raw) To: Yan, Zheng Cc: Jeff Layton, Sage Weil, Ilya Dryomov, ceph-devel, Linux Kernel Mailing List, liuzhiqiang26, linfeilong On 2020/4/30 10:50, Yan, Zheng wrote: > On Wed, Apr 29, 2020 at 8:49 AM Wu Bo <wubo40@huawei.com> wrote: >> >> On 2020/4/28 22:48, Jeff Layton wrote: >>> On Tue, 2020-04-28 at 21:13 +0800, Wu Bo wrote: >>>> if the ceph_mdsc_open_export_target_session() return fails, >>>> should add a lock to avoid twice unlocking. >>>> Because the lock will be released at the retry or out_unlock tag. >>>> >>> >>> The problem looks real, but... >>> >>>> -- >>>> v1 -> v2: >>>> add spin_lock(&ci->i_ceph_lock) before goto out_unlock tag. >>>> >>>> Signed-off-by: Wu Bo <wubo40@huawei.com> >>>> --- >>>> fs/ceph/caps.c | 27 +++++++++++++++------------ >>>> 1 file changed, 15 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >>>> index 185db76..414c0e2 100644 >>>> --- a/fs/ceph/caps.c >>>> +++ b/fs/ceph/caps.c >>>> @@ -3731,22 +3731,25 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, >>>> >>>> /* open target session */ >>>> tsession = ceph_mdsc_open_export_target_session(mdsc, target); >>>> - if (!IS_ERR(tsession)) { >>>> - if (mds > target) { >>>> - mutex_lock(&session->s_mutex); >>>> - mutex_lock_nested(&tsession->s_mutex, >>>> - SINGLE_DEPTH_NESTING); >>>> - } else { >>>> - mutex_lock(&tsession->s_mutex); >>>> - mutex_lock_nested(&session->s_mutex, >>>> - SINGLE_DEPTH_NESTING); >>>> - } >>>> - new_cap = ceph_get_cap(mdsc, NULL); >>>> - } else { >>>> + if (IS_ERR(tsession)) { >>>> WARN_ON(1); >>>> tsession = NULL; >>>> target = -1; >>>> + mutex_lock(&session->s_mutex); >>>> + spin_lock(&ci->i_ceph_lock); >>>> + goto out_unlock; >>> >>> Why did you make this case goto out_unlock instead of retrying as it did >>> before? >>> >> >> If the problem occurs, target = -1, and goto retry lable, you need to >> call __get_cap_for_mds() or even call __ceph_remove_cap(), and then jump >> to out_unlock lable. All I think is unnecessary, goto out_unlock instead >> of retrying directly. >> > > __ceph_remove_cap() must be called even if opening target session > failed. I think adding a mutex_lock(&session->s_mutex) to the > IS_ERR(tsession) block should be enough. > Yes，I will send the V3 patch later. > >> Thanks. >> Wu Bo >> >>>> + } >>>> + >>>> + if (mds > target) { >>>> + mutex_lock(&session->s_mutex); >>>> + mutex_lock_nested(&tsession->s_mutex, >>>> + SINGLE_DEPTH_NESTING); >>>> + } else { >>>> + mutex_lock(&tsession->s_mutex); >>>> + mutex_lock_nested(&session->s_mutex, >>>> + SINGLE_DEPTH_NESTING); >>>> } >>>> + new_cap = ceph_get_cap(mdsc, NULL); >>>> goto retry; >>>> >>>> out_unlock: >>> >> >> > > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-30 4:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 13:13 [PATCH V2] fs/ceph:fix double unlock in handle_cap_export() Wu Bo 2020-04-28 14:48 ` Jeff Layton 2020-04-29 0:46 ` Wu Bo 2020-04-29 15:31 ` Jeff Layton 2020-04-30 2:50 ` Yan, Zheng 2020-04-30 4:31 ` Wu Bo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).