| Message ID | 20210603165231.110559-2-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| 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.
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(+)