[PATCH 0/3] ceph: don't call ceph_check_caps in page_mkwrite

[PATCH 0/3] ceph: don't call ceph_check_caps in page_mkwrite

[Jeff Layton]

I've been working on the fscache rework, and have hit some rather
complex lockdep circular locking warnings. Most of them involve two
filesystems (cephfs and the local cachefiles fs), so it's not clear
to me whether they are false positives.

They do involve the mmap_lock though, which is taken up in the vfs. I
think it would probably be best to not do the ceph_check_caps call with
that lock held if we can avoid it.

The first patch in this series fixes what looks like a probable bug to
me. If we want to avoid checking caps in some cases, then we probably
also want to avoid flushing the snaps too since that involves the same
locks.

The second patch replaces the patch that I sent a few weeks ago to add
a queue_inode_work helper.

The last patch extends some work that Xiubo did earlier to allow skipping
the caps check after putting references. It adds a new "flavor" when
putting caps that instead has the inode work do the check or flush after
the refcounts have been decremented.

Jeff Layton (3):
  ceph: fix flush_snap logic after putting caps
  ceph: clean up inode work queueing
  ceph: allow queueing cap/snap handling after putting cap references

 fs/ceph/addr.c  |  2 +-
 fs/ceph/caps.c  | 36 +++++++++++++++++++++++------
 fs/ceph/inode.c | 61 ++++++++++---------------------------------------
 fs/ceph/super.h | 40 +++++++++++++++++++++++++++-----
 4 files changed, 76 insertions(+), 63 deletions(-)

-- 
2.29.2

[PATCH 1/3] ceph: fix flush_snap logic after putting caps

[Jeff Layton]

A primary reason for skipping ceph_check_caps after putting the
references was to avoid the locking in ceph_check_caps during a
reconnect. __ceph_put_cap_refs can still call ceph_flush_snaps in that
case though, and that takes many of the same inconvenient locks.

Fix the logic in __ceph_put_cap_refs to skip flushing snaps when the
skip_checking_caps flag is set.

Fixes: e64f44a88465 (ceph: skip checking caps when session reconnecting and releasing reqs)
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/caps.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index e46871e2d9e0..336348e733b9 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -3092,10 +3092,12 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
 	dout("put_cap_refs %p had %s%s%s\n", inode, ceph_cap_string(had),
 	     last ? " last" : "", put ? " put" : "");
 
-	if (last && !skip_checking_caps)
-		ceph_check_caps(ci, 0, NULL);
-	else if (flushsnaps)
-		ceph_flush_snaps(ci, NULL);
+	if (!skip_checking_caps) {
+		if (last)
+			ceph_check_caps(ci, 0, NULL);
+		else if (flushsnaps)
+			ceph_flush_snaps(ci, NULL);
+	}
 	if (wake)
 		wake_up_all(&ci->i_cap_wq);
 	while (put-- > 0)
-- 
2.29.2

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

[Jeff Layton]

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)) {
+		dout("queue_inode_work %p, mask=%lx\n", inode, ci->i_work_mask);
 	} else {
-		dout("ceph_queue_vmtruncate %p already queued, mask=%lx\n",
+		dout("queue_inode_work %p already queued, mask=%lx\n",
 		     inode, ci->i_work_mask);
 		iput(inode);
 	}
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index b62d8fee3b86..59153ee201c0 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -962,11 +962,26 @@ extern int ceph_inode_holds_cap(struct inode *inode, int mask);
 
 extern bool ceph_inode_set_size(struct inode *inode, loff_t size);
 extern void __ceph_do_pending_vmtruncate(struct inode *inode);
-extern void ceph_queue_vmtruncate(struct inode *inode);
-extern void ceph_queue_invalidate(struct inode *inode);
-extern void ceph_queue_writeback(struct inode *inode);
+
 extern void ceph_async_iput(struct inode *inode);
 
+void ceph_queue_inode_work(struct inode *inode, int work_bit);
+
+static inline void ceph_queue_vmtruncate(struct inode *inode)
+{
+	ceph_queue_inode_work(inode, CEPH_I_WORK_VMTRUNCATE);
+}
+
+static inline void ceph_queue_invalidate(struct inode *inode)
+{
+	ceph_queue_inode_work(inode, CEPH_I_WORK_INVALIDATE_PAGES);
+}
+
+static inline void ceph_queue_writeback(struct inode *inode)
+{
+	ceph_queue_inode_work(inode, CEPH_I_WORK_WRITEBACK);
+}
+
 extern int __ceph_do_getattr(struct inode *inode, struct page *locked_page,
 			     int mask, bool force);
 static inline int ceph_do_getattr(struct inode *inode, int mask, bool force)
-- 
2.29.2

[PATCH 3/3] ceph: allow queueing cap/snap handling after putting cap references

[Jeff Layton]

Testing with the fscache overhaul has triggered some lockdep warnings
about circular lock dependencies involving page_mkwrite and the
mmap_lock. It'd be better to do the "real work" without the mmap lock
being held.

Change the skip_checking_caps parameter in __ceph_put_cap_refs to an
enum, and use that to determine whether to queue check_caps, do it
synchronously or not at all. Change ceph_page_mkwrite to do a
ceph_put_cap_refs_async().

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/addr.c  |  2 +-
 fs/ceph/caps.c  | 28 ++++++++++++++++++++++++----
 fs/ceph/inode.c |  6 ++++++
 fs/ceph/super.h | 19 ++++++++++++++++---
 4 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 950552944436..26e66436f005 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -1662,7 +1662,7 @@ static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf)
 
 	dout("page_mkwrite %p %llu~%zd dropping cap refs on %s ret %x\n",
 	     inode, off, len, ceph_cap_string(got), ret);
-	ceph_put_cap_refs(ci, got);
+	ceph_put_cap_refs_async(ci, got);
 out_free:
 	ceph_restore_sigs(&oldset);
 	sb_end_pagefault(inode->i_sb);
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 336348e733b9..a95ab4c02056 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -3026,6 +3026,12 @@ static int ceph_try_drop_cap_snap(struct ceph_inode_info *ci,
 	return 0;
 }
 
+enum PutCapRefsMode {
+	PutCapRefsModeSync = 0,
+	PutCapRefsModeSkip,
+	PutCapRefsModeAsync,
+};
+
 /*
  * Release cap refs.
  *
@@ -3036,7 +3042,7 @@ static int ceph_try_drop_cap_snap(struct ceph_inode_info *ci,
  * cap_snap, and wake up any waiters.
  */
 static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
-				bool skip_checking_caps)
+				enum PutCapRefsMode mode)
 {
 	struct inode *inode = &ci->vfs_inode;
 	int last = 0, put = 0, flushsnaps = 0, wake = 0;
@@ -3092,11 +3098,20 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
 	dout("put_cap_refs %p had %s%s%s\n", inode, ceph_cap_string(had),
 	     last ? " last" : "", put ? " put" : "");
 
-	if (!skip_checking_caps) {
+	switch(mode) {
+	default:
+		break;
+	case PutCapRefsModeSync:
 		if (last)
 			ceph_check_caps(ci, 0, NULL);
 		else if (flushsnaps)
 			ceph_flush_snaps(ci, NULL);
+		break;
+	case PutCapRefsModeAsync:
+		if (last)
+			ceph_queue_check_caps(inode);
+		else if (flushsnaps)
+			ceph_queue_flush_snaps(inode);
 	}
 	if (wake)
 		wake_up_all(&ci->i_cap_wq);
@@ -3106,12 +3121,17 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
 
 void ceph_put_cap_refs(struct ceph_inode_info *ci, int had)
 {
-	__ceph_put_cap_refs(ci, had, false);
+	__ceph_put_cap_refs(ci, had, PutCapRefsModeSync);
+}
+
+void ceph_put_cap_refs_async(struct ceph_inode_info *ci, int had)
+{
+	__ceph_put_cap_refs(ci, had, PutCapRefsModeAsync);
 }
 
 void ceph_put_cap_refs_no_check_caps(struct ceph_inode_info *ci, int had)
 {
-	__ceph_put_cap_refs(ci, had, true);
+	__ceph_put_cap_refs(ci, had, PutCapRefsModeSkip);
 }
 
 /*
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 9cd8b37e586a..a7ee19e27e26 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1965,6 +1965,12 @@ static void ceph_inode_work(struct work_struct *work)
 	if (test_and_clear_bit(CEPH_I_WORK_VMTRUNCATE, &ci->i_work_mask))
 		__ceph_do_pending_vmtruncate(inode);
 
+	if (test_and_clear_bit(CEPH_I_WORK_CHECK_CAPS, &ci->i_work_mask))
+		ceph_check_caps(ci, 0, NULL);
+
+	if (test_and_clear_bit(CEPH_I_WORK_FLUSH_SNAPS, &ci->i_work_mask))
+		ceph_flush_snaps(ci, NULL);
+
 	iput(inode);
 }
 
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 59153ee201c0..13b02887b085 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -562,9 +562,11 @@ static inline struct inode *ceph_find_inode(struct super_block *sb,
 /*
  * Masks of ceph inode work.
  */
-#define CEPH_I_WORK_WRITEBACK		0 /* writeback */
-#define CEPH_I_WORK_INVALIDATE_PAGES	1 /* invalidate pages */
-#define CEPH_I_WORK_VMTRUNCATE		2 /* vmtruncate */
+#define CEPH_I_WORK_WRITEBACK		0
+#define CEPH_I_WORK_INVALIDATE_PAGES	1
+#define CEPH_I_WORK_VMTRUNCATE		2
+#define CEPH_I_WORK_CHECK_CAPS		3
+#define CEPH_I_WORK_FLUSH_SNAPS		4
 
 /*
  * We set the ERROR_WRITE bit when we start seeing write errors on an inode
@@ -982,6 +984,16 @@ static inline void ceph_queue_writeback(struct inode *inode)
 	ceph_queue_inode_work(inode, CEPH_I_WORK_WRITEBACK);
 }
 
+static inline void ceph_queue_check_caps(struct inode *inode)
+{
+	ceph_queue_inode_work(inode, CEPH_I_WORK_CHECK_CAPS);
+}
+
+static inline void ceph_queue_flush_snaps(struct inode *inode)
+{
+	ceph_queue_inode_work(inode, CEPH_I_WORK_FLUSH_SNAPS);
+}
+
 extern int __ceph_do_getattr(struct inode *inode, struct page *locked_page,
 			     int mask, bool force);
 static inline int ceph_do_getattr(struct inode *inode, int mask, bool force)
@@ -1120,6 +1132,7 @@ extern void ceph_take_cap_refs(struct ceph_inode_info *ci, int caps,
 				bool snap_rwsem_locked);
 extern void ceph_get_cap_refs(struct ceph_inode_info *ci, int caps);
 extern void ceph_put_cap_refs(struct ceph_inode_info *ci, int had);
+extern void ceph_put_cap_refs_async(struct ceph_inode_info *ci, int had);
 extern void ceph_put_cap_refs_no_check_caps(struct ceph_inode_info *ci,
 					    int had);
 extern void ceph_put_wrbuffer_cap_refs(struct ceph_inode_info *ci, int nr,
-- 
2.29.2

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

[Luis Henriques]

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,
-- 
Luis

> +		dout("queue_inode_work %p, mask=%lx\n", inode, ci->i_work_mask);
>  	} else {
> -		dout("ceph_queue_vmtruncate %p already queued, mask=%lx\n",
> +		dout("queue_inode_work %p already queued, mask=%lx\n",
>  		     inode, ci->i_work_mask);
>  		iput(inode);
>  	}
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index b62d8fee3b86..59153ee201c0 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -962,11 +962,26 @@ extern int ceph_inode_holds_cap(struct inode *inode, int mask);
>  
>  extern bool ceph_inode_set_size(struct inode *inode, loff_t size);
>  extern void __ceph_do_pending_vmtruncate(struct inode *inode);
> -extern void ceph_queue_vmtruncate(struct inode *inode);
> -extern void ceph_queue_invalidate(struct inode *inode);
> -extern void ceph_queue_writeback(struct inode *inode);
> +
>  extern void ceph_async_iput(struct inode *inode);
>  
> +void ceph_queue_inode_work(struct inode *inode, int work_bit);
> +
> +static inline void ceph_queue_vmtruncate(struct inode *inode)
> +{
> +	ceph_queue_inode_work(inode, CEPH_I_WORK_VMTRUNCATE);
> +}
> +
> +static inline void ceph_queue_invalidate(struct inode *inode)
> +{
> +	ceph_queue_inode_work(inode, CEPH_I_WORK_INVALIDATE_PAGES);
> +}
> +
> +static inline void ceph_queue_writeback(struct inode *inode)
> +{
> +	ceph_queue_inode_work(inode, CEPH_I_WORK_WRITEBACK);
> +}
> +
>  extern int __ceph_do_getattr(struct inode *inode, struct page *locked_page,
>  			     int mask, bool force);
>  static inline int ceph_do_getattr(struct inode *inode, int mask, bool force)
> -- 
>
> 2.29.2
>

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

[Jeff Layton]

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>

Re: [PATCH 3/3] ceph: allow queueing cap/snap handling after putting cap references

[Ilya Dryomov]

On Fri, Dec 11, 2020 at 1:39 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> Testing with the fscache overhaul has triggered some lockdep warnings
> about circular lock dependencies involving page_mkwrite and the
> mmap_lock. It'd be better to do the "real work" without the mmap lock
> being held.
>
> Change the skip_checking_caps parameter in __ceph_put_cap_refs to an
> enum, and use that to determine whether to queue check_caps, do it
> synchronously or not at all. Change ceph_page_mkwrite to do a
> ceph_put_cap_refs_async().
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/addr.c  |  2 +-
>  fs/ceph/caps.c  | 28 ++++++++++++++++++++++++----
>  fs/ceph/inode.c |  6 ++++++
>  fs/ceph/super.h | 19 ++++++++++++++++---
>  4 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 950552944436..26e66436f005 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -1662,7 +1662,7 @@ static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf)
>
>         dout("page_mkwrite %p %llu~%zd dropping cap refs on %s ret %x\n",
>              inode, off, len, ceph_cap_string(got), ret);
> -       ceph_put_cap_refs(ci, got);
> +       ceph_put_cap_refs_async(ci, got);
>  out_free:
>         ceph_restore_sigs(&oldset);
>         sb_end_pagefault(inode->i_sb);
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 336348e733b9..a95ab4c02056 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3026,6 +3026,12 @@ static int ceph_try_drop_cap_snap(struct ceph_inode_info *ci,
>         return 0;
>  }
>
> +enum PutCapRefsMode {
> +       PutCapRefsModeSync = 0,
> +       PutCapRefsModeSkip,
> +       PutCapRefsModeAsync,
> +};

Hi Jeff,

A couple style nits, since mixed case stood out ;)

Let's avoid CamelCase.  Page flags and existing protocol definitions
like SMB should be the only exception.  I'd suggest PUT_CAP_REFS_SYNC,
etc.

> +
>  /*
>   * Release cap refs.
>   *
> @@ -3036,7 +3042,7 @@ static int ceph_try_drop_cap_snap(struct ceph_inode_info *ci,
>   * cap_snap, and wake up any waiters.
>   */
>  static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
> -                               bool skip_checking_caps)
> +                               enum PutCapRefsMode mode)
>  {
>         struct inode *inode = &ci->vfs_inode;
>         int last = 0, put = 0, flushsnaps = 0, wake = 0;
> @@ -3092,11 +3098,20 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
>         dout("put_cap_refs %p had %s%s%s\n", inode, ceph_cap_string(had),
>              last ? " last" : "", put ? " put" : "");
>
> -       if (!skip_checking_caps) {
> +       switch(mode) {
> +       default:
> +               break;
> +       case PutCapRefsModeSync:
>                 if (last)
>                         ceph_check_caps(ci, 0, NULL);
>                 else if (flushsnaps)
>                         ceph_flush_snaps(ci, NULL);
> +               break;
> +       case PutCapRefsModeAsync:
> +               if (last)
> +                       ceph_queue_check_caps(inode);
> +               else if (flushsnaps)
> +                       ceph_queue_flush_snaps(inode);

Add a break here.  I'd also move the default clause to the end.

>         }
>         if (wake)
>                 wake_up_all(&ci->i_cap_wq);
> @@ -3106,12 +3121,17 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
>
>  void ceph_put_cap_refs(struct ceph_inode_info *ci, int had)
>  {
> -       __ceph_put_cap_refs(ci, had, false);
> +       __ceph_put_cap_refs(ci, had, PutCapRefsModeSync);
> +}
> +
> +void ceph_put_cap_refs_async(struct ceph_inode_info *ci, int had)
> +{
> +       __ceph_put_cap_refs(ci, had, PutCapRefsModeAsync);
>  }
>
>  void ceph_put_cap_refs_no_check_caps(struct ceph_inode_info *ci, int had)
>  {
> -       __ceph_put_cap_refs(ci, had, true);
> +       __ceph_put_cap_refs(ci, had, PutCapRefsModeSkip);

Perhaps name the enum member PUT_CAP_REFS_NO_CHECK to match the
exported function?

Thanks,

                Ilya

Re: [PATCH 3/3] ceph: allow queueing cap/snap handling after putting cap references

[Jeff Layton]

On Fri, Dec 18, 2020 at 03:22:51PM +0100, Ilya Dryomov wrote:
> On Fri, Dec 11, 2020 at 1:39 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > Testing with the fscache overhaul has triggered some lockdep warnings
> > about circular lock dependencies involving page_mkwrite and the
> > mmap_lock. It'd be better to do the "real work" without the mmap lock
> > being held.
> >
> > Change the skip_checking_caps parameter in __ceph_put_cap_refs to an
> > enum, and use that to determine whether to queue check_caps, do it
> > synchronously or not at all. Change ceph_page_mkwrite to do a
> > ceph_put_cap_refs_async().
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/addr.c  |  2 +-
> >  fs/ceph/caps.c  | 28 ++++++++++++++++++++++++----
> >  fs/ceph/inode.c |  6 ++++++
> >  fs/ceph/super.h | 19 ++++++++++++++++---
> >  4 files changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index 950552944436..26e66436f005 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -1662,7 +1662,7 @@ static vm_fault_t ceph_page_mkwrite(struct vm_fault *vmf)
> >
> >         dout("page_mkwrite %p %llu~%zd dropping cap refs on %s ret %x\n",
> >              inode, off, len, ceph_cap_string(got), ret);
> > -       ceph_put_cap_refs(ci, got);
> > +       ceph_put_cap_refs_async(ci, got);
> >  out_free:
> >         ceph_restore_sigs(&oldset);
> >         sb_end_pagefault(inode->i_sb);
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 336348e733b9..a95ab4c02056 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -3026,6 +3026,12 @@ static int ceph_try_drop_cap_snap(struct ceph_inode_info *ci,
> >         return 0;
> >  }
> >
> > +enum PutCapRefsMode {
> > +       PutCapRefsModeSync = 0,
> > +       PutCapRefsModeSkip,
> > +       PutCapRefsModeAsync,
> > +};
> 
> Hi Jeff,
> 
> A couple style nits, since mixed case stood out ;)
> 
> Let's avoid CamelCase.  Page flags and existing protocol definitions
> like SMB should be the only exception.  I'd suggest PUT_CAP_REFS_SYNC,
> etc.
> 
> > +
> >  /*
> >   * Release cap refs.
> >   *
> > @@ -3036,7 +3042,7 @@ static int ceph_try_drop_cap_snap(struct ceph_inode_info *ci,
> >   * cap_snap, and wake up any waiters.
> >   */
> >  static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
> > -                               bool skip_checking_caps)
> > +                               enum PutCapRefsMode mode)
> >  {
> >         struct inode *inode = &ci->vfs_inode;
> >         int last = 0, put = 0, flushsnaps = 0, wake = 0;
> > @@ -3092,11 +3098,20 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
> >         dout("put_cap_refs %p had %s%s%s\n", inode, ceph_cap_string(had),
> >              last ? " last" : "", put ? " put" : "");
> >
> > -       if (!skip_checking_caps) {
> > +       switch(mode) {
> > +       default:
> > +               break;
> > +       case PutCapRefsModeSync:
> >                 if (last)
> >                         ceph_check_caps(ci, 0, NULL);
> >                 else if (flushsnaps)
> >                         ceph_flush_snaps(ci, NULL);
> > +               break;
> > +       case PutCapRefsModeAsync:
> > +               if (last)
> > +                       ceph_queue_check_caps(inode);
> > +               else if (flushsnaps)
> > +                       ceph_queue_flush_snaps(inode);
> 
> Add a break here.  I'd also move the default clause to the end.
> 
> >         }
> >         if (wake)
> >                 wake_up_all(&ci->i_cap_wq);
> > @@ -3106,12 +3121,17 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had,
> >
> >  void ceph_put_cap_refs(struct ceph_inode_info *ci, int had)
> >  {
> > -       __ceph_put_cap_refs(ci, had, false);
> > +       __ceph_put_cap_refs(ci, had, PutCapRefsModeSync);
> > +}
> > +
> > +void ceph_put_cap_refs_async(struct ceph_inode_info *ci, int had)
> > +{
> > +       __ceph_put_cap_refs(ci, had, PutCapRefsModeAsync);
> >  }
> >
> >  void ceph_put_cap_refs_no_check_caps(struct ceph_inode_info *ci, int had)
> >  {
> > -       __ceph_put_cap_refs(ci, had, true);
> > +       __ceph_put_cap_refs(ci, had, PutCapRefsModeSkip);
> 
> Perhaps name the enum member PUT_CAP_REFS_NO_CHECK to match the
> exported function?
> 
> Thanks,
> 
>                 Ilya

That all sounds reasonable. I'll send a v2 patch here in a few mins.

Thanks,
Jeff