All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] ceph: locking fixes for snaprealm handling
@ 2021-06-03 16:52 Jeff Layton
  2021-06-03 16:52 ` [PATCH 1/3] ceph: add some lockdep assertions around " Jeff Layton
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Jeff Layton @ 2021-06-03 16:52 UTC (permalink / raw)
  To: ceph-devel, idryomov

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] ceph: add some lockdep assertions around snaprealm handling
  2021-06-03 16:52 [PATCH 0/3] ceph: locking fixes for snaprealm handling Jeff Layton
@ 2021-06-03 16:52 ` Jeff Layton
  2021-06-21 20:05   ` 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-03 16:52 ` [PATCH 3/3] ceph: must hold snap_rwsem when filling inode for async create Jeff Layton
  2 siblings, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2021-06-03 16:52 UTC (permalink / raw)
  To: ceph-devel, idryomov

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/3] ceph: clean up locking annotation for ceph_get_snap_realm and __lookup_snap_realm
  2021-06-03 16:52 [PATCH 0/3] ceph: locking fixes for snaprealm handling Jeff Layton
  2021-06-03 16:52 ` [PATCH 1/3] ceph: add some lockdep assertions around " Jeff Layton
@ 2021-06-03 16:52 ` 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
  2 siblings, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2021-06-03 16:52 UTC (permalink / raw)
  To: ceph-devel, idryomov

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/3] ceph: must hold snap_rwsem when filling inode for async create
  2021-06-03 16:52 [PATCH 0/3] ceph: locking fixes for snaprealm handling Jeff Layton
  2021-06-03 16:52 ` [PATCH 1/3] ceph: add some lockdep assertions around " Jeff Layton
  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-03 16:52 ` Jeff Layton
  2021-06-21 19:45   ` Ilya Dryomov
  2 siblings, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2021-06-03 16:52 UTC (permalink / raw)
  To: ceph-devel, idryomov; +Cc: stable

...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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ceph: must hold snap_rwsem when filling inode for async create
  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
  0 siblings, 0 replies; 9+ messages in thread
From: Ilya Dryomov @ 2021-06-21 19:45 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development, stable

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] ceph: add some lockdep assertions around snaprealm handling
  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
  0 siblings, 1 reply; 9+ messages in thread
From: Ilya Dryomov @ 2021-06-21 20:05 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] ceph: clean up locking annotation for ceph_get_snap_realm and __lookup_snap_realm
  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
  0 siblings, 0 replies; 9+ messages in thread
From: Ilya Dryomov @ 2021-06-21 20:48 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] ceph: add some lockdep assertions around snaprealm handling
  2021-06-21 20:05   ` Ilya Dryomov
@ 2021-06-21 20:51     ` Jeff Layton
  2021-06-21 21:27       ` Ilya Dryomov
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2021-06-21 20:51 UTC (permalink / raw)
  To: Ilya Dryomov; +Cc: Ceph Development

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>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] ceph: add some lockdep assertions around snaprealm handling
  2021-06-21 20:51     ` Jeff Layton
@ 2021-06-21 21:27       ` Ilya Dryomov
  0 siblings, 0 replies; 9+ messages in thread
From: Ilya Dryomov @ 2021-06-21 21:27 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-21 21:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-03 16:52 [PATCH 0/3] ceph: locking fixes for snaprealm handling 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
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

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.