From: David Howells <[email protected]>
Make __d_materialise_dentry() set the materialised dentry name correctly by
flipping the arguments to switch_names().
switch_names() is lazy: if both names are internal to their dentries, it'll
overwrite that of the first dentry with that of the second, and won't update
that of the second.
In the case of __d_materialise_dentry(), the second is an already extant
anonymous dentry that we want to insert into the tree in place of the dentry we
just looked up[*]. However, the dentry we just looked up carries the name we
actually want to use.
[*] This is used by NFS to join a mount of a subtree into a mount of a tree
nearer the root when the two meet, where both mounts share a superblock and
thus a set of dentries.
Signed-off-by: David Howells <[email protected]>
---
fs/dcache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 265e0ce..ff779d4 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2698,7 +2698,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
dparent = dentry->d_parent;
- switch_names(dentry, anon);
+ switch_names(anon, dentry);
swap(dentry->d_name.hash, anon->d_name.hash);
dentry->d_parent = dentry;
--
1.8.5.3
On Fri, Mar 07, 2014 at 01:55:52PM -0500, J. Bruce Fields wrote:
> On Thu, Mar 06, 2014 at 09:35:01AM -0500, Trond Myklebust wrote:
> >
> > On Mar 6, 2014, at 9:16, Yan, Zheng <[email protected]> wrote:
> >
> > > From: David Howells <[email protected]>
> > >
> > > Make __d_materialise_dentry() set the materialised dentry name correctly by
> > > flipping the arguments to switch_names().
> > >
> > > switch_names() is lazy: if both names are internal to their dentries, it'll
> > > overwrite that of the first dentry with that of the second, and won't update
> > > that of the second.
> > >
> > > In the case of __d_materialise_dentry(), the second is an already extant
> > > anonymous dentry that we want to insert into the tree in place of the dentry we
> > > just looked up[*]. However, the dentry we just looked up carries the name we
> > > actually want to use.
> > >
> > > [*] This is used by NFS to join a mount of a subtree into a mount of a tree
> > > nearer the root when the two meet, where both mounts share a superblock and
> > > thus a set of dentries.
> > >
> > > Signed-off-by: David Howells <[email protected]>
> > > ---
> > > fs/dcache.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > index 265e0ce..ff779d4 100644
> > > --- a/fs/dcache.c
> > > +++ b/fs/dcache.c
> > > @@ -2698,7 +2698,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
> > >
> > > dparent = dentry->d_parent;
> > >
> > > - switch_names(dentry, anon);
> > > + switch_names(anon, dentry);
> > > swap(dentry->d_name.hash, anon->d_name.hash);
> > >
> > > dentry->d_parent = dentry;
> >
> > Well spotted...
> >
> > We ought to better document the fact that ’switch_names’ is asymmetrical. Perhaps change it to ‘update_target_name’, and then switch the argument names so that the ’target’ really is the thing that gets updated?
>
> Probably not worth it if Miklos's RENAME_EXCHANGE operation is making it
> symmetrical soon anyway?:
>
> http://mid.gmane.org/<[email protected]>
To clarify: I mean, the suggested cleanup may not be worth it. The
__d_materialise_dentry bugfix of course is.
(What I wonder about is the duplication between this and __d_move. Is
there any way to make d_materialise_unique a special case of d_move?)
--b.
On Thu, Mar 06, 2014 at 09:35:01AM -0500, Trond Myklebust wrote:
>
> On Mar 6, 2014, at 9:16, Yan, Zheng <[email protected]> wrote:
>
> > From: David Howells <[email protected]>
> >
> > Make __d_materialise_dentry() set the materialised dentry name correctly by
> > flipping the arguments to switch_names().
> >
> > switch_names() is lazy: if both names are internal to their dentries, it'll
> > overwrite that of the first dentry with that of the second, and won't update
> > that of the second.
> >
> > In the case of __d_materialise_dentry(), the second is an already extant
> > anonymous dentry that we want to insert into the tree in place of the dentry we
> > just looked up[*]. However, the dentry we just looked up carries the name we
> > actually want to use.
> >
> > [*] This is used by NFS to join a mount of a subtree into a mount of a tree
> > nearer the root when the two meet, where both mounts share a superblock and
> > thus a set of dentries.
> >
> > Signed-off-by: David Howells <[email protected]>
> > ---
> > fs/dcache.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 265e0ce..ff779d4 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2698,7 +2698,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
> >
> > dparent = dentry->d_parent;
> >
> > - switch_names(dentry, anon);
> > + switch_names(anon, dentry);
> > swap(dentry->d_name.hash, anon->d_name.hash);
> >
> > dentry->d_parent = dentry;
>
> Well spotted...
>
> We ought to better document the fact that ’switch_names’ is asymmetrical. Perhaps change it to ‘update_target_name’, and then switch the argument names so that the ’target’ really is the thing that gets updated?
Probably not worth it if Miklos's RENAME_EXCHANGE operation is making it
symmetrical soon anyway?:
http://mid.gmane.org/<[email protected]>
--b.
On Mar 6, 2014, at 9:16, Yan, Zheng <[email protected]> wrote:
> From: David Howells <[email protected]>
>
> Make __d_materialise_dentry() set the materialised dentry name correctly by
> flipping the arguments to switch_names().
>
> switch_names() is lazy: if both names are internal to their dentries, it'll
> overwrite that of the first dentry with that of the second, and won't update
> that of the second.
>
> In the case of __d_materialise_dentry(), the second is an already extant
> anonymous dentry that we want to insert into the tree in place of the dentry we
> just looked up[*]. However, the dentry we just looked up carries the name we
> actually want to use.
>
> [*] This is used by NFS to join a mount of a subtree into a mount of a tree
> nearer the root when the two meet, where both mounts share a superblock and
> thus a set of dentries.
>
> Signed-off-by: David Howells <[email protected]>
> ---
> fs/dcache.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 265e0ce..ff779d4 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2698,7 +2698,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
>
> dparent = dentry->d_parent;
>
> - switch_names(dentry, anon);
> + switch_names(anon, dentry);
> swap(dentry->d_name.hash, anon->d_name.hash);
>
> dentry->d_parent = dentry;
Well spotted...
We ought to better document the fact that ?switch_names? is asymmetrical. Perhaps change it to ?update_target_name?, and then switch the argument names so that the ?target? really is the thing that gets updated?
_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
Hi Al,
Could you include this in 3.15
Regards
Yan, Zheng
On 03/06/2014 10:35 PM, Trond Myklebust wrote:
>
> On Mar 6, 2014, at 9:16, Yan, Zheng <[email protected]> wrote:
>
>> From: David Howells <[email protected]>
>>
>> Make __d_materialise_dentry() set the materialised dentry name correctly by
>> flipping the arguments to switch_names().
>>
>> switch_names() is lazy: if both names are internal to their dentries, it'll
>> overwrite that of the first dentry with that of the second, and won't update
>> that of the second.
>>
>> In the case of __d_materialise_dentry(), the second is an already extant
>> anonymous dentry that we want to insert into the tree in place of the dentry we
>> just looked up[*]. However, the dentry we just looked up carries the name we
>> actually want to use.
>>
>> [*] This is used by NFS to join a mount of a subtree into a mount of a tree
>> nearer the root when the two meet, where both mounts share a superblock and
>> thus a set of dentries.
>>
>> Signed-off-by: David Howells <[email protected]>
>> ---
>> fs/dcache.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 265e0ce..ff779d4 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -2698,7 +2698,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
>>
>> dparent = dentry->d_parent;
>>
>> - switch_names(dentry, anon);
>> + switch_names(anon, dentry);
>> swap(dentry->d_name.hash, anon->d_name.hash);
>>
>> dentry->d_parent = dentry;
>
> Well spotted...
>
> We ought to better document the fact that ?switch_names? is asymmetrical. Perhaps change it to ?update_target_name?, and then switch the argument names so that the ?target? really is the thing that gets updated?
>
> _________________________________
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> [email protected]
>
Hi Al,
Can this go to Linus via your tree?
Thanks!
sage
On Mon, 31 Mar 2014, Yan, Zheng wrote:
> Hi Al,
>
> Could you include this in 3.15
>
> Regards
> Yan, Zheng
>
> On 03/06/2014 10:35 PM, Trond Myklebust wrote:
> >
> > On Mar 6, 2014, at 9:16, Yan, Zheng <[email protected]> wrote:
> >
> >> From: David Howells <[email protected]>
> >>
> >> Make __d_materialise_dentry() set the materialised dentry name correctly by
> >> flipping the arguments to switch_names().
> >>
> >> switch_names() is lazy: if both names are internal to their dentries, it'll
> >> overwrite that of the first dentry with that of the second, and won't update
> >> that of the second.
> >>
> >> In the case of __d_materialise_dentry(), the second is an already extant
> >> anonymous dentry that we want to insert into the tree in place of the dentry we
> >> just looked up[*]. However, the dentry we just looked up carries the name we
> >> actually want to use.
> >>
> >> [*] This is used by NFS to join a mount of a subtree into a mount of a tree
> >> nearer the root when the two meet, where both mounts share a superblock and
> >> thus a set of dentries.
> >>
> >> Signed-off-by: David Howells <[email protected]>
> >> ---
> >> fs/dcache.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/dcache.c b/fs/dcache.c
> >> index 265e0ce..ff779d4 100644
> >> --- a/fs/dcache.c
> >> +++ b/fs/dcache.c
> >> @@ -2698,7 +2698,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
> >>
> >> dparent = dentry->d_parent;
> >>
> >> - switch_names(dentry, anon);
> >> + switch_names(anon, dentry);
> >> swap(dentry->d_name.hash, anon->d_name.hash);
> >>
> >> dentry->d_parent = dentry;
> >
> > Well spotted...
> >
> > We ought to better document the fact that ?switch_names? is asymmetrical. Perhaps change it to ?update_target_name?, and then switch the argument names so that the ?target? really is the thing that gets updated?
> >
> > _________________________________
> > Trond Myklebust
> > Linux NFS client maintainer, PrimaryData
> > [email protected]
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
On Mon, Apr 07, 2014 at 06:56:28AM -0700, Sage Weil wrote:
> Hi Al,
>
> Can this go to Linus via your tree?
I believe this has been fixed by
da1ce0670c14d8380e423a3239e562a1dc15fa9e "vfs: add cross-rename".
Though probably the patch should still go in, possibly with a changelog
update. We want it for stable kernels if nothing else.
--b.
>
> Thanks!
> sage
>
>
> On Mon, 31 Mar 2014, Yan, Zheng wrote:
>
> > Hi Al,
> >
> > Could you include this in 3.15
> >
> > Regards
> > Yan, Zheng
> >
> > On 03/06/2014 10:35 PM, Trond Myklebust wrote:
> > >
> > > On Mar 6, 2014, at 9:16, Yan, Zheng <[email protected]> wrote:
> > >
> > >> From: David Howells <[email protected]>
> > >>
> > >> Make __d_materialise_dentry() set the materialised dentry name correctly by
> > >> flipping the arguments to switch_names().
> > >>
> > >> switch_names() is lazy: if both names are internal to their dentries, it'll
> > >> overwrite that of the first dentry with that of the second, and won't update
> > >> that of the second.
> > >>
> > >> In the case of __d_materialise_dentry(), the second is an already extant
> > >> anonymous dentry that we want to insert into the tree in place of the dentry we
> > >> just looked up[*]. However, the dentry we just looked up carries the name we
> > >> actually want to use.
> > >>
> > >> [*] This is used by NFS to join a mount of a subtree into a mount of a tree
> > >> nearer the root when the two meet, where both mounts share a superblock and
> > >> thus a set of dentries.
> > >>
> > >> Signed-off-by: David Howells <[email protected]>
> > >> ---
> > >> fs/dcache.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/fs/dcache.c b/fs/dcache.c
> > >> index 265e0ce..ff779d4 100644
> > >> --- a/fs/dcache.c
> > >> +++ b/fs/dcache.c
> > >> @@ -2698,7 +2698,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
> > >>
> > >> dparent = dentry->d_parent;
> > >>
> > >> - switch_names(dentry, anon);
> > >> + switch_names(anon, dentry);
> > >> swap(dentry->d_name.hash, anon->d_name.hash);
> > >>
> > >> dentry->d_parent = dentry;
> > >
> > > Well spotted...
> > >
> > > We ought to better document the fact that ?switch_names? is asymmetrical. Perhaps change it to ?update_target_name?, and then switch the argument names so that the ?target? really is the thing that gets updated?
> > >
> > > _________________________________
> > > Trond Myklebust
> > > Linux NFS client maintainer, PrimaryData
> > > [email protected]
> > >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html