| Message ID | 20210603165231.110559-2-jlayton@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: locking fixes for snaprealm handling | expand |
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
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>
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 --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);
Turn some comments into lockdep asserts. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/snap.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)