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@xxxxxxxxxx>
> > > ---
> > > 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.)
Cheers,
--
Luís
