From: Jeff Layton <jlayton@redhat.com>
To: "Yan, Zheng" <zyan@redhat.com>
Cc: ceph-devel <ceph-devel@vger.kernel.org>,
Sage Weil <sage@redhat.com>,
idryomov@gmail.com
Subject: Re: [PATCH v3 4/8] ceph: add a new flag to indicate whether parent is locked
Date: Sat, 04 Feb 2017 06:40:37 -0500 [thread overview]
Message-ID: <1486208437.16508.3.camel@redhat.com> (raw)
In-Reply-To: <AA245BF2-C788-4AF1-B2B2-10A76FEE30B5@redhat.com>
On Sat, 2017-02-04 at 11:16 +0800, Yan, Zheng wrote:
> > On 4 Feb 2017, at 02:04, Jeff Layton <jlayton@redhat.com> wrote:
> >
> > struct ceph_mds_request has an r_locked_dir pointer, which is set to
> > indicate the parent inode and that its i_rwsem is locked. In some
> > critical places, we need to be able to indicate the parent inode to the
> > request handling code, even when its i_rwsem may not be locked.
> >
> > Most of the code that operates on r_locked_dir doesn't require that the
> > i_rwsem be locked. We only really need it to handle manipulation of the
> > dcache. The rest (filling of the inode, updating dentry leases, etc.)
> > already has its own locking.
> >
> > Add a new r_req_flags bit that indicates whether the parent is locked
> > when doing the request, and rename the pointer to "r_parent". For now,
> > all the places that set r_parent also set this flag, but that will
> > change in a later patch.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > fs/ceph/dir.c | 21 ++++++++++++++-------
> > fs/ceph/export.c | 3 ++-
> > fs/ceph/file.c | 3 ++-
> > fs/ceph/inode.c | 17 ++++++++++-------
> > fs/ceph/mds_client.c | 24 ++++++++++++------------
> > fs/ceph/mds_client.h | 3 ++-
> > 6 files changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index 5fb86d71220e..c8562b8a8c7e 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -752,7 +752,8 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
> > mask |= CEPH_CAP_XATTR_SHARED;
> > req->r_args.getattr.mask = cpu_to_le32(mask);
> >
> > - req->r_locked_dir = dir;
> > + req->r_parent = dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > err = ceph_mdsc_do_request(mdsc, NULL, req);
> > err = ceph_handle_snapdir(req, dentry, err);
> > dentry = ceph_finish_lookup(req, dentry, err);
> > @@ -813,7 +814,8 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
> > }
> > req->r_dentry = dget(dentry);
> > req->r_num_caps = 2;
> > - req->r_locked_dir = dir;
> > + req->r_parent = dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > req->r_args.mknod.mode = cpu_to_le32(mode);
> > req->r_args.mknod.rdev = cpu_to_le32(rdev);
> > req->r_dentry_drop = CEPH_CAP_FILE_SHARED;
> > @@ -864,7 +866,8 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
> > ceph_mdsc_put_request(req);
> > goto out;
> > }
> > - req->r_locked_dir = dir;
> > + req->r_parent = dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > req->r_dentry = dget(dentry);
> > req->r_num_caps = 2;
> > req->r_dentry_drop = CEPH_CAP_FILE_SHARED;
> > @@ -913,7 +916,8 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
> >
> > req->r_dentry = dget(dentry);
> > req->r_num_caps = 2;
> > - req->r_locked_dir = dir;
> > + req->r_parent = dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > req->r_args.mkdir.mode = cpu_to_le32(mode);
> > req->r_dentry_drop = CEPH_CAP_FILE_SHARED;
> > req->r_dentry_unless = CEPH_CAP_FILE_EXCL;
> > @@ -957,7 +961,8 @@ static int ceph_link(struct dentry *old_dentry, struct inode *dir,
> > req->r_dentry = dget(dentry);
> > req->r_num_caps = 2;
> > req->r_old_dentry = dget(old_dentry);
> > - req->r_locked_dir = dir;
> > + req->r_parent = dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > req->r_dentry_drop = CEPH_CAP_FILE_SHARED;
> > req->r_dentry_unless = CEPH_CAP_FILE_EXCL;
> > /* release LINK_SHARED on source inode (mds will lock it) */
> > @@ -1023,7 +1028,8 @@ static int ceph_unlink(struct inode *dir, struct dentry *dentry)
> > }
> > req->r_dentry = dget(dentry);
> > req->r_num_caps = 2;
> > - req->r_locked_dir = dir;
> > + req->r_parent = dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > req->r_dentry_drop = CEPH_CAP_FILE_SHARED;
> > req->r_dentry_unless = CEPH_CAP_FILE_EXCL;
> > req->r_inode_drop = drop_caps_for_unlink(inode);
> > @@ -1066,7 +1072,8 @@ static int ceph_rename(struct inode *old_dir, struct dentry *old_dentry,
> > req->r_num_caps = 2;
> > req->r_old_dentry = dget(old_dentry);
> > req->r_old_dentry_dir = old_dir;
> > - req->r_locked_dir = new_dir;
> > + req->r_parent = new_dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > req->r_old_dentry_drop = CEPH_CAP_FILE_SHARED;
> > req->r_old_dentry_unless = CEPH_CAP_FILE_EXCL;
> > req->r_dentry_drop = CEPH_CAP_FILE_SHARED;
> > diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> > index 180bbef760f2..e8f11fa565c5 100644
> > --- a/fs/ceph/export.c
> > +++ b/fs/ceph/export.c
> > @@ -207,7 +207,8 @@ static int ceph_get_name(struct dentry *parent, char *name,
> > req->r_inode = d_inode(child);
> > ihold(d_inode(child));
> > req->r_ino2 = ceph_vino(d_inode(parent));
> > - req->r_locked_dir = d_inode(parent);
> > + req->r_parent = d_inode(parent);
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > req->r_num_caps = 2;
> > err = ceph_mdsc_do_request(mdsc, NULL, req);
> >
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index 19419eb87f38..a91239c4b31c 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -310,7 +310,8 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> > mask |= CEPH_CAP_XATTR_SHARED;
> > req->r_args.open.mask = cpu_to_le32(mask);
> >
> > - req->r_locked_dir = dir; /* caller holds dir->i_mutex */
> > + req->r_parent = dir;
> > + set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> > err = ceph_mdsc_do_request(mdsc,
> > (flags & (O_CREAT|O_TRUNC)) ? dir : NULL,
> > req);
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index e54ba03d3df7..b4b61da03dbf 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -1122,13 +1122,13 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> >
> > if (!rinfo->head->is_target && !rinfo->head->is_dentry) {
> > dout("fill_trace reply is empty!\n");
> > - if (rinfo->head->result == 0 && req->r_locked_dir)
> > + if (rinfo->head->result == 0 && req->r_parent)
> > ceph_invalidate_dir_request(req);
> > return 0;
> > }
> >
> > if (rinfo->head->is_dentry) {
> > - struct inode *dir = req->r_locked_dir;
> > + struct inode *dir = req->r_parent;
> >
> > if (dir) {
> > err = fill_inode(dir, NULL,
> > @@ -1216,7 +1216,9 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> > * ignore null lease/binding on snapdir ENOENT, or else we
> > * will have trouble splicing in the virtual snapdir later
> > */
> > - if (rinfo->head->is_dentry && req->r_locked_dir &&
> > + if (rinfo->head->is_dentry &&
> > + !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags) &&
>
> why adding the !ABORTED check back. I think previous patch add:
>
> if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
> goto done;
>
Oof, good catch -- I screwed that up.
We want to keep the R_ABORTED checks where r_aborted is checked now, as
that will allow us to update the dentry leases even when the task that
spawned the call was aborted.
I'll fix that in the ceph-client/testing branch, but I'll not bother
reposting unless there are other problems that need fixing in here.
> > + test_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags) &&
> > (rinfo->head->is_target || strncmp(req->r_dentry->d_name.name,
> > fsc->mount_options->snapdir_name,
> > req->r_dentry->d_name.len))) {
> > @@ -1225,7 +1227,7 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> > * mknod symlink mkdir : null -> new inode
> > * unlink : linked -> null
> > */
> > - struct inode *dir = req->r_locked_dir;
> > + struct inode *dir = req->r_parent;
> > struct dentry *dn = req->r_dentry;
> > bool have_dir_cap, have_lease;
> >
> > @@ -1319,10 +1321,11 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> > update_dentry_lease(dn, rinfo->dlease, session,
> > req->r_request_started);
> > dout(" final dn %p\n", dn);
> > - } else if (req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||
> > - req->r_op == CEPH_MDS_OP_MKSNAP) {
> > + } else if ((req->r_op == CEPH_MDS_OP_LOOKUPSNAP ||
> > + req->r_op == CEPH_MDS_OP_MKSNAP) &&
> > + !test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) {
>
> same here
>
> > struct dentry *dn = req->r_dentry;
> > - struct inode *dir = req->r_locked_dir;
> > + struct inode *dir = req->r_parent;
> >
> > /* fill out a snapdir LOOKUPSNAP dentry */
> > BUG_ON(!dn);
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index ccf75a3260e8..52521f339745 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -547,8 +547,8 @@ void ceph_mdsc_release_request(struct kref *kref)
> > ceph_put_cap_refs(ceph_inode(req->r_inode), CEPH_CAP_PIN);
> > iput(req->r_inode);
> > }
> > - if (req->r_locked_dir)
> > - ceph_put_cap_refs(ceph_inode(req->r_locked_dir), CEPH_CAP_PIN);
> > + if (req->r_parent)
> > + ceph_put_cap_refs(ceph_inode(req->r_parent), CEPH_CAP_PIN);
> > iput(req->r_target_inode);
> > if (req->r_dentry)
> > dput(req->r_dentry);
> > @@ -735,7 +735,7 @@ static int __choose_mds(struct ceph_mds_client *mdsc,
> >
> > rcu_read_lock();
> > parent = req->r_dentry->d_parent;
> > - dir = req->r_locked_dir ? : d_inode_rcu(parent);
> > + dir = req->r_parent ? : d_inode_rcu(parent);
> >
> > if (!dir || dir->i_sb != mdsc->fsc->sb) {
> > /* not this fs or parent went negative */
> > @@ -1894,7 +1894,7 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
> > int ret;
> >
> > ret = set_request_path_attr(req->r_inode, req->r_dentry,
> > - req->r_locked_dir, req->r_path1, req->r_ino1.ino,
> > + req->r_parent, req->r_path1, req->r_ino1.ino,
> > &path1, &pathlen1, &ino1, &freepath1);
> > if (ret < 0) {
> > msg = ERR_PTR(ret);
> > @@ -1956,7 +1956,7 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
> > mds, req->r_inode_drop, req->r_inode_unless, 0);
> > if (req->r_dentry_drop)
> > releases += ceph_encode_dentry_release(&p, req->r_dentry,
> > - req->r_locked_dir, mds, req->r_dentry_drop,
> > + req->r_parent, mds, req->r_dentry_drop,
> > req->r_dentry_unless);
> > if (req->r_old_dentry_drop)
> > releases += ceph_encode_dentry_release(&p, req->r_old_dentry,
> > @@ -2095,14 +2095,14 @@ static int __prepare_send_request(struct ceph_mds_client *mdsc,
> > rhead->oldest_client_tid = cpu_to_le64(__get_oldest_tid(mdsc));
> > if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags))
> > flags |= CEPH_MDS_FLAG_REPLAY;
> > - if (req->r_locked_dir)
> > + if (req->r_parent)
> > flags |= CEPH_MDS_FLAG_WANT_DENTRY;
> > rhead->flags = cpu_to_le32(flags);
> > rhead->num_fwd = req->r_num_fwd;
> > rhead->num_retry = req->r_attempts - 1;
> > rhead->ino = 0;
> >
> > - dout(" r_locked_dir = %p\n", req->r_locked_dir);
> > + dout(" r_parent = %p\n", req->r_parent);
> > return 0;
> > }
> >
> > @@ -2282,11 +2282,11 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
> >
> > dout("do_request on %p\n", req);
> >
> > - /* take CAP_PIN refs for r_inode, r_locked_dir, r_old_dentry */
> > + /* take CAP_PIN refs for r_inode, r_parent, r_old_dentry */
> > if (req->r_inode)
> > ceph_get_cap_refs(ceph_inode(req->r_inode), CEPH_CAP_PIN);
> > - if (req->r_locked_dir)
> > - ceph_get_cap_refs(ceph_inode(req->r_locked_dir), CEPH_CAP_PIN);
> > + if (req->r_parent)
> > + ceph_get_cap_refs(ceph_inode(req->r_parent), CEPH_CAP_PIN);
> > if (req->r_old_dentry_dir)
> > ceph_get_cap_refs(ceph_inode(req->r_old_dentry_dir),
> > CEPH_CAP_PIN);
> > @@ -2336,7 +2336,7 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
> > set_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags);
> > mutex_unlock(&req->r_fill_mutex);
> >
> > - if (req->r_locked_dir &&
> > + if (req->r_parent &&
> > (req->r_op & CEPH_MDS_OP_WRITE))
> > ceph_invalidate_dir_request(req);
> > } else {
> > @@ -2355,7 +2355,7 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
> > */
> > void ceph_invalidate_dir_request(struct ceph_mds_request *req)
> > {
> > - struct inode *inode = req->r_locked_dir;
> > + struct inode *inode = req->r_parent;
> >
> > dout("invalidate_dir_request %p (complete, lease(s))\n", inode);
> >
> > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > index 409b0e3c3b7a..ac0475a2daa7 100644
> > --- a/fs/ceph/mds_client.h
> > +++ b/fs/ceph/mds_client.h
> > @@ -202,7 +202,7 @@ struct ceph_mds_request {
> > char *r_path1, *r_path2;
> > struct ceph_vino r_ino1, r_ino2;
> >
> > - struct inode *r_locked_dir; /* dir (if any) i_mutex locked by vfs */
> > + struct inode *r_parent; /* parent dir inode */
> > struct inode *r_target_inode; /* resulting inode */
> >
> > #define CEPH_MDS_R_DIRECT_IS_HASH (1) /* r_direct_hash is valid */
> > @@ -211,6 +211,7 @@ struct ceph_mds_request {
> > #define CEPH_MDS_R_GOT_SAFE (4) /* got a safe reply */
> > #define CEPH_MDS_R_GOT_RESULT (5) /* got a result */
> > #define CEPH_MDS_R_DID_PREPOPULATE (6) /* prepopulated readdir */
> > +#define CEPH_MDS_R_PARENT_LOCKED (7) /* is r_parent->i_rwsem wlocked? */
> > unsigned long r_req_flags;
> >
> > struct mutex r_fill_mutex;
> > --
> > 2.9.3
> >
>
>
--
Jeff Layton <jlayton@redhat.com>
next prev parent reply other threads:[~2017-02-04 11:40 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-30 16:19 [PATCH 0/5] ceph: fix performance regression due to ceph_d_revalidate fixes Jeff Layton
2017-01-30 16:19 ` [PATCH 1/5] ceph: don't update_dentry_lease unless we actually got one Jeff Layton
2017-01-30 16:19 ` [PATCH 2/5] ceph: vet the parent inode before updating dentry lease Jeff Layton
2017-01-30 16:19 ` [PATCH 3/5] ceph: clean up r_aborted handling in ceph_fill_trace Jeff Layton
2017-01-30 16:19 ` [PATCH 4/5] ceph: call update_dentry_lease even when r_locked dir is not set Jeff Layton
2017-01-30 16:19 ` [PATCH 5/5] ceph: do a LOOKUP in d_revalidate instead of GETATTR Jeff Layton
2017-02-01 11:49 ` [PATCH v2 00/13] ceph: fix performance regression due to ceph_d_revalidate fixes Jeff Layton
2017-02-01 11:49 ` [PATCH v2 01/13] ceph: add new r_req_flags field to ceph_mds_request Jeff Layton
2017-02-01 14:08 ` Ilya Dryomov
2017-02-01 17:47 ` Jeff Layton
2017-02-01 19:10 ` Ilya Dryomov
2017-02-01 19:14 ` Jeff Layton
2017-02-01 11:49 ` [PATCH v2 02/13] ceph: move r_aborted flag into r_req_flags Jeff Layton
2017-02-02 8:06 ` Yan, Zheng
2017-02-02 11:26 ` Jeff Layton
2017-02-02 15:16 ` Yan, Zheng
2017-02-02 15:27 ` Jeff Layton
2017-02-01 11:49 ` [PATCH v2 03/13] ceph: move r_got_unsafe " Jeff Layton
2017-02-01 11:49 ` [PATCH v2 04/13] ceph: move r_got_safe " Jeff Layton
2017-02-01 11:49 ` [PATCH v2 05/13] ceph: move r_got_result " Jeff Layton
2017-02-01 11:49 ` [PATCH v2 06/13] ceph: move r_did_prepopulate " Jeff Layton
2017-02-01 11:49 ` [PATCH v2 07/13] ceph: remove "Debugging hook" from ceph_fill_trace Jeff Layton
2017-02-01 11:49 ` [PATCH v2 08/13] ceph: don't need session pointer to ceph_fill_trace Jeff Layton
2017-02-01 11:49 ` [PATCH v2 09/13] ceph: add a new flag to indicate whether parent is locked Jeff Layton
2017-02-01 11:49 ` [PATCH v2 10/13] ceph: don't update_dentry_lease unless we actually got one Jeff Layton
2017-02-01 11:49 ` [PATCH v2 11/13] ceph: vet the parent inode before updating dentry lease Jeff Layton
2017-02-01 11:49 ` [PATCH v2 12/13] ceph: call update_dentry_lease even when r_locked dir is not set Jeff Layton
2017-02-02 8:34 ` Yan, Zheng
2017-02-02 11:27 ` Jeff Layton
2017-02-01 11:49 ` [PATCH v2 13/13] ceph: do a LOOKUP in d_revalidate instead of GETATTR Jeff Layton
2017-02-03 18:03 ` [PATCH v3 0/8] ceph: fix performance regression due to ceph_d_revalidate fixes Jeff Layton
2017-02-03 18:03 ` [PATCH v3 1/8] ceph: remove "Debugging hook" from ceph_fill_trace Jeff Layton
2017-02-03 18:03 ` [PATCH v3 2/8] ceph: drop session argument to ceph_fill_trace Jeff Layton
2017-02-03 18:03 ` [PATCH v3 3/8] ceph: convert bools in ceph_mds_request to a new r_req_flags field Jeff Layton
2017-02-03 18:04 ` [PATCH v3 4/8] ceph: add a new flag to indicate whether parent is locked Jeff Layton
2017-02-04 3:16 ` Yan, Zheng
2017-02-04 11:40 ` Jeff Layton [this message]
2017-02-03 18:04 ` [PATCH v3 5/8] ceph: don't update_dentry_lease unless we actually got one Jeff Layton
2017-02-03 18:04 ` [PATCH v3 6/8] ceph: vet the target and parent inodes before updating dentry lease Jeff Layton
2017-02-04 3:20 ` Yan, Zheng
2017-02-04 11:37 ` Jeff Layton
2017-02-03 18:04 ` [PATCH v3 7/8] ceph: call update_dentry_lease even when r_locked dir is not set Jeff Layton
2017-02-03 18:04 ` [PATCH v3 8/8] ceph: do a LOOKUP in d_revalidate instead of GETATTR Jeff Layton
2017-02-04 3:25 ` [PATCH v3 0/8] ceph: fix performance regression due to ceph_d_revalidate fixes Yan, Zheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1486208437.16508.3.camel@redhat.com \
--to=jlayton@redhat.com \
--cc=ceph-devel@vger.kernel.org \
--cc=idryomov@gmail.com \
--cc=sage@redhat.com \
--cc=zyan@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.