[SSSD] [PATCH] Don't go to the backend for identical cache entry requests

Simo Sorce ssorce at redhat.com
Thu Aug 13 16:31:55 UTC 2009


On Thu, 2009-08-13 at 12:18 -0400, Stephen Gallagher wrote:
> Currently, if an additional request comes in for a cache entry
> while that same entry is already in the process of being refreshed,
> we start a duplicate cache update request. This patch adds allows
> the cache to maintain a hash table of all in-progress requests and
> queue up multiple callbacks for updates in progress. Once the data
> is returned, all of these callbacks will fire.

Concept is fine.

> Because requests can carry multiple callbacks, we cannot rely on
> the life of the current caller to manage the memory for the
> request. We'll manage the memory ourselves internally and remove
> the mem_ctx argument from nss_dp_send_acct_req() so there's no
> ambiguity.

Nack this, we still need to pass the original request context and append
the nss_dp_callback structure off of it, otherwise if the original
request is terminated while waiting for the reply you will later access
freed memory when you run through the list of callbacks.
Allocate nss_dp_callback on the orginal request and attach a destructor
that will remove it from the ndp_req list of callbacks (requires that
you keep a pointer to ndp_req in the nss_dp_callback structure).

Also sss_dp_callback_ctx is redundant as is the refcount variable.
You should just pass ndp_req to sss_dp_invoke_callback().
When the function is called run the first callback in list, then free it
(the destructor will assure it is removed from the list), if there are
still callback in the list when it returns then reschedule a new
sss_dp_invoke_callback() again, otherwise just talloc_free(ndp_req) and
return.

Also DLIST_ADD() adds on the head of the list, use DLIST_ADD_AFTER() for
the callbacks so that they are called in the order the requests have
been issued. It may make the difference between a timeout and a
successful reply when you are tight on time and have many callbacks to
go through (and it is more fair to serve in order).

Other errors:
in an error path you talloc_free(tmp_ctx); the goto done; where you
talloc_free(tmp_ctx); again ... this is a double free.

Other style issues:
- please standardize on either sss_ or nss_ prefixes, given this is
common code you should probably standardize on sss_ and change all
function names you touch.
- please use "if (" not "if("
- prefer talloc_zfree() to talloc_free() its safer


> This patch also introduces the necessary framework to implement the
> out-of-band cache updates (which will follow in a subsequent patch)

Haven't seen this in the patch.

Simo.




More information about the sssd-devel mailing list