| Message ID | 20220314200717.52033-1-dossche.niels@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: get snap_rwsem read lock in handle_cap_export for ceph_add_cap | expand |
On Mon, 2022-03-14 at 21:07 +0100, Niels Dossche wrote: > ceph_add_cap says in its function documentation that the caller should > hold the read lock on the session snap_rwsem. Furthermore, not only > ceph_add_cap needs that lock, when it calls to ceph_lookup_snap_realm it > eventually calls ceph_get_snap_realm which states via lockdep that > snap_rwsem needs to be held. handle_cap_export calls ceph_add_cap > without that mdsc->snap_rwsem held. Thus, since ceph_get_snap_realm > and ceph_add_cap both need the lock, the common place to acquire that > lock is inside handle_cap_export. > > Signed-off-by: Niels Dossche <dossche.niels@gmail.com> > --- > fs/ceph/caps.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index b472cd066d1c..0dd60db285b1 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -3903,8 +3903,10 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, > /* add placeholder for the export tagert */ > int flag = (cap == ci->i_auth_cap) ? CEPH_CAP_FLAG_AUTH : 0; > tcap = new_cap; > + down_read(&mdsc->snap_rwsem); > ceph_add_cap(inode, tsession, t_cap_id, issued, 0, > t_seq - 1, t_mseq, (u64)-1, flag, &new_cap); > + up_read(&mdsc->snap_rwsem); > > if (!list_empty(&ci->i_cap_flush_list) && > ci->i_auth_cap == tcap) { Looks good. The other ceph_add_cap callsites already hold this. Merged into ceph testing branch. Thanks!
On Tue, 2022-03-15 at 08:26 -0400, Jeff Layton wrote: > On Mon, 2022-03-14 at 21:07 +0100, Niels Dossche wrote: > > ceph_add_cap says in its function documentation that the caller should > > hold the read lock on the session snap_rwsem. Furthermore, not only > > ceph_add_cap needs that lock, when it calls to ceph_lookup_snap_realm it > > eventually calls ceph_get_snap_realm which states via lockdep that > > snap_rwsem needs to be held. handle_cap_export calls ceph_add_cap > > without that mdsc->snap_rwsem held. Thus, since ceph_get_snap_realm > > and ceph_add_cap both need the lock, the common place to acquire that > > lock is inside handle_cap_export. > > > > Signed-off-by: Niels Dossche <dossche.niels@gmail.com> > > --- > > fs/ceph/caps.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > index b472cd066d1c..0dd60db285b1 100644 > > --- a/fs/ceph/caps.c > > +++ b/fs/ceph/caps.c > > @@ -3903,8 +3903,10 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, > > /* add placeholder for the export tagert */ > > int flag = (cap == ci->i_auth_cap) ? CEPH_CAP_FLAG_AUTH : 0; > > tcap = new_cap; > > + down_read(&mdsc->snap_rwsem); > > ceph_add_cap(inode, tsession, t_cap_id, issued, 0, > > t_seq - 1, t_mseq, (u64)-1, flag, &new_cap); > > + up_read(&mdsc->snap_rwsem); > > > > if (!list_empty(&ci->i_cap_flush_list) && > > ci->i_auth_cap == tcap) { > > Looks good. The other ceph_add_cap callsites already hold this. > > Merged into ceph testing branch. > Oops, spoke too soon. This patch calls down_read (a potentially sleeping function) while holding the i_ceph_lock spinlock. I think you'll need to take the rwsem earlier in the function, before taking the spinlock. Dropped from testing branch for now...
On 3/15/22 16:10, Jeff Layton wrote: > On Tue, 2022-03-15 at 08:26 -0400, Jeff Layton wrote: >> On Mon, 2022-03-14 at 21:07 +0100, Niels Dossche wrote: >>> ceph_add_cap says in its function documentation that the caller should >>> hold the read lock on the session snap_rwsem. Furthermore, not only >>> ceph_add_cap needs that lock, when it calls to ceph_lookup_snap_realm it >>> eventually calls ceph_get_snap_realm which states via lockdep that >>> snap_rwsem needs to be held. handle_cap_export calls ceph_add_cap >>> without that mdsc->snap_rwsem held. Thus, since ceph_get_snap_realm >>> and ceph_add_cap both need the lock, the common place to acquire that >>> lock is inside handle_cap_export. >>> >>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com> >>> --- >>> fs/ceph/caps.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c >>> index b472cd066d1c..0dd60db285b1 100644 >>> --- a/fs/ceph/caps.c >>> +++ b/fs/ceph/caps.c >>> @@ -3903,8 +3903,10 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, >>> /* add placeholder for the export tagert */ >>> int flag = (cap == ci->i_auth_cap) ? CEPH_CAP_FLAG_AUTH : 0; >>> tcap = new_cap; >>> + down_read(&mdsc->snap_rwsem); >>> ceph_add_cap(inode, tsession, t_cap_id, issued, 0, >>> t_seq - 1, t_mseq, (u64)-1, flag, &new_cap); >>> + up_read(&mdsc->snap_rwsem); >>> >>> if (!list_empty(&ci->i_cap_flush_list) && >>> ci->i_auth_cap == tcap) { >> >> Looks good. The other ceph_add_cap callsites already hold this. >> >> Merged into ceph testing branch. >> > > > Oops, spoke too soon. This patch calls down_read (a potentially sleeping > function) while holding the i_ceph_lock spinlock. I think you'll need to > take the rwsem earlier in the function, before taking the spinlock. > > Dropped from testing branch for now... Ah my bad. I notice that handle_cap_export is actually called with the i_ceph_lock spinlock. I can send a v2 which acquires the down_read lock just before the i_ceph_lock spinlock is taken (i.e. just under the retry label). Does that work for you? If so, I'll send a v2. Thanks!
On Tue, 2022-03-15 at 16:16 +0100, Niels Dossche wrote: > On 3/15/22 16:10, Jeff Layton wrote: > > On Tue, 2022-03-15 at 08:26 -0400, Jeff Layton wrote: > > > On Mon, 2022-03-14 at 21:07 +0100, Niels Dossche wrote: > > > > ceph_add_cap says in its function documentation that the caller should > > > > hold the read lock on the session snap_rwsem. Furthermore, not only > > > > ceph_add_cap needs that lock, when it calls to ceph_lookup_snap_realm it > > > > eventually calls ceph_get_snap_realm which states via lockdep that > > > > snap_rwsem needs to be held. handle_cap_export calls ceph_add_cap > > > > without that mdsc->snap_rwsem held. Thus, since ceph_get_snap_realm > > > > and ceph_add_cap both need the lock, the common place to acquire that > > > > lock is inside handle_cap_export. > > > > > > > > Signed-off-by: Niels Dossche <dossche.niels@gmail.com> > > > > --- > > > > fs/ceph/caps.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > > > index b472cd066d1c..0dd60db285b1 100644 > > > > --- a/fs/ceph/caps.c > > > > +++ b/fs/ceph/caps.c > > > > @@ -3903,8 +3903,10 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, > > > > /* add placeholder for the export tagert */ > > > > int flag = (cap == ci->i_auth_cap) ? CEPH_CAP_FLAG_AUTH : 0; > > > > tcap = new_cap; > > > > + down_read(&mdsc->snap_rwsem); > > > > ceph_add_cap(inode, tsession, t_cap_id, issued, 0, > > > > t_seq - 1, t_mseq, (u64)-1, flag, &new_cap); > > > > + up_read(&mdsc->snap_rwsem); > > > > > > > > if (!list_empty(&ci->i_cap_flush_list) && > > > > ci->i_auth_cap == tcap) { > > > > > > Looks good. The other ceph_add_cap callsites already hold this. > > > > > > Merged into ceph testing branch. > > > > > > > > > Oops, spoke too soon. This patch calls down_read (a potentially sleeping > > function) while holding the i_ceph_lock spinlock. I think you'll need to > > take the rwsem earlier in the function, before taking the spinlock. > > > > Dropped from testing branch for now... > > Ah my bad. I notice that handle_cap_export is actually called with the i_ceph_lock spinlock. > I can send a v2 which acquires the down_read lock just before the i_ceph_lock spinlock is taken (i.e. just under the retry label). > Does that work for you? If so, I'll send a v2. > Thanks! That should be fine.
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index b472cd066d1c..0dd60db285b1 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -3903,8 +3903,10 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex, /* add placeholder for the export tagert */ int flag = (cap == ci->i_auth_cap) ? CEPH_CAP_FLAG_AUTH : 0; tcap = new_cap; + down_read(&mdsc->snap_rwsem); ceph_add_cap(inode, tsession, t_cap_id, issued, 0, t_seq - 1, t_mseq, (u64)-1, flag, &new_cap); + up_read(&mdsc->snap_rwsem); if (!list_empty(&ci->i_cap_flush_list) && ci->i_auth_cap == tcap) {
ceph_add_cap says in its function documentation that the caller should hold the read lock on the session snap_rwsem. Furthermore, not only ceph_add_cap needs that lock, when it calls to ceph_lookup_snap_realm it eventually calls ceph_get_snap_realm which states via lockdep that snap_rwsem needs to be held. handle_cap_export calls ceph_add_cap without that mdsc->snap_rwsem held. Thus, since ceph_get_snap_realm and ceph_add_cap both need the lock, the common place to acquire that lock is inside handle_cap_export. Signed-off-by: Niels Dossche <dossche.niels@gmail.com> --- fs/ceph/caps.c | 2 ++ 1 file changed, 2 insertions(+)