diff mbox series

[6/8] ceph: use READ_ONCE to access d_parent in RCU critical section

Message ID 20190523081345.20410-6-zyan@redhat.com (mailing list archive)
State New, archived
Headers show
Series [1/8] ceph: fix error handling in ceph_get_caps() | expand

Commit Message

Yan, Zheng May 23, 2019, 8:13 a.m. UTC 
Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
---
 fs/ceph/mds_client.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jeff Layton May 28, 2019, 1:55 p.m. UTC | #1 
On Thu, 2019-05-23 at 16:13 +0800, Yan, Zheng wrote:
> Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> ---
>  fs/ceph/mds_client.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 60e8ddbdfdc5..870754e9d572 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -913,7 +913,7 @@ static int __choose_mds(struct ceph_mds_client *mdsc,
>  		struct inode *dir;
>  
>  		rcu_read_lock();
> -		parent = req->r_dentry->d_parent;
> +		parent = READ_ONCE(req->r_dentry->d_parent);

Can we use rcu_dereference() instead?

>  		dir = req->r_parent ? : d_inode_rcu(parent);
>  
>  		if (!dir || dir->i_sb != mdsc->fsc->sb) {
> @@ -2131,8 +2131,8 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
>  		if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
>  			dout("build_path path+%d: %p SNAPDIR\n",
>  			     pos, temp);
> -		} else if (stop_on_nosnap && inode && dentry != temp &&
> -			   ceph_snap(inode) == CEPH_NOSNAP) {
> +		} else if (stop_on_nosnap && dentry != temp &&
> +			   inode && ceph_snap(inode) == CEPH_NOSNAP) {

^^^ Unrelated delta?

>  			spin_unlock(&temp->d_lock);
>  			pos++; /* get rid of any prepended '/' */
>  			break;
> @@ -2145,7 +2145,7 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
>  			memcpy(path + pos, temp->d_name.name, temp->d_name.len);
>  		}
>  		spin_unlock(&temp->d_lock);
> -		temp = temp->d_parent;
> +		temp = READ_ONCE(temp->d_parent);

Better to use rcu_dereference() here.

>  
>  		/* Are we at the root? */
>  		if (IS_ROOT(temp))
Yan, Zheng May 29, 2019, 2:17 a.m. UTC | #2 
On Tue, May 28, 2019 at 9:57 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Thu, 2019-05-23 at 16:13 +0800, Yan, Zheng wrote:
> > Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> > ---
> >  fs/ceph/mds_client.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 60e8ddbdfdc5..870754e9d572 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -913,7 +913,7 @@ static int __choose_mds(struct ceph_mds_client *mdsc,
> >               struct inode *dir;
> >
> >               rcu_read_lock();
> > -             parent = req->r_dentry->d_parent;
> > +             parent = READ_ONCE(req->r_dentry->d_parent);
>
> Can we use rcu_dereference() instead?
>

d_parent has no __rcu mark. It will cause sparse warning.

Regards
Yan, Zheng

> >               dir = req->r_parent ? : d_inode_rcu(parent);
> >
> >               if (!dir || dir->i_sb != mdsc->fsc->sb) {
> > @@ -2131,8 +2131,8 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> >               if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
> >                       dout("build_path path+%d: %p SNAPDIR\n",
> >                            pos, temp);
> > -             } else if (stop_on_nosnap && inode && dentry != temp &&
> > -                        ceph_snap(inode) == CEPH_NOSNAP) {
> > +             } else if (stop_on_nosnap && dentry != temp &&
> > +                        inode && ceph_snap(inode) == CEPH_NOSNAP) {
>
> ^^^ Unrelated delta?
>
> >                       spin_unlock(&temp->d_lock);
> >                       pos++; /* get rid of any prepended '/' */
> >                       break;
> > @@ -2145,7 +2145,7 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> >                       memcpy(path + pos, temp->d_name.name, temp->d_name.len);
> >               }
> >               spin_unlock(&temp->d_lock);
> > -             temp = temp->d_parent;
> > +             temp = READ_ONCE(temp->d_parent);
>
> Better to use rcu_dereference() here.
>
> >
> >               /* Are we at the root? */
> >               if (IS_ROOT(temp))
>
> --
> Jeff Layton <jlayton@redhat.com>
>
Jeff Layton May 29, 2019, 10:10 a.m. UTC | #3 
On Wed, 2019-05-29 at 10:17 +0800, Yan, Zheng wrote:
> On Tue, May 28, 2019 at 9:57 PM Jeff Layton <jlayton@redhat.com> wrote:
> > On Thu, 2019-05-23 at 16:13 +0800, Yan, Zheng wrote:
> > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
> > > ---
> > >  fs/ceph/mds_client.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > index 60e8ddbdfdc5..870754e9d572 100644
> > > --- a/fs/ceph/mds_client.c
> > > +++ b/fs/ceph/mds_client.c
> > > @@ -913,7 +913,7 @@ static int __choose_mds(struct ceph_mds_client *mdsc,
> > >               struct inode *dir;
> > > 
> > >               rcu_read_lock();
> > > -             parent = req->r_dentry->d_parent;
> > > +             parent = READ_ONCE(req->r_dentry->d_parent);
> > 
> > Can we use rcu_dereference() instead?
> > 
> 
> d_parent has no __rcu mark. It will cause sparse warning.
> 
> Regards
> Yan, Zheng
> 

Good point.

> > >               dir = req->r_parent ? : d_inode_rcu(parent);
> > > 
> > >               if (!dir || dir->i_sb != mdsc->fsc->sb) {
> > > @@ -2131,8 +2131,8 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> > >               if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
> > >                       dout("build_path path+%d: %p SNAPDIR\n",
> > >                            pos, temp);
> > > -             } else if (stop_on_nosnap && inode && dentry != temp &&
> > > -                        ceph_snap(inode) == CEPH_NOSNAP) {
> > > +             } else if (stop_on_nosnap && dentry != temp &&
> > > +                        inode && ceph_snap(inode) == CEPH_NOSNAP) {
> > 
> > ^^^ Unrelated delta?
> > 
> > >                       spin_unlock(&temp->d_lock);
> > >                       pos++; /* get rid of any prepended '/' */
> > >                       break;
> > > @@ -2145,7 +2145,7 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> > >                       memcpy(path + pos, temp->d_name.name, temp->d_name.len);
> > >               }
> > >               spin_unlock(&temp->d_lock);
> > > -             temp = temp->d_parent;
> > > +             temp = READ_ONCE(temp->d_parent);
> > 
> > Better to use rcu_dereference() here.
> > 
> > >               /* Are we at the root? */
> > >               if (IS_ROOT(temp))
> > 

Reviewed-by: Jeff Layton <jlayton@redhat.com>
diff mbox series

Patch

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 60e8ddbdfdc5..870754e9d572 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -913,7 +913,7 @@  static int __choose_mds(struct ceph_mds_client *mdsc,
 		struct inode *dir;
 
 		rcu_read_lock();
-		parent = req->r_dentry->d_parent;
+		parent = READ_ONCE(req->r_dentry->d_parent);
 		dir = req->r_parent ? : d_inode_rcu(parent);
 
 		if (!dir || dir->i_sb != mdsc->fsc->sb) {
@@ -2131,8 +2131,8 @@  char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
 		if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
 			dout("build_path path+%d: %p SNAPDIR\n",
 			     pos, temp);
-		} else if (stop_on_nosnap && inode && dentry != temp &&
-			   ceph_snap(inode) == CEPH_NOSNAP) {
+		} else if (stop_on_nosnap && dentry != temp &&
+			   inode && ceph_snap(inode) == CEPH_NOSNAP) {
 			spin_unlock(&temp->d_lock);
 			pos++; /* get rid of any prepended '/' */
 			break;
@@ -2145,7 +2145,7 @@  char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
 			memcpy(path + pos, temp->d_name.name, temp->d_name.len);
 		}
 		spin_unlock(&temp->d_lock);
-		temp = temp->d_parent;
+		temp = READ_ONCE(temp->d_parent);
 
 		/* Are we at the root? */
 		if (IS_ROOT(temp))