On Wed, Dec 02, 2009 at 05:11:27PM -0500, J. Bruce Fields wrote:
> On Wed, Sep 09, 2009 at 04:32:54PM +1000, NeilBrown wrote:
> > @@ -793,9 +793,18 @@ exp_find_key(svc_client *clp, int fsid_type, u32 *fsidv, struct cache_req *reqp)
> > memcpy(key.ek_fsid, fsidv, key_len(fsid_type));
> >
> > ek = svc_expkey_lookup(&key);
> > + again:
> > if (ek == NULL)
> > return ERR_PTR(-ENOMEM);
> > err = cache_check(&svc_expkey_cache, &ek->h, reqp);
> > + if (err == -ETIMEDOUT) {
> > + struct svc_expkey *prev_ek = ek;
> > + ek = svc_expkey_lookup(&key);
> > + if (ek != prev_ek)
> > + goto again;
> > + if (ek)
> > + cache_put(&ek->h, &svc_expkey_cache);
> > + }
>
> This is very subtle. (We're comparing to a pointer which may not
> actually point to anything any more.) And it's repeated in every
> caller. Is there any simpler way to handle this?
Actually, is it even right? After the cache_check:
> > err = cache_check(&svc_expkey_cache, &ek->h, reqp);
we no longer hold a reference on ek, so it could be freed. There's no
reason that address couldn't then be reused for something else. It's
even possible that a new entry could be created at the same address
here. So:
> > + if (err == -ETIMEDOUT) {
> > + struct svc_expkey *prev_ek = ek;
> > + ek = svc_expkey_lookup(&key);
the "ek" that's returned might well equal prev_ek,
> > + if (ek != prev_ek)
> > + goto again;
but that doesn't necessarily imply that this is the same object that
used to exist at that address. So we could still return an ek which
isn't actually a positive cache entry.
Am I missing something?
--b.
> > + if (ek)
> > + cache_put(&ek->h, &svc_expkey_cache);
> > + }
>
> --b.
>
> > if (err)
> > return ERR_PTR(err);
> > return ek;
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
