* [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 related [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 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
* [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 related [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
* [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 related [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
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.