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

Stephen Gallagher sgallagh at redhat.com
Thu Aug 13 19:47:20 UTC 2009


On 08/13/2009 12:31 PM, Simo Sorce wrote:
> 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.
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel


Simo's concerns above have been addressed in the attached patches.

Patch 0001: Don't go to the backend for identical cache entry requests

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.

Patch 0002: Refactor responder_dp.c

Many of the functions in responder_dp.c were originally NSS-
specific and were moved there from the NSS responder code. Since
they are now generic to any responder, rename them to sss_dp_*

The second patch is a global find-and-replace and has no functional effect.


-- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Don-t-go-to-the-backend-for-identical-cache-entry-re.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090813/5636a85b/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-Refactor-responder_dp.c.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090813/5636a85b/attachment-0001.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 260 bytes
Desc: OpenPGP digital signature
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090813/5636a85b/attachment.sig>


More information about the sssd-devel mailing list