Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1344512 > unrolled thread

[ceph] what's going on with d_rehash() in splice_dentry()?

Back to article view | Back to linux.kernel

Contents

  [ceph] what's going on with d_rehash() in splice_dentry()? Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-26 18:50 +0100
    Re: [ceph] what's going on with d_rehash() in splice_dentry()? Sage Weil <sweil@redhat.com> - 2016-03-01 16:00 +0100
      Re: [ceph] what's going on with d_rehash() in splice_dentry()? "Yan, Zheng" <zyan@redhat.com> - 2016-03-02 04:10 +0100

#1344512 — [ceph] what's going on with d_rehash() in splice_dentry()?

You have, modulo printks and BUG_ON(),
{
        struct dentry *realdn;
        /* dn must be unhashed */
        if (!d_unhashed(dn))
                d_drop(dn);
        realdn = d_splice_alias(in, dn);
        if (IS_ERR(realdn)) {
                if (prehash)
                        *prehash = false; /* don't rehash on error */
                dn = realdn; /* note realdn contains the error */
                goto out;
        } else if (realdn) {
                dput(dn);
                dn = realdn;
        }
        if ((!prehash || *prehash) && d_unhashed(dn))
                d_rehash(dn);

When d_splice_alias() returns NULL it has hashed the dentry you'd given it;
when it returns a different dentry, that dentry is also returned hashed.
IOW, d_rehash(dn) in there should never be called.

If you have a case when it _is_ called, you've found a bug somewhere and
I'd like to see details.  AFAICS, the whole prehash thing appears to be
pointless - even the place where we modify *prehash, since in that case
we return ERR_PTR() and the only caller passing non-NULL prehash (&have_lease)
buggers off on such return value past all code that would look at have_lease
value.

One possible reading is that you want to prevent hashing in !have_lease
case of
                        dn = splice_dentry(dn, in, &have_lease);
If that's the case, you might have a problem, since it will be hashed no
matter what...

PS: the proof that d_splice_alias() always hashes is simple - if you exclude
the places where it returns ERR_PTR(), you are left with
        d_rehash(dentry);
        return NULL;
in the very end,
                                __d_move(new, dentry, false);
				...
			return new;
and
                                int err = __d_unalias(inode, dentry, new);
				...
				// err turned out to be zero
			return new;
The first one is obvious - we return NULL after an explicit d_rehash() of
the argument.  __d_move() is guaranteed to return with its first argument
hashed due to
        __d_drop(dentry);
        __d_rehash(dentry, d_hash(target->d_parent, target->d_name.hash));
(dentry here refers to the first argument of __d_move() - it's our 'new').
And zero-returning __d_unalias() ends up calling __d_move(), with the
third argument of __d_unalias() ending up as the first one of __d_move().
So in both remaining cases we return a dentry that has just been hashed.

[toc] | [next] | [standalone]

#1346678

Hi Al,

On Fri, 26 Feb 2016, Al Viro wrote:
> You have, modulo printks and BUG_ON(),
> {
>         struct dentry *realdn;
>         /* dn must be unhashed */
>         if (!d_unhashed(dn))
>                 d_drop(dn);
>         realdn = d_splice_alias(in, dn);
>         if (IS_ERR(realdn)) {
>                 if (prehash)
>                         *prehash = false; /* don't rehash on error */
>                 dn = realdn; /* note realdn contains the error */
>                 goto out;
>         } else if (realdn) {
>                 dput(dn);
>                 dn = realdn;
>         }
>         if ((!prehash || *prehash) && d_unhashed(dn))
>                 d_rehash(dn);
> 
> When d_splice_alias() returns NULL it has hashed the dentry you'd given it;
> when it returns a different dentry, that dentry is also returned hashed.
> IOW, d_rehash(dn) in there should never be called.
> 
> If you have a case when it _is_ called, you've found a bug somewhere and
> I'd like to see details.  AFAICS, the whole prehash thing appears to be
> pointless - even the place where we modify *prehash, since in that case
> we return ERR_PTR() and the only caller passing non-NULL prehash (&have_lease)
> buggers off on such return value past all code that would look at have_lease
> value.

Right.
 
> One possible reading is that you want to prevent hashing in !have_lease
> case of
>                         dn = splice_dentry(dn, in, &have_lease);
> If that's the case, you might have a problem, since it will be hashed no
> matter what...

In this case it doesn't actually matter if it is hashed or not, since 
we will look at the lease state on the dentry before trusting it...

This code dates back to when Ceph was originally upstreamed, so the 
history is murky, but I expect at that point I wanted to avoid hashing in 
the no-lease case.  But I don't think it matters.  We should just remove 
the prehash argument from splice_dentry entirely.

Zheng, does that sound right?

Thanks!
sage

[toc] | [prev] | [next] | [standalone]

#1347751

> On Mar 1, 2016, at 22:50, Sage Weil <sweil@redhat.com> wrote:
> 
> Hi Al,
> 
> On Fri, 26 Feb 2016, Al Viro wrote:
>> You have, modulo printks and BUG_ON(),
>> {
>>        struct dentry *realdn;
>>        /* dn must be unhashed */
>>        if (!d_unhashed(dn))
>>                d_drop(dn);
>>        realdn = d_splice_alias(in, dn);
>>        if (IS_ERR(realdn)) {
>>                if (prehash)
>>                        *prehash = false; /* don't rehash on error */
>>                dn = realdn; /* note realdn contains the error */
>>                goto out;
>>        } else if (realdn) {
>>                dput(dn);
>>                dn = realdn;
>>        }
>>        if ((!prehash || *prehash) && d_unhashed(dn))
>>                d_rehash(dn);
>> 
>> When d_splice_alias() returns NULL it has hashed the dentry you'd given it;
>> when it returns a different dentry, that dentry is also returned hashed.
>> IOW, d_rehash(dn) in there should never be called.
>> 
>> If you have a case when it _is_ called, you've found a bug somewhere and
>> I'd like to see details.  AFAICS, the whole prehash thing appears to be
>> pointless - even the place where we modify *prehash, since in that case
>> we return ERR_PTR() and the only caller passing non-NULL prehash (&have_lease)
>> buggers off on such return value past all code that would look at have_lease
>> value.
> 
> Right.
> 
>> One possible reading is that you want to prevent hashing in !have_lease
>> case of
>>                        dn = splice_dentry(dn, in, &have_lease);
>> If that's the case, you might have a problem, since it will be hashed no
>> matter what...
> 
> In this case it doesn't actually matter if it is hashed or not, since 
> we will look at the lease state on the dentry before trusting it...
> 
> This code dates back to when Ceph was originally upstreamed, so the 
> history is murky, but I expect at that point I wanted to avoid hashing in 
> the no-lease case.  But I don't think it matters.  We should just remove 
> the prehash argument from splice_dentry entirely.
> 
> Zheng, does that sound right?

Yes. I think we can remove the d_rehash(dn) call and rehash parameter.

Regards
Yan, Zheng 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web