[PATCH 0/3] ceph: locking fixes for snaprealm handling

[PATCH 0/3] ceph: locking fixes for snaprealm handling

[Jeff Layton]

The snaprealm handling code has a number of really old and misleading
comments that claim that we need certain locks when holding certain
functions. Some of them are wrong, and at least one caller is not
holding the snap_rwsem when filling an inode.

The first patch in this series adds some lockdep annotations that mirror
the comments. The second one relaxes the lockdep annotations for a
couple of the functions to better conform with the real requirements of
the code. The last patch then fixes a bug in async create code and
ensures that we're holding the correct locks when filling a new inode.

Jeff Layton (3):
  ceph: add some lockdep assertions around snaprealm handling
  ceph: clean up locking annotation for ceph_get_snap_realm and
    __lookup_snap_realm
  ceph: must hold snap_rwsem when filling inode for async create

 fs/ceph/file.c  |  3 +++
 fs/ceph/inode.c |  2 ++
 fs/ceph/snap.c  | 20 ++++++++++++++++++--
 3 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.31.1

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

[Jeff Layton]

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);
+
 	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);
-- 
2.31.1

[PATCH 2/3] ceph: clean up locking annotation for ceph_get_snap_realm and __lookup_snap_realm

[Jeff Layton]

They both say that the snap_rwsem must be held for write, but I don't
see any real reason for it, and it's not currently always called that
way.

The lookup is just walking the rbtree, so holding it for read should be
fine there. The "get" is bumping the refcount and (possibly) removing
it from the empty list. I see no need to hold the snap_rwsem for write
for that.

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

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index bc6c33d485e6..f8cac2abab3f 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -60,12 +60,12 @@
 /*
  * increase ref count for the realm
  *
- * caller must hold snap_rwsem for write.
+ * caller must hold snap_rwsem.
  */
 void ceph_get_snap_realm(struct ceph_mds_client *mdsc,
 			 struct ceph_snap_realm *realm)
 {
-	lockdep_assert_held_write(&mdsc->snap_rwsem);
+	lockdep_assert_held(&mdsc->snap_rwsem);
 
 	dout("get_realm %p %d -> %d\n", realm,
 	     atomic_read(&realm->nref), atomic_read(&realm->nref)+1);
@@ -139,7 +139,7 @@ static struct ceph_snap_realm *ceph_create_snap_realm(
 /*
  * lookup the realm rooted at @ino.
  *
- * caller must hold snap_rwsem for write.
+ * caller must hold snap_rwsem.
  */
 static struct ceph_snap_realm *__lookup_snap_realm(struct ceph_mds_client *mdsc,
 						   u64 ino)
@@ -147,7 +147,7 @@ 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);
+	lockdep_assert_held(&mdsc->snap_rwsem);
 
 	while (n) {
 		r = rb_entry(n, struct ceph_snap_realm, node);
-- 
2.31.1

[PATCH 3/3] ceph: must hold snap_rwsem when filling inode for async create

[Jeff Layton]

...and add a lockdep assertion for it to ceph_fill_inode().

Cc: stable@vger.kernel.org # v5.7+
Fixes: 9a8d03ca2e2c3 ("ceph: attempt to do async create when possible")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/file.c  | 3 +++
 fs/ceph/inode.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index a01ad342a91d..d3874c2df4b1 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -578,6 +578,7 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 	struct ceph_inode_info *ci = ceph_inode(dir);
 	struct inode *inode;
 	struct timespec64 now;
+	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(dir->i_sb);
 	struct ceph_vino vino = { .ino = req->r_deleg_ino,
 				  .snap = CEPH_NOSNAP };
 
@@ -615,8 +616,10 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 
 	ceph_file_layout_to_legacy(lo, &in.layout);
 
+	down_read(&mdsc->snap_rwsem);
 	ret = ceph_fill_inode(inode, NULL, &iinfo, NULL, req->r_session,
 			      req->r_fmode, NULL);
+	up_read(&mdsc->snap_rwsem);
 	if (ret) {
 		dout("%s failed to fill inode: %d\n", __func__, ret);
 		ceph_dir_clear_complete(dir);
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index e1c63adb196d..df0c8a724609 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -777,6 +777,8 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page,
 	umode_t mode = le32_to_cpu(info->mode);
 	dev_t rdev = le32_to_cpu(info->rdev);
 
+	lockdep_assert_held(&mdsc->snap_rwsem);
+
 	dout("%s %p ino %llx.%llx v %llu had %llu\n", __func__,
 	     inode, ceph_vinop(inode), le64_to_cpu(info->version),
 	     ci->i_version);
-- 
2.31.1

Re: [PATCH 3/3] ceph: must hold snap_rwsem when filling inode for async create

[Ilya Dryomov]

On Thu, Jun 3, 2021 at 6:52 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> ...and add a lockdep assertion for it to ceph_fill_inode().
>
> Cc: stable@vger.kernel.org # v5.7+
> Fixes: 9a8d03ca2e2c3 ("ceph: attempt to do async create when possible")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/file.c  | 3 +++
>  fs/ceph/inode.c | 2 ++
>  2 files changed, 5 insertions(+)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index a01ad342a91d..d3874c2df4b1 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -578,6 +578,7 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
>         struct ceph_inode_info *ci = ceph_inode(dir);
>         struct inode *inode;
>         struct timespec64 now;
> +       struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(dir->i_sb);
>         struct ceph_vino vino = { .ino = req->r_deleg_ino,
>                                   .snap = CEPH_NOSNAP };
>
> @@ -615,8 +616,10 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
>
>         ceph_file_layout_to_legacy(lo, &in.layout);
>
> +       down_read(&mdsc->snap_rwsem);
>         ret = ceph_fill_inode(inode, NULL, &iinfo, NULL, req->r_session,
>                               req->r_fmode, NULL);
> +       up_read(&mdsc->snap_rwsem);
>         if (ret) {
>                 dout("%s failed to fill inode: %d\n", __func__, ret);
>                 ceph_dir_clear_complete(dir);
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index e1c63adb196d..df0c8a724609 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -777,6 +777,8 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page,
>         umode_t mode = le32_to_cpu(info->mode);
>         dev_t rdev = le32_to_cpu(info->rdev);
>
> +       lockdep_assert_held(&mdsc->snap_rwsem);
> +
>         dout("%s %p ino %llx.%llx v %llu had %llu\n", __func__,
>              inode, ceph_vinop(inode), le64_to_cpu(info->version),
>              ci->i_version);
> --
> 2.31.1
>

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

Thanks,

                Ilya

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

[Ilya Dryomov]

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

Re: [PATCH 2/3] ceph: clean up locking annotation for ceph_get_snap_realm and __lookup_snap_realm

[Ilya Dryomov]

On Thu, Jun 3, 2021 at 6:52 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> They both say that the snap_rwsem must be held for write, but I don't
> see any real reason for it, and it's not currently always called that
> way.
>
> The lookup is just walking the rbtree, so holding it for read should be
> fine there. The "get" is bumping the refcount and (possibly) removing
> it from the empty list. I see no need to hold the snap_rwsem for write
> for that.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/snap.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index bc6c33d485e6..f8cac2abab3f 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -60,12 +60,12 @@
>  /*
>   * increase ref count for the realm
>   *
> - * caller must hold snap_rwsem for write.
> + * caller must hold snap_rwsem.
>   */
>  void ceph_get_snap_realm(struct ceph_mds_client *mdsc,
>                          struct ceph_snap_realm *realm)
>  {
> -       lockdep_assert_held_write(&mdsc->snap_rwsem);
> +       lockdep_assert_held(&mdsc->snap_rwsem);
>
>         dout("get_realm %p %d -> %d\n", realm,
>              atomic_read(&realm->nref), atomic_read(&realm->nref)+1);
> @@ -139,7 +139,7 @@ static struct ceph_snap_realm *ceph_create_snap_realm(
>  /*
>   * lookup the realm rooted at @ino.
>   *
> - * caller must hold snap_rwsem for write.
> + * caller must hold snap_rwsem.
>   */
>  static struct ceph_snap_realm *__lookup_snap_realm(struct ceph_mds_client *mdsc,
>                                                    u64 ino)
> @@ -147,7 +147,7 @@ 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);
> +       lockdep_assert_held(&mdsc->snap_rwsem);
>
>         while (n) {
>                 r = rb_entry(n, struct ceph_snap_realm, node);
> --
> 2.31.1
>

Ah, since you are relaxing the requirement some of those lockdep
asserts from the previous patch aren't actually redundant.  This seems
fine to me: lookup definitely doesn't need the write lock and allowing
concurrent gets should be OK.  The write lock made some sense back when
get was an atomic_read followed by atomic_inc but not now.

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

Thanks,

                Ilya

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

[Jeff Layton]

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>

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

[Ilya Dryomov]

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