Re: [RFC PATCH 6/6] ceph: eliminate ceph_async_iput()

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2021-06-18 at 13:56 +0100, Luis Henriques wrote:
> On Thu, Jun 17, 2021 at 12:24:35PM -0400, Jeff Layton wrote:
> > On Wed, 2021-06-16 at 16:26 +0100, Luis Henriques wrote:
> > > On Tue, Jun 15, 2021 at 10:57:30AM -0400, Jeff Layton wrote:
> > > > Now that we don't need to hold session->s_mutex or the snap_rwsem when
> > > > calling ceph_check_caps, we can eliminate ceph_async_iput and just use
> > > > normal iput calls.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/caps.c       |  6 +++---
> > > >  fs/ceph/inode.c      | 25 +++----------------------
> > > >  fs/ceph/mds_client.c | 22 +++++++++++-----------
> > > >  fs/ceph/quota.c      |  6 +++---
> > > >  fs/ceph/snap.c       | 10 +++++-----
> > > >  fs/ceph/super.h      |  2 --
> > > >  6 files changed, 25 insertions(+), 46 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > > index 5864d5088e27..fd9243e9a1b2 100644
> > > > --- a/fs/ceph/caps.c
> > > > +++ b/fs/ceph/caps.c
> > > > @@ -3147,7 +3147,7 @@ void ceph_put_wrbuffer_cap_refs(struct ceph_inode_info *ci, int nr,
> > > >  		wake_up_all(&ci->i_cap_wq);
> > > >  	while (put-- > 0) {
> > > >  		/* avoid calling iput_final() in osd dispatch threads */
> > > > -		ceph_async_iput(inode);
> > > > +		iput(inode);
> > > >  	}
> > > >  }
> > > >  
> > > > @@ -4136,7 +4136,7 @@ void ceph_handle_caps(struct ceph_mds_session *session,
> > > >  done_unlocked:
> > > >  	ceph_put_string(extra_info.pool_ns);
> > > >  	/* avoid calling iput_final() in mds dispatch threads */
> > > > -	ceph_async_iput(inode);
> > > > +	iput(inode);
> > > 
> > > To be honest, I'm not really convinced we can blindly substitute
> > > ceph_async_iput() by iput().  This case specifically can problematic
> > > because we may have called ceph_queue_vmtruncate() above (or
> > > handle_cap_grant()).  If we did, we have ci->i_work_mask set and the wq
> > > would have invoked __ceph_do_pending_vmtruncate().  Using the iput() here
> > > won't have that result.  Am I missing something?
> > > 
> > 
> > The point of this set is to make iput safe to run even when the s_mutex
> > and/or snap_rwsem is held. When we queue up the ci->i_work, we do take a
> > reference to the inode and still run iput there. This set just stops
> > queueing iputs themselves to the workqueue.
> > 
> > I really don't see a problem with this call site in ceph_handle_caps in
> > particular, as it's calling iput after dropping the s_mutex. Probably
> > that should not have been converted to use ceph_async_iput in the first
> > place.
> 
> Obviously, you're right.  I don't what I was thinking of when I was
> reading this code and saw: ci->i_work_mask bits are set in one place (in
> ceph_queue_vmtruncate(), for ex.) and then the work is queued only in
> ceph_async_iput().  Which is obviously wrong!
> 
> Anyway, sorry for the noise.
> 
> (Oh, and BTW: this patch should also remove comments such as "avoid
> calling iput_final() in mds dispatch threads", and similar that exist in
> several places before ceph_async_iput() is (or rather *was*) called.)
> 

Thanks. I saw that I missed a few of those sorts of comments. I'll plan
to remove those before I merge this.

Cheers,
-- 
Jeff Layton <jlayton@kernel.org>