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@xxxxxxxxx>
> > > > ---
> > > > 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.
--
Jeff Layton <jlayton@xxxxxxxxxx>
