| Message ID | 1520941940-215581-1-git-send-email-cgxu519@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 13, 2018 at 7:52 PM, Chengguang Xu <cgxu519@gmx.com> wrote: > Obviously wrong mutex lock/unlock for nothing. > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > --- > fs/ceph/mds_client.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 8fc37a8..5439dfd 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -3514,8 +3514,6 @@ static void drop_leases(struct ceph_mds_client *mdsc) > if (!s) > continue; > mutex_unlock(&mdsc->mutex); > - mutex_lock(&s->s_mutex); > - mutex_unlock(&s->s_mutex); > ceph_put_mds_session(s); > mutex_lock(&mdsc->mutex); > } > -- > 1.8.3.1 > Applied, thanks Yan, Zheng > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 15 Mar 2018, at 02:55, Gregory Farnum <gfarnum@redhat.com> wrote: > > On Wed, Mar 14, 2018 at 1:14 AM Yan, Zheng <ukernel@gmail.com> wrote: > On Tue, Mar 13, 2018 at 7:52 PM, Chengguang Xu <cgxu519@gmx.com> wrote: > > Obviously wrong mutex lock/unlock for nothing. > > > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > > --- > > fs/ceph/mds_client.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > index 8fc37a8..5439dfd 100644 > > --- a/fs/ceph/mds_client.c > > +++ b/fs/ceph/mds_client.c > > @@ -3514,8 +3514,6 @@ static void drop_leases(struct ceph_mds_client *mdsc) > > if (!s) > > continue; > > mutex_unlock(&mdsc->mutex); > > - mutex_lock(&s->s_mutex); > > - mutex_unlock(&s->s_mutex); > > ceph_put_mds_session(s); > > mutex_lock(&mdsc->mutex); > > } > > -- > > 1.8.3.1 > > > Applied, thanks > Yan, Zheng > > So I'm sure it's not the case, but when I saw this go by I was assuming that the mutex grab is to make sure we don't have anybody still running in a critical section that assumes those caps are held. Is that not an issue and/or definitely not the case? :) > -Greg I think you are right. Thanks Yan, Zheng -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 14, 2018 at 4:58 PM, Yan, Zheng <zyan@redhat.com> wrote: > > >> On 15 Mar 2018, at 02:55, Gregory Farnum <gfarnum@redhat.com> wrote: >> >> On Wed, Mar 14, 2018 at 1:14 AM Yan, Zheng <ukernel@gmail.com> wrote: >> On Tue, Mar 13, 2018 at 7:52 PM, Chengguang Xu <cgxu519@gmx.com> wrote: >> > Obviously wrong mutex lock/unlock for nothing. >> > >> > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >> > --- >> > fs/ceph/mds_client.c | 2 -- >> > 1 file changed, 2 deletions(-) >> > >> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >> > index 8fc37a8..5439dfd 100644 >> > --- a/fs/ceph/mds_client.c >> > +++ b/fs/ceph/mds_client.c >> > @@ -3514,8 +3514,6 @@ static void drop_leases(struct ceph_mds_client *mdsc) >> > if (!s) >> > continue; >> > mutex_unlock(&mdsc->mutex); >> > - mutex_lock(&s->s_mutex); >> > - mutex_unlock(&s->s_mutex); >> > ceph_put_mds_session(s); >> > mutex_lock(&mdsc->mutex); >> > } >> > -- >> > 1.8.3.1 >> > >> Applied, thanks >> Yan, Zheng >> >> So I'm sure it's not the case, but when I saw this go by I was assuming that the mutex grab is to make sure we don't have anybody still running in a critical section that assumes those caps are held. Is that not an issue and/or definitely not the case? :) >> -Greg > > I think you are right. Thanks In that case, it needs a comment. :)
> Sent: Thursday, March 15, 2018 at 7:58 AM > From: "Yan, Zheng" <zyan@redhat.com> > To: "Gregory Farnum" <gfarnum@redhat.com> > Cc: "Zheng Yan" <ukernel@gmail.com>, "Chengguang Xu" <cgxu519@gmx.com>, "Ilya Dryomov" <idryomov@gmail.com>, ceph-devel <ceph-devel@vger.kernel.org> > Subject: Re: [PATCH] ceph: delete unnecessary mutex lock/unlock > > > > > On 15 Mar 2018, at 02:55, Gregory Farnum <gfarnum@redhat.com> wrote: > > > > On Wed, Mar 14, 2018 at 1:14 AM Yan, Zheng <ukernel@gmail.com> wrote: > > On Tue, Mar 13, 2018 at 7:52 PM, Chengguang Xu <cgxu519@gmx.com> wrote: > > > Obviously wrong mutex lock/unlock for nothing. > > > > > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > > > --- > > > fs/ceph/mds_client.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > > index 8fc37a8..5439dfd 100644 > > > --- a/fs/ceph/mds_client.c > > > +++ b/fs/ceph/mds_client.c > > > @@ -3514,8 +3514,6 @@ static void drop_leases(struct ceph_mds_client *mdsc) > > > if (!s) > > > continue; > > > mutex_unlock(&mdsc->mutex); > > > - mutex_lock(&s->s_mutex); > > > - mutex_unlock(&s->s_mutex); > > > ceph_put_mds_session(s); > > > mutex_lock(&mdsc->mutex); > > > } > > > -- > > > 1.8.3.1 > > > > > Applied, thanks > > Yan, Zheng > > > > So I'm sure it's not the case, but when I saw this go by I was assuming that the mutex grab is to make sure we don't have anybody still running in a critical section that assumes those caps are held. Is that not an issue and/or definitely not the case? :) I understand the purpose of the lock here now, but it does not always look safe. Won't any bad things happen by timing? > > -Greg > > I think you are right. Thanks > > Yan, Zheng > > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 15, 2018 at 2:06 PM, Chengguang Xu <cgxu519@gmx.com> wrote: >> Sent: Thursday, March 15, 2018 at 7:58 AM >> From: "Yan, Zheng" <zyan@redhat.com> >> To: "Gregory Farnum" <gfarnum@redhat.com> >> Cc: "Zheng Yan" <ukernel@gmail.com>, "Chengguang Xu" <cgxu519@gmx.com>, "Ilya Dryomov" <idryomov@gmail.com>, ceph-devel <ceph-devel@vger.kernel.org> >> Subject: Re: [PATCH] ceph: delete unnecessary mutex lock/unlock >> >> >> >> > On 15 Mar 2018, at 02:55, Gregory Farnum <gfarnum@redhat.com> wrote: >> > >> > On Wed, Mar 14, 2018 at 1:14 AM Yan, Zheng <ukernel@gmail.com> wrote: >> > On Tue, Mar 13, 2018 at 7:52 PM, Chengguang Xu <cgxu519@gmx.com> wrote: >> > > Obviously wrong mutex lock/unlock for nothing. >> > > >> > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >> > > --- >> > > fs/ceph/mds_client.c | 2 -- >> > > 1 file changed, 2 deletions(-) >> > > >> > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >> > > index 8fc37a8..5439dfd 100644 >> > > --- a/fs/ceph/mds_client.c >> > > +++ b/fs/ceph/mds_client.c >> > > @@ -3514,8 +3514,6 @@ static void drop_leases(struct ceph_mds_client *mdsc) >> > > if (!s) >> > > continue; >> > > mutex_unlock(&mdsc->mutex); >> > > - mutex_lock(&s->s_mutex); >> > > - mutex_unlock(&s->s_mutex); >> > > ceph_put_mds_session(s); >> > > mutex_lock(&mdsc->mutex); >> > > } >> > > -- >> > > 1.8.3.1 >> > > >> > Applied, thanks >> > Yan, Zheng >> > >> > So I'm sure it's not the case, but when I saw this go by I was assuming that the mutex grab is to make sure we don't have anybody still running in a critical section that assumes those caps are held. Is that not an issue and/or definitely not the case? :) > > I understand the purpose of the lock here now, but it does not always look safe. > Won't any bad things happen by timing? What timing are you concerned about? The requirement in this function is to drop our leases and make sure there are no users left. By updating the data structures we prevent new users; by locking the mutex we ensure there are no concurrent threads still making use of the leases. -Greg -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 8fc37a8..5439dfd 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -3514,8 +3514,6 @@ static void drop_leases(struct ceph_mds_client *mdsc) if (!s) continue; mutex_unlock(&mdsc->mutex); - mutex_lock(&s->s_mutex); - mutex_unlock(&s->s_mutex); ceph_put_mds_session(s); mutex_lock(&mdsc->mutex); }
Obviously wrong mutex lock/unlock for nothing. Signed-off-by: Chengguang Xu <cgxu519@gmx.com> --- fs/ceph/mds_client.c | 2 -- 1 file changed, 2 deletions(-)