Re: [PATCH 2/3] ceph: clean up inode work queueing

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2020-12-14 at 15:34 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > Add a generic function for taking an inode reference, setting the I_WORK
> > bit and queueing i_work. Turn the ceph_queue_* functions into static
> > inline wrappers that pass in the right bit.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/inode.c | 55 ++++++-------------------------------------------
> >  fs/ceph/super.h | 21 ++++++++++++++++---
> >  2 files changed, 24 insertions(+), 52 deletions(-)
> > 
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index c870be90d850..9cd8b37e586a 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -1816,60 +1816,17 @@ void ceph_async_iput(struct inode *inode)
> >  	}
> >  }
> >  
> > 
> > -/*
> > - * Write back inode data in a worker thread.  (This can't be done
> > - * in the message handler context.)
> > - */
> > -void ceph_queue_writeback(struct inode *inode)
> > -{
> > -	struct ceph_inode_info *ci = ceph_inode(inode);
> > -	set_bit(CEPH_I_WORK_WRITEBACK, &ci->i_work_mask);
> > -
> > -	ihold(inode);
> > -	if (queue_work(ceph_inode_to_client(inode)->inode_wq,
> > -		       &ci->i_work)) {
> > -		dout("ceph_queue_writeback %p\n", inode);
> > -	} else {
> > -		dout("ceph_queue_writeback %p already queued, mask=%lx\n",
> > -		     inode, ci->i_work_mask);
> > -		iput(inode);
> > -	}
> > -}
> > -
> > -/*
> > - * queue an async invalidation
> > - */
> > -void ceph_queue_invalidate(struct inode *inode)
> > -{
> > -	struct ceph_inode_info *ci = ceph_inode(inode);
> > -	set_bit(CEPH_I_WORK_INVALIDATE_PAGES, &ci->i_work_mask);
> > -
> > -	ihold(inode);
> > -	if (queue_work(ceph_inode_to_client(inode)->inode_wq,
> > -		       &ceph_inode(inode)->i_work)) {
> > -		dout("ceph_queue_invalidate %p\n", inode);
> > -	} else {
> > -		dout("ceph_queue_invalidate %p already queued, mask=%lx\n",
> > -		     inode, ci->i_work_mask);
> > -		iput(inode);
> > -	}
> > -}
> > -
> > -/*
> > - * Queue an async vmtruncate.  If we fail to queue work, we will handle
> > - * the truncation the next time we call __ceph_do_pending_vmtruncate.
> > - */
> > -void ceph_queue_vmtruncate(struct inode *inode)
> > +void ceph_queue_inode_work(struct inode *inode, int work_bit)
> >  {
> > +	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> >  	struct ceph_inode_info *ci = ceph_inode(inode);
> > -	set_bit(CEPH_I_WORK_VMTRUNCATE, &ci->i_work_mask);
> > +	set_bit(work_bit, &ci->i_work_mask);
> >  
> > 
> >  	ihold(inode);
> > -	if (queue_work(ceph_inode_to_client(inode)->inode_wq,
> > -		       &ci->i_work)) {
> > -		dout("ceph_queue_vmtruncate %p\n", inode);
> > +	if (queue_work(fsc->inode_wq, &ceph_inode(inode)->i_work)) {
> 
> Nit: since we have ci, it should probably be used here^^ instead of
> ceph_inode() (this is likely a ceph_queue_invalidate function leftover,
> which already had this inconsistency).
> 
> Other than that, these patches look good although I (obviously) haven't
> seen the lockdep warning you mention.  Hopefully I'll never see it ever,
> with these patches applied ;-)
> 
> Cheers,

Thanks Luis,

I went ahead and made the above change and pushed the set into the
testing branch (note that this patch supersedes the queue_inode_work
patch I sent a couple of weeks ago).
 
-- 
Jeff Layton <jlayton@kernel.org>