[PATCH] ceph: trivial comment fix

[PATCH] ceph: trivial comment fix

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

"disconnected" is too easily confused with "DCACHE_DISCONNECTED".  I
think "unhashed" is the more precise term here.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/ceph/caps.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Just ran across this while wondering what d_find_alias callers do....

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 3c0a4bd..697f9d7 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -2350,11 +2350,11 @@ static void invalidate_aliases(struct inode *inode)
 	d_prune_aliases(inode);
 	/*
 	 * For non-directory inode, d_find_alias() only returns
-	 * connected dentry. After calling d_invalidate(), the
-	 * dentry become disconnected.
+	 * hashed dentry. After calling d_invalidate(), the
+	 * dentry becomes unhashed.
 	 *
 	 * For directory inode, d_find_alias() can return
-	 * disconnected dentry. But directory inode should have
+	 * unhashed dentry. But directory inode should have
 	 * one alias at most.
 	 */
 	while ((dn = d_find_alias(inode))) {
-- 
1.7.9.5

Re: [PATCH] ceph: trivial comment fix

[Sage Weil]

On Thu, 16 Jan 2014, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> "disconnected" is too easily confused with "DCACHE_DISCONNECTED".  I
> think "unhashed" is the more precise term here.

Good point.  Applied, thanks!
sage


> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/ceph/caps.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Just ran across this while wondering what d_find_alias callers do....
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 3c0a4bd..697f9d7 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -2350,11 +2350,11 @@ static void invalidate_aliases(struct inode *inode)
>  	d_prune_aliases(inode);
>  	/*
>  	 * For non-directory inode, d_find_alias() only returns
> -	 * connected dentry. After calling d_invalidate(), the
> -	 * dentry become disconnected.
> +	 * hashed dentry. After calling d_invalidate(), the
> +	 * dentry becomes unhashed.
>  	 *
>  	 * For directory inode, d_find_alias() can return
> -	 * disconnected dentry. But directory inode should have
> +	 * unhashed dentry. But directory inode should have
>  	 * one alias at most.
>  	 */
>  	while ((dn = d_find_alias(inode))) {
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH] ceph: trivial comment fix

[J. Bruce Fields]

On Thu, Jan 16, 2014 at 04:03:38PM -0800, Sage Weil wrote:
> On Thu, 16 Jan 2014, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > "disconnected" is too easily confused with "DCACHE_DISCONNECTED".  I
> > think "unhashed" is the more precise term here.
> 
> Good point.  Applied, thanks!

Thanks!  While I'm looking, there's another d_find_alias() caller in
build_inode_path.  What's that?  (Do the mds protocol messages actually
use full paths?)  And will this break if you get a DCACHE_DISCONNECTED
or an unhashed alias?

--b.

> sage
> 
> 
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/ceph/caps.c |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > Just ran across this while wondering what d_find_alias callers do....
> > 
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 3c0a4bd..697f9d7 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -2350,11 +2350,11 @@ static void invalidate_aliases(struct inode *inode)
> >  	d_prune_aliases(inode);
> >  	/*
> >  	 * For non-directory inode, d_find_alias() only returns
> > -	 * connected dentry. After calling d_invalidate(), the
> > -	 * dentry become disconnected.
> > +	 * hashed dentry. After calling d_invalidate(), the
> > +	 * dentry becomes unhashed.
> >  	 *
> >  	 * For directory inode, d_find_alias() can return
> > -	 * disconnected dentry. But directory inode should have
> > +	 * unhashed dentry. But directory inode should have
> >  	 * one alias at most.
> >  	 */
> >  	while ((dn = d_find_alias(inode))) {
> > -- 
> > 1.7.9.5
> > 
> > 

Re: [PATCH] ceph: trivial comment fix

[Sage Weil]

On Thu, 16 Jan 2014, J. Bruce Fields wrote:
> On Thu, Jan 16, 2014 at 04:03:38PM -0800, Sage Weil wrote:
> > On Thu, 16 Jan 2014, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > 
> > > "disconnected" is too easily confused with "DCACHE_DISCONNECTED".  I
> > > think "unhashed" is the more precise term here.
> > 
> > Good point.  Applied, thanks!
> 
> Thanks!  While I'm looking, there's another d_find_alias() caller in
> build_inode_path.  What's that?  (Do the mds protocol messages actually
> use full paths?)  And will this break if you get a DCACHE_DISCONNECTED
> or an unhashed alias?

They build a path relative to the first non-snapped parent, which in most 
cases is either no path at all (just an ino) or a single path segment.  
The exception is if you are inside a snapshotted directory (e.g. 
a/b/.snap/mysnap/c/d), in which case it can be deeper.  In any case, I 
think the only problem is if you have a disconnected dentry from an old 
nfs filename for an inode within a snapshot.  There may be some issues 
there (nfs reexport + snaps isn't currently part of the test suite), but 
in general I don't think there is any issue with disconnected dentries.

sage


> 
> --b.
> 
> > sage
> > 
> > 
> > > 
> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > ---
> > >  fs/ceph/caps.c |    6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > Just ran across this while wondering what d_find_alias callers do....
> > > 
> > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > index 3c0a4bd..697f9d7 100644
> > > --- a/fs/ceph/caps.c
> > > +++ b/fs/ceph/caps.c
> > > @@ -2350,11 +2350,11 @@ static void invalidate_aliases(struct inode *inode)
> > >  	d_prune_aliases(inode);
> > >  	/*
> > >  	 * For non-directory inode, d_find_alias() only returns
> > > -	 * connected dentry. After calling d_invalidate(), the
> > > -	 * dentry become disconnected.
> > > +	 * hashed dentry. After calling d_invalidate(), the
> > > +	 * dentry becomes unhashed.
> > >  	 *
> > >  	 * For directory inode, d_find_alias() can return
> > > -	 * disconnected dentry. But directory inode should have
> > > +	 * unhashed dentry. But directory inode should have
> > >  	 * one alias at most.
> > >  	 */
> > >  	while ((dn = d_find_alias(inode))) {
> > > -- 
> > > 1.7.9.5
> > > 
> > > 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] ceph: trivial comment fix

[J. Bruce Fields]

On Fri, Jan 17, 2014 at 08:33:48PM -0800, Sage Weil wrote:
> On Thu, 16 Jan 2014, J. Bruce Fields wrote:
> > On Thu, Jan 16, 2014 at 04:03:38PM -0800, Sage Weil wrote:
> > > On Thu, 16 Jan 2014, J. Bruce Fields wrote:
> > > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > > 
> > > > "disconnected" is too easily confused with "DCACHE_DISCONNECTED".  I
> > > > think "unhashed" is the more precise term here.
> > > 
> > > Good point.  Applied, thanks!
> > 
> > Thanks!  While I'm looking, there's another d_find_alias() caller in
> > build_inode_path.  What's that?  (Do the mds protocol messages actually
> > use full paths?)  And will this break if you get a DCACHE_DISCONNECTED
> > or an unhashed alias?
> 
> They build a path relative to the first non-snapped parent, which in most 
> cases is either no path at all (just an ino) or a single path segment.  
> The exception is if you are inside a snapshotted directory (e.g. 
> a/b/.snap/mysnap/c/d), in which case it can be deeper.  In any case, I 
> think the only problem is if you have a disconnected dentry from an old 
> nfs filename for an inode within a snapshot.

Right, so I guess there may be a problem you get a disconnected dentry
that's not yet connected all the way back up to the main dcache, in
which case the !IS_ROOT tests in the loops in ceph_mdsc_build_path() may
fail too soon.

--b.

> There may be some issues 
> there (nfs reexport + snaps isn't currently part of the test suite), but 
> in general I don't think there is any issue with disconnected dentries.
> 
> sage
> 
> 
> > 
> > --b.
> > 
> > > sage
> > > 
> > > 
> > > > 
> > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > > > ---
> > > >  fs/ceph/caps.c |    6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > Just ran across this while wondering what d_find_alias callers do....
> > > > 
> > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > > index 3c0a4bd..697f9d7 100644
> > > > --- a/fs/ceph/caps.c
> > > > +++ b/fs/ceph/caps.c
> > > > @@ -2350,11 +2350,11 @@ static void invalidate_aliases(struct inode *inode)
> > > >  	d_prune_aliases(inode);
> > > >  	/*
> > > >  	 * For non-directory inode, d_find_alias() only returns
> > > > -	 * connected dentry. After calling d_invalidate(), the
> > > > -	 * dentry become disconnected.
> > > > +	 * hashed dentry. After calling d_invalidate(), the
> > > > +	 * dentry becomes unhashed.
> > > >  	 *
> > > >  	 * For directory inode, d_find_alias() can return
> > > > -	 * disconnected dentry. But directory inode should have
> > > > +	 * unhashed dentry. But directory inode should have
> > > >  	 * one alias at most.
> > > >  	 */
> > > >  	while ((dn = d_find_alias(inode))) {
> > > > -- 
> > > > 1.7.9.5
> > > > 
> > > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> >