[PATCH] sm-notify: perform DNS lookup in the background.

[PATCH] sm-notify: perform DNS lookup in the background.

[Neil Brown]

Hi Steve,
 if you agree with the following patch, it can be pulled from

  git://neil.brown.name/nfs-utils for-steved


Thanks,
NeilBrown


From: Neil Brown <neilb@suse.de>
Date: Tue, 15 Jul 2008 06:11:55 +1000
Subject: [PATCH] sm-notify: perform DNS lookup in the background.

If an NFS server has no network connectivity when it reboots,
it will block in sm-notify waiting for DNS lookup for a potentially
large number of hosts.  This is not helpful and just annoys the
sysadmin.

So do the DNS lookup in the backgrounded phase of sm-notify,
before sending off the NOTIFY requests.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 utils/statd/sm-notify.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index bb67c37..8e00aac 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -286,6 +286,20 @@ notify(void)
 			hp = hosts;
 			hosts = hp->next;
 
+			if (hp->ai == NULL)
+				hp->ai = host_lookup(AF_UNSPEC, hp->name);
+			if (hp->ai == NULL) {
+				nsm_log(LOG_WARNING,
+					"%s doesn't seem to be a valid address,"
+					" skipped",
+					hp->name);
+				unlink(hp->path);
+				free(hp->name);
+				free(hp->path);
+				free(hp);
+				continue;
+			}
+
 			notify_host(sock, hp);
 
 			/* Set the timeout for this call, using an
@@ -532,15 +546,6 @@ get_hosts(const char *dirname)
 		if (stat(path, &stb) < 0)
 			continue;
 
-		host->ai = host_lookup(AF_UNSPEC, de->d_name);
-		if (! host->ai) {
-			nsm_log(LOG_WARNING,
-				"%s doesn't seem to be a valid address, skipped",
-				de->d_name);
-			unlink(path);
-			continue;
-		}
-
 		host->last_used = stb.st_mtime;
 		host->timeout = NSM_TIMEOUT;
 		host->path = strdup(path);
-- 
1.5.6.2

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Chuck Lever]

On Tue, Jul 15, 2008 at 2:21 AM, Neil Brown <neilb@suse.de> wrote:
>
>
> Hi Steve,
>  if you agree with the following patch, it can be pulled from
>
>  git://neil.brown.name/nfs-utils for-steved
>
>
> Thanks,
> NeilBrown
>
>
> From: Neil Brown <neilb@suse.de>
> Date: Tue, 15 Jul 2008 06:11:55 +1000
> Subject: [PATCH] sm-notify: perform DNS lookup in the background.
>
> If an NFS server has no network connectivity when it reboots,
> it will block in sm-notify waiting for DNS lookup for a potentially
> large number of hosts.  This is not helpful and just annoys the
> sysadmin.
>
> So do the DNS lookup in the backgrounded phase of sm-notify,
> before sending off the NOTIFY requests.

Good idea.  A couple of minor comments below.

>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  utils/statd/sm-notify.c |   23 ++++++++++++++---------
>  1 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
> index bb67c37..8e00aac 100644
> --- a/utils/statd/sm-notify.c
> +++ b/utils/statd/sm-notify.c
> @@ -286,6 +286,20 @@ notify(void)
>                        hp = hosts;
>                        hosts = hp->next;
>
> +                       if (hp->ai == NULL)
> +                               hp->ai = host_lookup(AF_UNSPEC, hp->name);
> +                       if (hp->ai == NULL) {
> +                               nsm_log(LOG_WARNING,
> +                                       "%s doesn't seem to be a valid address,"
> +                                       " skipped",
> +                                       hp->name);
> +                               unlink(hp->path);
> +                               free(hp->name);
> +                               free(hp->path);
> +                               free(hp);
> +                               continue;
> +                       }
> +

Arguably it would be cleaner architecturally if the DNS lookup were in
notify_host().  Since doing it in notify() means you will be looking
up these addresses on retries, maybe the host_lookup() call should be
integrated into the "If we retransmitted 4 times" logic.  That would
be an opportunity to fix the addrinfo leak in there.

[Note that freeaddrinfo(3) simply walks the list of addrinfo
structures and frees them.  By setting ai_next to NULL in some of the
addrinfo structures, sm-notify is orphaning them -- freeaddrinfo(3)
will never find them].

>                        notify_host(sock, hp);
>
>                        /* Set the timeout for this call, using an
> @@ -532,15 +546,6 @@ get_hosts(const char *dirname)

You should probably fix the documenting comment for get_hosts() --
"convert them to host entries" might be more precise.

>                if (stat(path, &stb) < 0)
>                        continue;
>
> -               host->ai = host_lookup(AF_UNSPEC, de->d_name);
> -               if (! host->ai) {
> -                       nsm_log(LOG_WARNING,
> -                               "%s doesn't seem to be a valid address, skipped",
> -                               de->d_name);
> -                       unlink(path);
> -                       continue;
> -               }
> -
>                host->last_used = stb.st_mtime;
>                host->timeout = NSM_TIMEOUT;
>                host->path = strdup(path);
> --
> 1.5.6.2

-- 
Chuck Lever

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Steve Dickson]

Chuck Lever wrote:
> On Tue, Jul 15, 2008 at 2:21 AM, Neil Brown <neilb@suse.de> wrote:

>> If an NFS server has no network connectivity when it reboots,
>> it will block in sm-notify waiting for DNS lookup for a potentially
>> large number of hosts.  This is not helpful and just annoys the
>> sysadmin.
>>
>> So do the DNS lookup in the backgrounded phase of sm-notify,
>> before sending off the NOTIFY requests.
> 
> Good idea.  A couple of minor comments below.
> 

> 
> Arguably it would be cleaner architecturally if the DNS lookup were in
> notify_host().  Since doing it in notify() means you will be looking
> up these addresses on retries, maybe the host_lookup() call should be
> integrated into the "If we retransmitted 4 times" logic.  That would
> be an opportunity to fix the addrinfo leak in there.
> 
> [Note that freeaddrinfo(3) simply walks the list of addrinfo
> structures and frees them.  By setting ai_next to NULL in some of the
> addrinfo structures, sm-notify is orphaning them -- freeaddrinfo(3)
> will never find them].
> 

>>                        /* Set the timeout for this call, using an
>> @@ -532,15 +546,6 @@ get_hosts(const char *dirname)
> 
> You should probably fix the documenting comment for get_hosts() --
> "convert them to host entries" might be more precise.
> 


I believe the following patch address those minor comments... true?

Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index bb67c37..2fc8ec6 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -76,7 +76,7 @@ static int		log_syslog = 0;
 
 static unsigned int	nsm_get_state(int);
 static void		notify(void);
-static void		notify_host(int, struct nsm_host *);
+static int		notify_host(int, struct nsm_host *);
 static void		recv_reply(int);
 static void		backup_hosts(const char *, const char *);
 static void		get_hosts(const char *);
@@ -286,7 +286,13 @@ notify(void)
 			hp = hosts;
 			hosts = hp->next;
 
-			notify_host(sock, hp);
+			if (notify_host(sock, hp)){
+				unlink(hp->path);
+				free(hp->name);
+				free(hp->path);
+				free(hp);
+				continue;
+			}
 
 			/* Set the timeout for this call, using an
 			   exponential timeout strategy */
@@ -318,7 +324,7 @@ notify(void)
 /*
  * Send notification to a single host
  */
-void
+int
 notify_host(int sock, struct nsm_host *host)
 {
 	static unsigned int	xid = 0;
@@ -331,6 +337,15 @@ notify_host(int sock, struct nsm_host *host)
 	if (!host->xid)
 		host->xid = xid++;
 
+	if (hp->ai == NULL) {
+		hp->ai = host_lookup(AF_UNSPEC, hp->name);
+		if (hp->ai == NULL)
+			nsm_log(LOG_WARNING,
+				"%s doesn't seem to be a valid address,"
+				" skipped", hp->name);
+		return 1;
+	}
+
 	memset(msgbuf, 0, sizeof(msgbuf));
 	p = msgbuf;
 	*p++ = htonl(host->xid);
@@ -349,7 +364,6 @@ notify_host(int sock, struct nsm_host *host)
 		while ( *next )
 			next = & (*next)->ai_next;
 		*next = hold;
-		hold->ai_next = NULL;
 		memcpy(&host->addr, hold->ai_addr, hold->ai_addrlen);
 		addr_set_port(&host->addr, 0);
 		host->retries = 0;
@@ -394,7 +408,11 @@ notify_host(int sock, struct nsm_host *host)
 	}
 	len = (p - msgbuf) << 2;
 
-	sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest));
+	if (sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest)) < 0)
+		nsm_log(LOG_WARNING, "Sending Reboot Notification to "
+			"'%s' failed: errno %d (%s)", host->name, errno, strerror(errno));
+	
+	return 0;
 }
 
 /*
@@ -504,7 +522,7 @@ backup_hosts(const char *dirname, const char *bakname)
 }
 
 /*
- * Get all entries from sm.bak and convert them to host names
+ * Get all entries from sm.bak and convert them to host entries
  */
 static void
 get_hosts(const char *dirname)
@@ -532,15 +550,6 @@ get_hosts(const char *dirname)
 		if (stat(path, &stb) < 0)
 			continue;
 
-		host->ai = host_lookup(AF_UNSPEC, de->d_name);
-		if (! host->ai) {
-			nsm_log(LOG_WARNING,
-				"%s doesn't seem to be a valid address, skipped",
-				de->d_name);
-			unlink(path);
-			continue;
-		}
-
 		host->last_used = stb.st_mtime;
 		host->timeout = NSM_TIMEOUT;
 		host->path = strdup(path);

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Neil Brown]

On Wednesday July 16, SteveD@redhat.com wrote:
> 
> > 
> > Arguably it would be cleaner architecturally if the DNS lookup were in
> > notify_host().  Since doing it in notify() means you will be looking
> > up these addresses on retries, maybe the host_lookup() call should be
> > integrated into the "If we retransmitted 4 times" logic.  That would
> > be an opportunity to fix the addrinfo leak in there.

What addrinfo leak is that?

> > 
> > [Note that freeaddrinfo(3) simply walks the list of addrinfo
> > structures and frees them.  By setting ai_next to NULL in some of the
> > addrinfo structures, sm-notify is orphaning them -- freeaddrinfo(3)
> > will never find them].
> > 

I assume we are talking about the code in notify_host in the
  if (host->retries >= 4) {
branch.

What the code is doing is taking the first addrinfo from the host->ai
list, and moving it to the end - effectively rotating the list.
This shouldn't change what freeaddrinfo will do.

One of us is missing something.
> 
> 
> I believe the following patch address those minor comments... true?

Except for:

> @@ -349,7 +364,6 @@ notify_host(int sock, struct nsm_host *host)
>  		while ( *next )
>  			next = & (*next)->ai_next;
>  		*next = hold;
> -		hold->ai_next = NULL;
>  		memcpy(&host->addr, hold->ai_addr, hold->ai_addrlen);
>  		addr_set_port(&host->addr, 0);
>  		host->retries = 0;

which I think is wrong, and wondering why:

> @@ -394,7 +408,11 @@ notify_host(int sock, struct nsm_host *host)
>  	}
>  	len = (p - msgbuf) << 2;
>  
> -	sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest));
> +	if (sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest)) < 0)
> +		nsm_log(LOG_WARNING, "Sending Reboot Notification to "
> +			"'%s' failed: errno %d (%s)", host->name, errno, strerror(errno));
> +	
> +	return 0;
>  }
>  
>  /*

Yes, I believe it addresses those comments.

Acked-by: NeilBrown <neilb@suse.de>

Thanks!
NeilBrown

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Steve Dickson]

Neil Brown wrote:
> On Wednesday July 16, SteveD@redhat.com wrote:
>>> Arguably it would be cleaner architecturally if the DNS lookup were in
>>> notify_host().  Since doing it in notify() means you will be looking
>>> up these addresses on retries, maybe the host_lookup() call should be
>>> integrated into the "If we retransmitted 4 times" logic.  That would
>>> be an opportunity to fix the addrinfo leak in there.
> 
> What addrinfo leak is that?
See questions below...

> 
>>> [Note that freeaddrinfo(3) simply walks the list of addrinfo
>>> structures and frees them.  By setting ai_next to NULL in some of the
>>> addrinfo structures, sm-notify is orphaning them -- freeaddrinfo(3)
>>> will never find them].
>>>
> 
> I assume we are talking about the code in notify_host in the
>   if (host->retries >= 4) {
> branch.
> 
> What the code is doing is taking the first addrinfo from the host->ai
> list, and moving it to the end - effectively rotating the list.
> This shouldn't change what freeaddrinfo will do.
> 
> One of us is missing something.
>>
>> I believe the following patch address those minor comments... true?
> 
> Except for:
> 
>> @@ -349,7 +364,6 @@ notify_host(int sock, struct nsm_host *host)
>>  		while ( *next )
>>  			next = & (*next)->ai_next;
>>  		*next = hold;
>> -		hold->ai_next = NULL;
>>  		memcpy(&host->addr, hold->ai_addr, hold->ai_addrlen);
>>  		addr_set_port(&host->addr, 0);
>>  		host->retries = 0;
> 
> which I think is wrong, and wondering why:
After the while loop doesn't hold point to the head of the list?
If so, setting hold->ai_next = NULL; orphans the rest of the list, right?
Or am I missing something... 

steved.
  

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Chuck Lever]

On Wed, Jul 16, 2008 at 10:41 PM, Neil Brown <neilb@suse.de> wrote:
> On Wednesday July 16, SteveD@redhat.com wrote:
>>
>> >
>> > Arguably it would be cleaner architecturally if the DNS lookup were in
>> > notify_host().  Since doing it in notify() means you will be looking
>> > up these addresses on retries, maybe the host_lookup() call should be
>> > integrated into the "If we retransmitted 4 times" logic.  That would
>> > be an opportunity to fix the addrinfo leak in there.
>
> What addrinfo leak is that?
>
>> >
>> > [Note that freeaddrinfo(3) simply walks the list of addrinfo
>> > structures and frees them.  By setting ai_next to NULL in some of the
>> > addrinfo structures, sm-notify is orphaning them -- freeaddrinfo(3)
>> > will never find them].
>> >
>
> I assume we are talking about the code in notify_host in the
>  if (host->retries >= 4) {
> branch.
>
> What the code is doing is taking the first addrinfo from the host->ai
> list, and moving it to the end - effectively rotating the list.
> This shouldn't change what freeaddrinfo will do.
>
> One of us is missing something.

Even after staring at this for half an hour last week, it looked to me
like you were chopping off the last addrinfo in the list each time
through the "retries" loop.  I could hope for something a little more
straightforward to do the address rotation.

It would be nice to consolidate the acquisition and freeing of the
addrinfo structs for each host so the lifetime of the results is made
clear.  Moving the host_lookup() into notify_host() does help with
that.

There is another addrinfo leak in notify(), I think.  In the "Bind
source IP if provided on command line" paragraph, host_lookup() is
called and the address is copied, but that list of addrinfo structs is
never freed, is it?

>> I believe the following patch address those minor comments... true?
>
> Except for:
>
>> @@ -349,7 +364,6 @@ notify_host(int sock, struct nsm_host *host)
>>               while ( *next )
>>                       next = & (*next)->ai_next;
>>               *next = hold;
>> -             hold->ai_next = NULL;
>>               memcpy(&host->addr, hold->ai_addr, hold->ai_addrlen);
>>               addr_set_port(&host->addr, 0);
>>               host->retries = 0;
>
> which I think is wrong, and wondering why:
>
>> @@ -394,7 +408,11 @@ notify_host(int sock, struct nsm_host *host)
>>       }
>>       len = (p - msgbuf) << 2;
>>
>> -     sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest));
>> +     if (sendto(sock, msgbuf, len, 0, (struct sockaddr *) &dest, sizeof(dest)) < 0)
>> +             nsm_log(LOG_WARNING, "Sending Reboot Notification to "
>> +                     "'%s' failed: errno %d (%s)", host->name, errno, strerror(errno));
>> +
>> +     return 0;
>>  }

Yes, it's good to check for a return code here; but I'm wondering why
change it in this patch?

>>
>>  /*
>
> Yes, I believe it addresses those comments.
>
> Acked-by: NeilBrown <neilb@suse.de>
>
> Thanks!
> NeilBrown

--
Chuck Lever

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Neil Brown]

On Thursday July 17, SteveD@redhat.com wrote:
> > 
> >> @@ -349,7 +364,6 @@ notify_host(int sock, struct nsm_host *host)
		struct addrinfo *hold = host->ai;
		struct addrinfo **next = &host->ai;
		*next = hold->ai_next;
> >>  		while ( *next )
> >>  			next = & (*next)->ai_next;
> >>  		*next = hold;
> >> -		hold->ai_next = NULL;
> >>  		memcpy(&host->addr, hold->ai_addr, hold->ai_addrlen);
> >>  		addr_set_port(&host->addr, 0);
> >>  		host->retries = 0;
> > 
> > which I think is wrong, and wondering why:
> After the while loop doesn't hold point to the head of the list?
> If so, setting hold->ai_next = NULL; orphans the rest of the list, right?
> Or am I missing something... 
> 
> steved.
>   

After the while loop, hold points to the original head of the list.
However the new head of this list was put in place by
        *next = hold->ai_next;

as next == &host->ai, the above line is equivalent to

        host->ai = host->ai->ai_next;

which clearly move the head pointer to the second entry.
hold->ai_next also points to this new head, so we have to clear it
(hold->ai = NULL) when we attach it to the end of the list
(*next = hold;, after the while loop).

NeilBrown

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Neil Brown]

On Thursday July 17, chucklever@gmail.com wrote:
> > What the code is doing is taking the first addrinfo from the host->ai
> > list, and moving it to the end - effectively rotating the list.
> > This shouldn't change what freeaddrinfo will do.
> >
> > One of us is missing something.
> 
> Even after staring at this for half an hour last week, it looked to me
> like you were chopping off the last addrinfo in the list each time
> through the "retries" loop.  I could hope for something a little more
> straightforward to do the address rotation.

I assume from your use of the past-tense (looked) that you can see how
it works now?
I guess you could argue that it has been over-optimised, and that
something like:

		struct addrinfo *first = host->ai;
		struct addrinfo **next = &host->ai;

		/* remove the first entry from the list */
		host->ai = first->ai_next;
		first->ai_next = NULL;
		/* find the end of the list */
		next = &host->ai;
		while ( *next )
			next = & (*next)->ai_next;
		/* put first entry at end */
		*next = first;
		memcpy(&host->addr, first->ai_addr, first->ai_addrlen);
		addr_set_port(&host->addr, 0);
		host->retries = 0;

might be a little clearer?


> 
> It would be nice to consolidate the acquisition and freeing of the
> addrinfo structs for each host so the lifetime of the results is made
> clear.  Moving the host_lookup() into notify_host() does help with
> that.
> 
> There is another addrinfo leak in notify(), I think.  In the "Bind
> source IP if provided on command line" paragraph, host_lookup() is
> called and the address is copied, but that list of addrinfo structs is
> never freed, is it?

Yes, that is theoretically a leak, though as it is only called once in
a program with a limited lifetime it isn't a serious leak.

The reality with this program, is that it:
   allocated lots of data structures
   processes them and frees them
   exits

as the exit will implicitly free everything, all the other calls to
free allocated memory are fairly moot.  But I would have no objection
to putting a freeaddrinfo call into that part of notify().

NeilBrown

Re: [PATCH] sm-notify: perform DNS lookup in the background.

[Chuck Lever]

On Thu, Jul 17, 2008 at 6:59 PM, Neil Brown <neilb@suse.de> wrote:
> On Thursday July 17, chucklever@gmail.com wrote:
>> > What the code is doing is taking the first addrinfo from the host->ai
>> > list, and moving it to the end - effectively rotating the list.
>> > This shouldn't change what freeaddrinfo will do.
>> >
>> > One of us is missing something.
>>
>> Even after staring at this for half an hour last week, it looked to me
>> like you were chopping off the last addrinfo in the list each time
>> through the "retries" loop.  I could hope for something a little more
>> straightforward to do the address rotation.
>
> I assume from your use of the past-tense (looked) that you can see how
> it works now?

Yes, and...

> I guess you could argue that it has been over-optimised, and that
> something like:
>
>                struct addrinfo *first = host->ai;
>                struct addrinfo **next = &host->ai;
>
>                /* remove the first entry from the list */
>                host->ai = first->ai_next;
>                first->ai_next = NULL;
>                /* find the end of the list */
>                next = &host->ai;
>                while ( *next )
>                        next = & (*next)->ai_next;
>                /* put first entry at end */
>                *next = first;
>                memcpy(&host->addr, first->ai_addr, first->ai_addrlen);
>                addr_set_port(&host->addr, 0);
>                host->retries = 0;
>
> might be a little clearer?

Yes.  Though I'm still a little leary of "next = & (*next)->ai_next;".
 I think that's the part that confused me initially.  Considering we
all have tens or hundreds of KLOC to look after, I like the little
details to be butt-simple.  (I said "butt." ;-)

I guess my preferred solution would be to house an extra pointer in
host-> that simply referred to the addrinfo struct of current
interest.  Re-arranging linked lists is somewhat expensive and
error-prone, but this is not a performance path, so no big deal.

It would also be slightly more bullet-proof to repeat the
host_lookup() call after you've rotated through all the existing
addresses, rather than simply forcing a rebind periodically.  Probably
not worth the trouble, but it _would_ be possible now that you have
moved the host_lookup() call into the background part of sm-notify.

Note that getaddrinfo(3) will always try to return the most useful
address in the first addrinfo struct (required by Internet standard,
it turns out).  It's worth trying the other addresses, but the first
one will likely produce the correct results.

Furthermore, I don't think the current logic adequately handles the
case where a hostname might resolve to multiple unique hosts (like a
round-robin load-balancing arrangement).  Do you consider this a
common or even a probable case, out of curiousity?

>> It would be nice to consolidate the acquisition and freeing of the
>> addrinfo structs for each host so the lifetime of the results is made
>> clear.  Moving the host_lookup() into notify_host() does help with
>> that.
>>
>> There is another addrinfo leak in notify(), I think.  In the "Bind
>> source IP if provided on command line" paragraph, host_lookup() is
>> called and the address is copied, but that list of addrinfo structs is
>> never freed, is it?
>
> Yes, that is theoretically a leak, though as it is only called once in
> a program with a limited lifetime it isn't a serious leak.

Yes, I agree.  I just like to be tidy.  Tidiness implies that it's
more likely all the various failure modes have been properly addressed
(perhaps I'm fooling myself).  Seeing these little leaks makes me
think the code may have other problems, but that's just the strange
obsessive/compulsive person that I am.

> The reality with this program, is that it:
>   allocated lots of data structures
>   processes them and frees them
>   exits
>
> as the exit will implicitly free everything, all the other calls to
> free allocated memory are fairly moot.  But I would have no objection
> to putting a freeaddrinfo call into that part of notify().

-- 
Chuck Lever