diff mbox series

[RFC,04/11] ceph: hold extra reference to r_parent over life of request

Message ID 20190409194229.8247-5-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series [RFC,01/11] ceph: after an MDS request, do callback and completions | expand

Commit Message

Jeff Layton April 9, 2019, 7:42 p.m. UTC 
Currently, we just assume that it will stick around by virtue of the
submitter's reference, but later patches will allow the syscall to
return early and we can't rely on that reference at that point.

Take an extra reference to the inode when setting r_parent and release
it when releasing the request.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/dir.c        | 8 ++++++++
 fs/ceph/export.c     | 1 +
 fs/ceph/file.c       | 1 +
 fs/ceph/mds_client.c | 4 +++-
 4 files changed, 13 insertions(+), 1 deletion(-)

Comments

Luis Henriques April 10, 2019, 10:27 a.m. UTC | #1 
Jeff Layton <jlayton@kernel.org> writes:

> Currently, we just assume that it will stick around by virtue of the
> submitter's reference, but later patches will allow the syscall to
> return early and we can't rely on that reference at that point.
>
> Take an extra reference to the inode when setting r_parent and release
> it when releasing the request.

Couldn't you move all these ihold into ceph_mdsc_do_request?  I took a
quick look and it seems like it could use a logic similar to what's in
ceph_mdsc_release_request, i.e.:

	if (req->r_parent)
        	ihold(req->r_parent);

Cheers,
Jeff Layton April 10, 2019, 10:34 a.m. UTC | #2 
On Wed, Apr 10, 2019 at 6:27 AM Luis Henriques <lhenriques@suse.com> wrote:
>
> Jeff Layton <jlayton@kernel.org> writes:
>
> > Currently, we just assume that it will stick around by virtue of the
> > submitter's reference, but later patches will allow the syscall to
> > return early and we can't rely on that reference at that point.
> >
> > Take an extra reference to the inode when setting r_parent and release
> > it when releasing the request.
>
> Couldn't you move all these ihold into ceph_mdsc_do_request?  I took a
> quick look and it seems like it could use a logic similar to what's in
> ceph_mdsc_release_request, i.e.:
>
>         if (req->r_parent)
>                 ihold(req->r_parent);
>
> Cheers,
> --
> Luis
>
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/dir.c        | 8 ++++++++
> >  fs/ceph/export.c     | 1 +
> >  fs/ceph/file.c       | 1 +
> >  fs/ceph/mds_client.c | 4 +++-
> >  4 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index a8f429882249..3eb9bc226b77 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -783,6 +783,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
> >       req->r_args.getattr.mask = cpu_to_le32(mask);
> >
> >       req->r_parent = dir;
> > +     ihold(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);
> > @@ -850,6 +851,7 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
> >       req->r_dentry = dget(dentry);
> >       req->r_num_caps = 2;
> >       req->r_parent = dir;
> > +     ihold(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);
> > @@ -907,6 +909,7 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
> >               goto out;
> >       }
> >       req->r_parent = dir;
> > +     ihold(dir);
> >       set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
> >       req->r_dentry = dget(dentry);
> >       req->r_num_caps = 2;
> > @@ -963,6 +966,7 @@ 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_parent = dir;
> > +     ihold(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 | CEPH_CAP_AUTH_EXCL;
> > @@ -1008,6 +1012,7 @@ static int ceph_link(struct dentry *old_dentry, struct inode *dir,
> >       req->r_num_caps = 2;
> >       req->r_old_dentry = dget(old_dentry);
> >       req->r_parent = dir;
> > +     ihold(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;
> > @@ -1055,6 +1060,7 @@ static int ceph_unlink(struct inode *dir, struct dentry *dentry)
> >       req->r_dentry = dget(dentry);
> >       req->r_num_caps = 2;
> >       req->r_parent = dir;
> > +     ihold(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;
> > @@ -1104,6 +1110,7 @@ static int ceph_rename(struct inode *old_dir, struct dentry *old_dentry,
> >       req->r_old_dentry = dget(old_dentry);
> >       req->r_old_dentry_dir = old_dir;
> >       req->r_parent = new_dir;
> > +     ihold(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;
> > @@ -1586,6 +1593,7 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
> >                       req->r_dentry = dget(dentry);
> >                       req->r_num_caps = 2;
> >                       req->r_parent = dir;
> > +                     ihold(dir);
> >
> >                       mask = CEPH_STAT_CAP_INODE | CEPH_CAP_AUTH_SHARED;
> >                       if (ceph_security_xattr_wanted(dir))
> > diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> > index d3ef7ee429ec..e7804720549d 100644
> > --- a/fs/ceph/export.c
> > +++ b/fs/ceph/export.c
> > @@ -519,6 +519,7 @@ static int ceph_get_name(struct dentry *parent, char *name,
> >       ihold(inode);
> >       req->r_ino2 = ceph_vino(d_inode(parent));
> >       req->r_parent = d_inode(parent);
> > +     ihold(req->r_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 0e505a5e09fe..f24d18f46715 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -478,6 +478,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> >         req->r_args.open.mask = cpu_to_le32(mask);
> >
> >       req->r_parent = dir;
> > +     ihold(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,
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 9d1ac87e5897..8c05cfe57adf 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -696,8 +696,10 @@ 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_parent)
> > +     if (req->r_parent) {
> >               ceph_put_cap_refs(ceph_inode(req->r_parent), CEPH_CAP_PIN);
> > +             iput(req->r_parent);
> > +     }
> >       iput(req->r_target_inode);
> >       if (req->r_dentry)
> >               dput(req->r_dentry);


Yes, I think so. I'll make that change.

Thanks,
diff mbox series

Patch

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index a8f429882249..3eb9bc226b77 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -783,6 +783,7 @@  static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 	req->r_args.getattr.mask = cpu_to_le32(mask);
 
 	req->r_parent = dir;
+	ihold(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);
@@ -850,6 +851,7 @@  static int ceph_mknod(struct inode *dir, struct dentry *dentry,
 	req->r_dentry = dget(dentry);
 	req->r_num_caps = 2;
 	req->r_parent = dir;
+	ihold(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);
@@ -907,6 +909,7 @@  static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 		goto out;
 	}
 	req->r_parent = dir;
+	ihold(dir);
 	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
 	req->r_dentry = dget(dentry);
 	req->r_num_caps = 2;
@@ -963,6 +966,7 @@  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_parent = dir;
+	ihold(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 | CEPH_CAP_AUTH_EXCL;
@@ -1008,6 +1012,7 @@  static int ceph_link(struct dentry *old_dentry, struct inode *dir,
 	req->r_num_caps = 2;
 	req->r_old_dentry = dget(old_dentry);
 	req->r_parent = dir;
+	ihold(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;
@@ -1055,6 +1060,7 @@  static int ceph_unlink(struct inode *dir, struct dentry *dentry)
 	req->r_dentry = dget(dentry);
 	req->r_num_caps = 2;
 	req->r_parent = dir;
+	ihold(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;
@@ -1104,6 +1110,7 @@  static int ceph_rename(struct inode *old_dir, struct dentry *old_dentry,
 	req->r_old_dentry = dget(old_dentry);
 	req->r_old_dentry_dir = old_dir;
 	req->r_parent = new_dir;
+	ihold(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;
@@ -1586,6 +1593,7 @@  static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
 			req->r_dentry = dget(dentry);
 			req->r_num_caps = 2;
 			req->r_parent = dir;
+			ihold(dir);
 
 			mask = CEPH_STAT_CAP_INODE | CEPH_CAP_AUTH_SHARED;
 			if (ceph_security_xattr_wanted(dir))
diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index d3ef7ee429ec..e7804720549d 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -519,6 +519,7 @@  static int ceph_get_name(struct dentry *parent, char *name,
 	ihold(inode);
 	req->r_ino2 = ceph_vino(d_inode(parent));
 	req->r_parent = d_inode(parent);
+	ihold(req->r_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 0e505a5e09fe..f24d18f46715 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -478,6 +478,7 @@  int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
        req->r_args.open.mask = cpu_to_le32(mask);
 
 	req->r_parent = dir;
+	ihold(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,
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 9d1ac87e5897..8c05cfe57adf 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -696,8 +696,10 @@  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_parent)
+	if (req->r_parent) {
 		ceph_put_cap_refs(ceph_inode(req->r_parent), CEPH_CAP_PIN);
+		iput(req->r_parent);
+	}
 	iput(req->r_target_inode);
 	if (req->r_dentry)
 		dput(req->r_dentry);