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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
