Re: NIS performance patches

[Thorsten Kukuk <kukuk at suse dot de>]

Hi,

On Wed, Jan 14, Jeff Bastian wrote:

> glibc developers,
> 
> Thorsten Kukuk has accepted the patches I posted last week and released 
> ypbind-mt-1.16 earlier today.  I haven't seen any discussion yet on the 
> glibc half of the patches.  Any thoughts?

As I wrote to you yesterday, I hadn't the time to look at it yet.

> >What exactly do our glibc patches do?
> > 1) Like test_bindings() above, we take the __yp_bind() function
> >    (glibc-2.3.2-200309260658/nis/ypclnt.c) and move chunks of it
> >    into three smaller functions.
> >      a) __yp_bind_client_create() is a small chunk of code that
> >         was duplicated in __yp_bind(), once for the section of
> >         code that looks at /var/yp/binding, the other for the
> >         section that talks to the ypbind daemon

This part is Ok, even if __yp_bind_client_create could be void,
since we always return 0.

> >      b) the section that looks at /var/yp/binding was moved into
> >         the __yp_bind_file() function, with a call to
> >         __yp_bind_client_create() where the duplicate code used
> >         to reside
> >      c) the section that talks to ypbind daemon was moved into
> >         the __yp_bind_ypbindprog() function, again, with a call to
> >         __yp_bind_client_create() replacing the duplicate code
> >    And, of course, calls to __yp_bind_file() and
> >    __yp_bind_ypbindprog() are inserted into __yp_bind() where the
> >    code used to reside.  Also, like ypbind change (1) above, this
> >    does not alter the functionality at all.

This parts are also ok to me.

> > 2) If /var/yp/binding has bad data in it (e.g., a server that went
> >    offline), then calls do_ypcall() will fail w/o trying to find
> >    a new NIS server.  So, two lines of code are added to do_ypcall()
> >    that, in the event of an error from clnt_call(), try calling
> >    __yp_bind_ypbindprog() (one of our new functions), which in turn
> >    does
> >      clnt_call (client, YPBINDPROC_DOMAIN, ...)
> >    which in turn calls ypbindproc_domain() in the ypbind daemon
> >    which calls test_bindings_once() and hopefully finds a new
> >    server.

And this part is not ok. While the idea is correct, the implementation
does the worst thing that could happen (in conjunction with your
ypbind changes):

If the cached data is invalid, we will now always ask ypbind for
a new server and ypbind will query the server. This is overkill.
If the cached data is invalid, we should at first try if the binding
files are updated. And only if this fails, too, we should start
searching a new server.

I will try to make a new patch based on your today.

  Thorsten

-- 
Thorsten Kukuk       http://www.suse.de/~kukuk/        kukuk@suse.de
SuSE Linux AG        Maxfeldstr. 5                 D-90409 Nuernberg
--------------------------------------------------------------------    
Key fingerprint = A368 676B 5E1B 3E46 CFCE  2D97 F8FD 4E23 56C6 FB4B