| Message ID | 20190509140147.20755-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix ceph_mdsc_build_path to not stop on first component | expand |
On 5/9/19 10:01 PM, Jeff Layton wrote: > When ceph_mdsc_build_path is handed a positive dentry, it will return a > zero-length path string with the base set to that dentry. This is not > what we want. Always include at least one path component in the string. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/mds_client.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > I found this while testing my asynchronous unlink patches. We have > to do the unlink in this case without the parent being locked, and > that showed that we got a bogus path built in this case. > > This seems to work correctly, but it's a little unclear whether the > existing behavior is desirable in some cases. Is this the right thing > to do here? > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 66eae336a68a..39f5bd2eafda 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -2114,9 +2114,10 @@ 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 && > + } else if (stop_on_nosnap && inode && dentry != temp && > ceph_snap(inode) == CEPH_NOSNAP) { > spin_unlock(&temp->d_lock); > + pos++; /* get rid of any prepended '/' */ > break; > } else { > pos -= temp->d_name.len; > Reviewed-by: "Yan, Zheng" <zyan@redhat.com>
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 66eae336a68a..39f5bd2eafda 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2114,9 +2114,10 @@ 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 && + } else if (stop_on_nosnap && inode && dentry != temp && ceph_snap(inode) == CEPH_NOSNAP) { spin_unlock(&temp->d_lock); + pos++; /* get rid of any prepended '/' */ break; } else { pos -= temp->d_name.len;
When ceph_mdsc_build_path is handed a positive dentry, it will return a zero-length path string with the base set to that dentry. This is not what we want. Always include at least one path component in the string. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/mds_client.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) I found this while testing my asynchronous unlink patches. We have to do the unlink in this case without the parent being locked, and that showed that we got a bogus path built in this case. This seems to work correctly, but it's a little unclear whether the existing behavior is desirable in some cases. Is this the right thing to do here?