* [PATCH] ceph: clean up spinlocking and list handling around cleanup_cap_releases
@ 2017-10-19 12:53 Jeff Layton
2017-10-19 13:46 ` Yan, Zheng
2017-10-20 9:26 ` Ilya Dryomov
0 siblings, 2 replies; 3+ messages in thread
From: Jeff Layton @ 2017-10-19 12:53 UTC (permalink / raw)
To: zyan; +Cc: sage, idryomov, ceph-devel
From: Jeff Layton <jlayton@redhat.com>
Functions that release a lock taken in a parent frame are notoriously
hard to follow. Split cleanup_cap_releases into two functions, one to
detach the cap releases from the session (which should be called with
the spinlock held), and another to dispose of those caps.
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
fs/ceph/mds_client.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index f23c820daaed..d58cb17ed491 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1038,22 +1038,23 @@ void ceph_mdsc_open_export_target_sessions(struct ceph_mds_client *mdsc,
* session caps
*/
-/* caller holds s_cap_lock, we drop it */
-static void cleanup_cap_releases(struct ceph_mds_client *mdsc,
- struct ceph_mds_session *session)
- __releases(session->s_cap_lock)
+static void detach_cap_releases(struct ceph_mds_session *session,
+ struct list_head *target)
{
- LIST_HEAD(tmp_list);
- list_splice_init(&session->s_cap_releases, &tmp_list);
+ lockdep_assert_held(&session->s_cap_lock);
+
+ list_splice_init(&session->s_cap_releases, target);
session->s_num_cap_releases = 0;
- spin_unlock(&session->s_cap_lock);
+ dout("dispose_cap_releases mds%d\n", session->s_mds);
+}
- dout("cleanup_cap_releases mds%d\n", session->s_mds);
- while (!list_empty(&tmp_list)) {
+static void dispose_cap_releases(struct ceph_mds_client *mdsc,
+ struct list_head *dispose)
+{
+ while (!list_empty(dispose)) {
struct ceph_cap *cap;
/* zero out the in-progress message */
- cap = list_first_entry(&tmp_list,
- struct ceph_cap, session_caps);
+ cap = list_first_entry(dispose, struct ceph_cap, session_caps);
list_del(&cap->session_caps);
ceph_put_cap(mdsc, cap);
}
@@ -1243,6 +1244,8 @@ static void remove_session_caps(struct ceph_mds_session *session)
{
struct ceph_fs_client *fsc = session->s_mdsc->fsc;
struct super_block *sb = fsc->sb;
+ LIST_HEAD(dispose);
+
dout("remove_session_caps on %p\n", session);
iterate_session_caps(session, remove_session_caps_cb, fsc);
@@ -1277,10 +1280,12 @@ static void remove_session_caps(struct ceph_mds_session *session)
}
// drop cap expires and unlock s_cap_lock
- cleanup_cap_releases(session->s_mdsc, session);
+ detach_cap_releases(session, &dispose);
BUG_ON(session->s_nr_caps > 0);
BUG_ON(!list_empty(&session->s_cap_flushing));
+ spin_unlock(&session->s_cap_lock);
+ dispose_cap_releases(session->s_mdsc, &dispose);
}
/*
@@ -2992,6 +2997,7 @@ static void send_mds_reconnect(struct ceph_mds_client *mdsc,
int s_nr_caps;
struct ceph_pagelist *pagelist;
struct ceph_reconnect_state recon_state;
+ LIST_HEAD(dispose);
pr_info("mds%d reconnect start\n", mds);
@@ -3025,7 +3031,9 @@ static void send_mds_reconnect(struct ceph_mds_client *mdsc,
*/
session->s_cap_reconnect = 1;
/* drop old cap expires; we're about to reestablish that state */
- cleanup_cap_releases(mdsc, session);
+ detach_cap_releases(session, &dispose);
+ spin_unlock(&session->s_cap_lock);
+ dispose_cap_releases(mdsc, &dispose);
/* trim unused caps to reduce MDS's cache rejoin time */
if (mdsc->fsc->sb->s_root)
--
2.13.6
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ceph: clean up spinlocking and list handling around cleanup_cap_releases
2017-10-19 12:53 [PATCH] ceph: clean up spinlocking and list handling around cleanup_cap_releases Jeff Layton
@ 2017-10-19 13:46 ` Yan, Zheng
2017-10-20 9:26 ` Ilya Dryomov
1 sibling, 0 replies; 3+ messages in thread
From: Yan, Zheng @ 2017-10-19 13:46 UTC (permalink / raw)
To: Jeff Layton; +Cc: Sage Weil, Ilya Dryomov, ceph-devel
> On 19 Oct 2017, at 20:53, Jeff Layton <jlayton@kernel.org> wrote:
>
> From: Jeff Layton <jlayton@redhat.com>
>
> Functions that release a lock taken in a parent frame are notoriously
> hard to follow. Split cleanup_cap_releases into two functions, one to
> detach the cap releases from the session (which should be called with
> the spinlock held), and another to dispose of those caps.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/ceph/mds_client.c | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index f23c820daaed..d58cb17ed491 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -1038,22 +1038,23 @@ void ceph_mdsc_open_export_target_sessions(struct ceph_mds_client *mdsc,
> * session caps
> */
>
> -/* caller holds s_cap_lock, we drop it */
> -static void cleanup_cap_releases(struct ceph_mds_client *mdsc,
> - struct ceph_mds_session *session)
> - __releases(session->s_cap_lock)
> +static void detach_cap_releases(struct ceph_mds_session *session,
> + struct list_head *target)
> {
> - LIST_HEAD(tmp_list);
> - list_splice_init(&session->s_cap_releases, &tmp_list);
> + lockdep_assert_held(&session->s_cap_lock);
> +
> + list_splice_init(&session->s_cap_releases, target);
> session->s_num_cap_releases = 0;
> - spin_unlock(&session->s_cap_lock);
> + dout("dispose_cap_releases mds%d\n", session->s_mds);
> +}
>
> - dout("cleanup_cap_releases mds%d\n", session->s_mds);
> - while (!list_empty(&tmp_list)) {
> +static void dispose_cap_releases(struct ceph_mds_client *mdsc,
> + struct list_head *dispose)
> +{
> + while (!list_empty(dispose)) {
> struct ceph_cap *cap;
> /* zero out the in-progress message */
> - cap = list_first_entry(&tmp_list,
> - struct ceph_cap, session_caps);
> + cap = list_first_entry(dispose, struct ceph_cap, session_caps);
> list_del(&cap->session_caps);
> ceph_put_cap(mdsc, cap);
> }
> @@ -1243,6 +1244,8 @@ static void remove_session_caps(struct ceph_mds_session *session)
> {
> struct ceph_fs_client *fsc = session->s_mdsc->fsc;
> struct super_block *sb = fsc->sb;
> + LIST_HEAD(dispose);
> +
> dout("remove_session_caps on %p\n", session);
> iterate_session_caps(session, remove_session_caps_cb, fsc);
>
> @@ -1277,10 +1280,12 @@ static void remove_session_caps(struct ceph_mds_session *session)
> }
>
> // drop cap expires and unlock s_cap_lock
> - cleanup_cap_releases(session->s_mdsc, session);
> + detach_cap_releases(session, &dispose);
>
> BUG_ON(session->s_nr_caps > 0);
> BUG_ON(!list_empty(&session->s_cap_flushing));
> + spin_unlock(&session->s_cap_lock);
> + dispose_cap_releases(session->s_mdsc, &dispose);
> }
>
> /*
> @@ -2992,6 +2997,7 @@ static void send_mds_reconnect(struct ceph_mds_client *mdsc,
> int s_nr_caps;
> struct ceph_pagelist *pagelist;
> struct ceph_reconnect_state recon_state;
> + LIST_HEAD(dispose);
>
> pr_info("mds%d reconnect start\n", mds);
>
> @@ -3025,7 +3031,9 @@ static void send_mds_reconnect(struct ceph_mds_client *mdsc,
> */
> session->s_cap_reconnect = 1;
> /* drop old cap expires; we're about to reestablish that state */
> - cleanup_cap_releases(mdsc, session);
> + detach_cap_releases(session, &dispose);
> + spin_unlock(&session->s_cap_lock);
> + dispose_cap_releases(mdsc, &dispose);
>
> /* trim unused caps to reduce MDS's cache rejoin time */
> if (mdsc->fsc->sb->s_root)
> --
> 2.13.6
>
Reviewed-by: "Yan, Zheng" <zyan@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ceph: clean up spinlocking and list handling around cleanup_cap_releases
2017-10-19 12:53 [PATCH] ceph: clean up spinlocking and list handling around cleanup_cap_releases Jeff Layton
2017-10-19 13:46 ` Yan, Zheng
@ 2017-10-20 9:26 ` Ilya Dryomov
1 sibling, 0 replies; 3+ messages in thread
From: Ilya Dryomov @ 2017-10-20 9:26 UTC (permalink / raw)
To: Jeff Layton; +Cc: Yan, Zheng, Sage Weil, Ceph Development
On Thu, Oct 19, 2017 at 2:53 PM, Jeff Layton <jlayton@kernel.org> wrote:
> From: Jeff Layton <jlayton@redhat.com>
>
> Functions that release a lock taken in a parent frame are notoriously
> hard to follow. Split cleanup_cap_releases into two functions, one to
> detach the cap releases from the session (which should be called with
> the spinlock held), and another to dispose of those caps.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/ceph/mds_client.c | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index f23c820daaed..d58cb17ed491 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -1038,22 +1038,23 @@ void ceph_mdsc_open_export_target_sessions(struct ceph_mds_client *mdsc,
> * session caps
> */
>
> -/* caller holds s_cap_lock, we drop it */
> -static void cleanup_cap_releases(struct ceph_mds_client *mdsc,
> - struct ceph_mds_session *session)
> - __releases(session->s_cap_lock)
> +static void detach_cap_releases(struct ceph_mds_session *session,
> + struct list_head *target)
> {
> - LIST_HEAD(tmp_list);
> - list_splice_init(&session->s_cap_releases, &tmp_list);
> + lockdep_assert_held(&session->s_cap_lock);
> +
> + list_splice_init(&session->s_cap_releases, target);
> session->s_num_cap_releases = 0;
> - spin_unlock(&session->s_cap_lock);
> + dout("dispose_cap_releases mds%d\n", session->s_mds);
> +}
>
> - dout("cleanup_cap_releases mds%d\n", session->s_mds);
> - while (!list_empty(&tmp_list)) {
> +static void dispose_cap_releases(struct ceph_mds_client *mdsc,
> + struct list_head *dispose)
> +{
> + while (!list_empty(dispose)) {
> struct ceph_cap *cap;
> /* zero out the in-progress message */
> - cap = list_first_entry(&tmp_list,
> - struct ceph_cap, session_caps);
> + cap = list_first_entry(dispose, struct ceph_cap, session_caps);
> list_del(&cap->session_caps);
> ceph_put_cap(mdsc, cap);
> }
> @@ -1243,6 +1244,8 @@ static void remove_session_caps(struct ceph_mds_session *session)
> {
> struct ceph_fs_client *fsc = session->s_mdsc->fsc;
> struct super_block *sb = fsc->sb;
> + LIST_HEAD(dispose);
> +
> dout("remove_session_caps on %p\n", session);
> iterate_session_caps(session, remove_session_caps_cb, fsc);
>
> @@ -1277,10 +1280,12 @@ static void remove_session_caps(struct ceph_mds_session *session)
> }
>
> // drop cap expires and unlock s_cap_lock
> - cleanup_cap_releases(session->s_mdsc, session);
> + detach_cap_releases(session, &dispose);
>
> BUG_ON(session->s_nr_caps > 0);
> BUG_ON(!list_empty(&session->s_cap_flushing));
> + spin_unlock(&session->s_cap_lock);
> + dispose_cap_releases(session->s_mdsc, &dispose);
> }
>
> /*
> @@ -2992,6 +2997,7 @@ static void send_mds_reconnect(struct ceph_mds_client *mdsc,
> int s_nr_caps;
> struct ceph_pagelist *pagelist;
> struct ceph_reconnect_state recon_state;
> + LIST_HEAD(dispose);
>
> pr_info("mds%d reconnect start\n", mds);
>
> @@ -3025,7 +3031,9 @@ static void send_mds_reconnect(struct ceph_mds_client *mdsc,
> */
> session->s_cap_reconnect = 1;
> /* drop old cap expires; we're about to reestablish that state */
> - cleanup_cap_releases(mdsc, session);
> + detach_cap_releases(session, &dispose);
> + spin_unlock(&session->s_cap_lock);
> + dispose_cap_releases(mdsc, &dispose);
>
> /* trim unused caps to reduce MDS's cache rejoin time */
> if (mdsc->fsc->sb->s_root)
Applied.
Thanks,
Ilya
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-20 9:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-19 12:53 [PATCH] ceph: clean up spinlocking and list handling around cleanup_cap_releases Jeff Layton
2017-10-19 13:46 ` Yan, Zheng
2017-10-20 9:26 ` Ilya Dryomov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.