diff mbox series

[1/3] ceph: add some lockdep assertions around snaprealm handling

Message ID 20210603165231.110559-2-jlayton@kernel.org
State New
Headers show
Series ceph: locking fixes for snaprealm handling | expand

Commit Message

Jeff Layton June 3, 2021, 4:52 p.m. UTC 
Turn some comments into lockdep asserts.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/snap.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Ilya Dryomov June 21, 2021, 8:05 p.m. UTC | #1 
On Thu, Jun 3, 2021 at 6:52 PM Jeff Layton <jlayton@kernel.org> wrote:
>

> Turn some comments into lockdep asserts.

>

> Signed-off-by: Jeff Layton <jlayton@kernel.org>

> ---

>  fs/ceph/snap.c | 16 ++++++++++++++++

>  1 file changed, 16 insertions(+)

>

> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c

> index 2a63fb37778b..bc6c33d485e6 100644

> --- a/fs/ceph/snap.c

> +++ b/fs/ceph/snap.c

> @@ -65,6 +65,8 @@

>  void ceph_get_snap_realm(struct ceph_mds_client *mdsc,

>                          struct ceph_snap_realm *realm)

>  {

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> +

>         dout("get_realm %p %d -> %d\n", realm,

>              atomic_read(&realm->nref), atomic_read(&realm->nref)+1);

>         /*

> @@ -113,6 +115,8 @@ static struct ceph_snap_realm *ceph_create_snap_realm(

>  {

>         struct ceph_snap_realm *realm;

>

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> +

>         realm = kzalloc(sizeof(*realm), GFP_NOFS);

>         if (!realm)

>                 return ERR_PTR(-ENOMEM);

> @@ -143,6 +147,8 @@ static struct ceph_snap_realm *__lookup_snap_realm(struct ceph_mds_client *mdsc,

>         struct rb_node *n = mdsc->snap_realms.rb_node;

>         struct ceph_snap_realm *r;

>

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> +

>         while (n) {

>                 r = rb_entry(n, struct ceph_snap_realm, node);

>                 if (ino < r->ino)

> @@ -176,6 +182,8 @@ static void __put_snap_realm(struct ceph_mds_client *mdsc,

>  static void __destroy_snap_realm(struct ceph_mds_client *mdsc,

>                                  struct ceph_snap_realm *realm)

>  {

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> +

>         dout("__destroy_snap_realm %p %llx\n", realm, realm->ino);

>

>         rb_erase(&realm->node, &mdsc->snap_realms);

> @@ -198,6 +206,8 @@ static void __destroy_snap_realm(struct ceph_mds_client *mdsc,

>  static void __put_snap_realm(struct ceph_mds_client *mdsc,

>                              struct ceph_snap_realm *realm)

>  {

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);


This one appears to be redundant since the only caller is
__destroy_snap_realm().

> +

>         dout("__put_snap_realm %llx %p %d -> %d\n", realm->ino, realm,

>              atomic_read(&realm->nref), atomic_read(&realm->nref)-1);

>         if (atomic_dec_and_test(&realm->nref))

> @@ -236,6 +246,8 @@ static void __cleanup_empty_realms(struct ceph_mds_client *mdsc)

>  {

>         struct ceph_snap_realm *realm;

>

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);


This too since it boils down to calling __destroy_snap_realm().

> +

>         spin_lock(&mdsc->snap_empty_lock);

>         while (!list_empty(&mdsc->snap_empty)) {

>                 realm = list_first_entry(&mdsc->snap_empty,

> @@ -269,6 +281,8 @@ static int adjust_snap_realm_parent(struct ceph_mds_client *mdsc,

>  {

>         struct ceph_snap_realm *parent;

>

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);


And this since ceph_lookup_snap_realm() is called right away.

> +

>         if (realm->parent_ino == parentino)

>                 return 0;

>

> @@ -696,6 +710,8 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,

>         int err = -ENOMEM;

>         LIST_HEAD(dirty_realms);

>

> +       lockdep_assert_held_write(&mdsc->snap_rwsem);


Ditto.

Thanks,

                Ilya
Jeff Layton June 21, 2021, 8:51 p.m. UTC | #2 
On Mon, 2021-06-21 at 22:05 +0200, Ilya Dryomov wrote:
> On Thu, Jun 3, 2021 at 6:52 PM Jeff Layton <jlayton@kernel.org> wrote:

> > 

> > Turn some comments into lockdep asserts.

> > 

> > Signed-off-by: Jeff Layton <jlayton@kernel.org>

> > ---

> >  fs/ceph/snap.c | 16 ++++++++++++++++

> >  1 file changed, 16 insertions(+)

> > 

> > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c

> > index 2a63fb37778b..bc6c33d485e6 100644

> > --- a/fs/ceph/snap.c

> > +++ b/fs/ceph/snap.c

> > @@ -65,6 +65,8 @@

> >  void ceph_get_snap_realm(struct ceph_mds_client *mdsc,

> >                          struct ceph_snap_realm *realm)

> >  {

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > +

> >         dout("get_realm %p %d -> %d\n", realm,

> >              atomic_read(&realm->nref), atomic_read(&realm->nref)+1);

> >         /*

> > @@ -113,6 +115,8 @@ static struct ceph_snap_realm *ceph_create_snap_realm(

> >  {

> >         struct ceph_snap_realm *realm;

> > 

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > +

> >         realm = kzalloc(sizeof(*realm), GFP_NOFS);

> >         if (!realm)

> >                 return ERR_PTR(-ENOMEM);

> > @@ -143,6 +147,8 @@ static struct ceph_snap_realm *__lookup_snap_realm(struct ceph_mds_client *mdsc,

> >         struct rb_node *n = mdsc->snap_realms.rb_node;

> >         struct ceph_snap_realm *r;

> > 

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > +

> >         while (n) {

> >                 r = rb_entry(n, struct ceph_snap_realm, node);

> >                 if (ino < r->ino)

> > @@ -176,6 +182,8 @@ static void __put_snap_realm(struct ceph_mds_client *mdsc,

> >  static void __destroy_snap_realm(struct ceph_mds_client *mdsc,

> >                                  struct ceph_snap_realm *realm)

> >  {

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > +

> >         dout("__destroy_snap_realm %p %llx\n", realm, realm->ino);

> > 

> >         rb_erase(&realm->node, &mdsc->snap_realms);

> > @@ -198,6 +206,8 @@ static void __destroy_snap_realm(struct ceph_mds_client *mdsc,

> >  static void __put_snap_realm(struct ceph_mds_client *mdsc,

> >                              struct ceph_snap_realm *realm)

> >  {

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> 

> This one appears to be redundant since the only caller is

> __destroy_snap_realm().

> 

> > +

> >         dout("__put_snap_realm %llx %p %d -> %d\n", realm->ino, realm,

> >              atomic_read(&realm->nref), atomic_read(&realm->nref)-1);

> >         if (atomic_dec_and_test(&realm->nref))

> > @@ -236,6 +246,8 @@ static void __cleanup_empty_realms(struct ceph_mds_client *mdsc)

> >  {

> >         struct ceph_snap_realm *realm;

> > 

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> 

> This too since it boils down to calling __destroy_snap_realm().

> 

> > +

> >         spin_lock(&mdsc->snap_empty_lock);

> >         while (!list_empty(&mdsc->snap_empty)) {

> >                 realm = list_first_entry(&mdsc->snap_empty,

> > @@ -269,6 +281,8 @@ static int adjust_snap_realm_parent(struct ceph_mds_client *mdsc,

> >  {

> >         struct ceph_snap_realm *parent;

> > 

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> 

> And this since ceph_lookup_snap_realm() is called right away.

> 

> > +

> >         if (realm->parent_ino == parentino)

> >                 return 0;

> > 

> > @@ -696,6 +710,8 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,

> >         int err = -ENOMEM;

> >         LIST_HEAD(dirty_realms);

> > 

> > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> 

> Ditto.

> 

> Thanks,

> 

>                 Ilya


Some of these calls may be redundant, but I'd still like to keep them.

The locking in this code is a mess, and this is the most reliable way
I've found to approach cleaning it up. These all compile out unless
you're running with lockdep enabled anyway.

-- 
Jeff Layton <jlayton@kernel.org>
Ilya Dryomov June 21, 2021, 9:27 p.m. UTC | #3 
On Mon, Jun 21, 2021 at 10:51 PM Jeff Layton <jlayton@kernel.org> wrote:
>

> On Mon, 2021-06-21 at 22:05 +0200, Ilya Dryomov wrote:

> > On Thu, Jun 3, 2021 at 6:52 PM Jeff Layton <jlayton@kernel.org> wrote:

> > >

> > > Turn some comments into lockdep asserts.

> > >

> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>

> > > ---

> > >  fs/ceph/snap.c | 16 ++++++++++++++++

> > >  1 file changed, 16 insertions(+)

> > >

> > > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c

> > > index 2a63fb37778b..bc6c33d485e6 100644

> > > --- a/fs/ceph/snap.c

> > > +++ b/fs/ceph/snap.c

> > > @@ -65,6 +65,8 @@

> > >  void ceph_get_snap_realm(struct ceph_mds_client *mdsc,

> > >                          struct ceph_snap_realm *realm)

> > >  {

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > > +

> > >         dout("get_realm %p %d -> %d\n", realm,

> > >              atomic_read(&realm->nref), atomic_read(&realm->nref)+1);

> > >         /*

> > > @@ -113,6 +115,8 @@ static struct ceph_snap_realm *ceph_create_snap_realm(

> > >  {

> > >         struct ceph_snap_realm *realm;

> > >

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > > +

> > >         realm = kzalloc(sizeof(*realm), GFP_NOFS);

> > >         if (!realm)

> > >                 return ERR_PTR(-ENOMEM);

> > > @@ -143,6 +147,8 @@ static struct ceph_snap_realm *__lookup_snap_realm(struct ceph_mds_client *mdsc,

> > >         struct rb_node *n = mdsc->snap_realms.rb_node;

> > >         struct ceph_snap_realm *r;

> > >

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > > +

> > >         while (n) {

> > >                 r = rb_entry(n, struct ceph_snap_realm, node);

> > >                 if (ino < r->ino)

> > > @@ -176,6 +182,8 @@ static void __put_snap_realm(struct ceph_mds_client *mdsc,

> > >  static void __destroy_snap_realm(struct ceph_mds_client *mdsc,

> > >                                  struct ceph_snap_realm *realm)

> > >  {

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> > > +

> > >         dout("__destroy_snap_realm %p %llx\n", realm, realm->ino);

> > >

> > >         rb_erase(&realm->node, &mdsc->snap_realms);

> > > @@ -198,6 +206,8 @@ static void __destroy_snap_realm(struct ceph_mds_client *mdsc,

> > >  static void __put_snap_realm(struct ceph_mds_client *mdsc,

> > >                              struct ceph_snap_realm *realm)

> > >  {

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> >

> > This one appears to be redundant since the only caller is

> > __destroy_snap_realm().

> >

> > > +

> > >         dout("__put_snap_realm %llx %p %d -> %d\n", realm->ino, realm,

> > >              atomic_read(&realm->nref), atomic_read(&realm->nref)-1);

> > >         if (atomic_dec_and_test(&realm->nref))

> > > @@ -236,6 +246,8 @@ static void __cleanup_empty_realms(struct ceph_mds_client *mdsc)

> > >  {

> > >         struct ceph_snap_realm *realm;

> > >

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> >

> > This too since it boils down to calling __destroy_snap_realm().

> >

> > > +

> > >         spin_lock(&mdsc->snap_empty_lock);

> > >         while (!list_empty(&mdsc->snap_empty)) {

> > >                 realm = list_first_entry(&mdsc->snap_empty,

> > > @@ -269,6 +281,8 @@ static int adjust_snap_realm_parent(struct ceph_mds_client *mdsc,

> > >  {

> > >         struct ceph_snap_realm *parent;

> > >

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> >

> > And this since ceph_lookup_snap_realm() is called right away.

> >

> > > +

> > >         if (realm->parent_ino == parentino)

> > >                 return 0;

> > >

> > > @@ -696,6 +710,8 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,

> > >         int err = -ENOMEM;

> > >         LIST_HEAD(dirty_realms);

> > >

> > > +       lockdep_assert_held_write(&mdsc->snap_rwsem);

> >

> > Ditto.

> >

> > Thanks,

> >

> >                 Ilya

>

> Some of these calls may be redundant, but I'd still like to keep them.

>

> The locking in this code is a mess, and this is the most reliable way

> I've found to approach cleaning it up. These all compile out unless

> you're running with lockdep enabled anyway.


Reviewed-by: Ilya Dryomov <idryomov@gmail.com>


Thanks,

                Ilya
diff mbox series

Patch

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index 2a63fb37778b..bc6c33d485e6 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -65,6 +65,8 @@ 
 void ceph_get_snap_realm(struct ceph_mds_client *mdsc,
 			 struct ceph_snap_realm *realm)
 {
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	dout("get_realm %p %d -> %d\n", realm,
 	     atomic_read(&realm->nref), atomic_read(&realm->nref)+1);
 	/*
@@ -113,6 +115,8 @@  static struct ceph_snap_realm *ceph_create_snap_realm(
 {
 	struct ceph_snap_realm *realm;
 
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	realm = kzalloc(sizeof(*realm), GFP_NOFS);
 	if (!realm)
 		return ERR_PTR(-ENOMEM);
@@ -143,6 +147,8 @@  static struct ceph_snap_realm *__lookup_snap_realm(struct ceph_mds_client *mdsc,
 	struct rb_node *n = mdsc->snap_realms.rb_node;
 	struct ceph_snap_realm *r;
 
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	while (n) {
 		r = rb_entry(n, struct ceph_snap_realm, node);
 		if (ino < r->ino)
@@ -176,6 +182,8 @@  static void __put_snap_realm(struct ceph_mds_client *mdsc,
 static void __destroy_snap_realm(struct ceph_mds_client *mdsc,
 				 struct ceph_snap_realm *realm)
 {
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	dout("__destroy_snap_realm %p %llx\n", realm, realm->ino);
 
 	rb_erase(&realm->node, &mdsc->snap_realms);
@@ -198,6 +206,8 @@  static void __destroy_snap_realm(struct ceph_mds_client *mdsc,
 static void __put_snap_realm(struct ceph_mds_client *mdsc,
 			     struct ceph_snap_realm *realm)
 {
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	dout("__put_snap_realm %llx %p %d -> %d\n", realm->ino, realm,
 	     atomic_read(&realm->nref), atomic_read(&realm->nref)-1);
 	if (atomic_dec_and_test(&realm->nref))
@@ -236,6 +246,8 @@  static void __cleanup_empty_realms(struct ceph_mds_client *mdsc)
 {
 	struct ceph_snap_realm *realm;
 
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	spin_lock(&mdsc->snap_empty_lock);
 	while (!list_empty(&mdsc->snap_empty)) {
 		realm = list_first_entry(&mdsc->snap_empty,
@@ -269,6 +281,8 @@  static int adjust_snap_realm_parent(struct ceph_mds_client *mdsc,
 {
 	struct ceph_snap_realm *parent;
 
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	if (realm->parent_ino == parentino)
 		return 0;
 
@@ -696,6 +710,8 @@  int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
 	int err = -ENOMEM;
 	LIST_HEAD(dirty_realms);
 
+	lockdep_assert_held_write(&mdsc->snap_rwsem);
+
 	dout("update_snap_trace deletion=%d\n", deletion);
 more:
 	ceph_decode_need(&p, e, sizeof(*ri), bad);