All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: Ilya Dryomov <idryomov@gmail.com>
Cc: Ceph Development <ceph-devel@vger.kernel.org>
Subject: Re: [PATCH 1/3] ceph: add some lockdep assertions around snaprealm handling
Date: Mon, 21 Jun 2021 16:51:22 -0400	[thread overview]
Message-ID: <4d985178b5b5b95d324054928d291bd65c3f56fe.camel@kernel.org> (raw)
In-Reply-To: <CAOi1vP9eGNxfS5suHGeBpK5H9jdWphoioutwT25=jKSw8u5UmA@mail.gmail.com>

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>


  reply	other threads:[~2021-06-21 20:51 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-03 16:52 [PATCH 0/3] ceph: locking fixes for " Jeff Layton
2021-06-03 16:52 ` [PATCH 1/3] ceph: add some lockdep assertions around " Jeff Layton
2021-06-21 20:05   ` Ilya Dryomov
2021-06-21 20:51     ` Jeff Layton [this message]
2021-06-21 21:27       ` Ilya Dryomov
2021-06-03 16:52 ` [PATCH 2/3] ceph: clean up locking annotation for ceph_get_snap_realm and __lookup_snap_realm Jeff Layton
2021-06-21 20:48   ` Ilya Dryomov
2021-06-03 16:52 ` [PATCH 3/3] ceph: must hold snap_rwsem when filling inode for async create Jeff Layton
2021-06-21 19:45   ` Ilya Dryomov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4d985178b5b5b95d324054928d291bd65c3f56fe.camel@kernel.org \
    --to=jlayton@kernel.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=idryomov@gmail.com \
    --subject='Re: [PATCH 1/3] ceph: add some lockdep assertions around snaprealm handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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.