On Mon, 2021-06-21 at 22:05 +0200, Ilya Dryomov wrote:
> On Thu, Jun 3, 2021 at 6:52 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > Turn some comments into lockdep asserts.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
