[sheepdog] [PATCH v4 1/2] lib: move sockfd cache from sheep to lib

[Hitoshi Mitake]

At Thu, 25 Jul 2013 15:11:51 +0800,
Liu Yuan wrote:
> 
> On Thu, Jul 25, 2013 at 03:43:54PM +0900, Hitoshi Mitake wrote:
> > On some subcommands, collie also issues lots of request to sheeps. So
> > collie can enjoy sockfd caching. For this purpose, this patch moves
> > sockfd from sheep to lib and generalize the interfaces of sockfd.
> > 
> > This patch doesn't change anything related to IO NIC in sockfd
> > cache. Because collie can know address and port of IO NIC so
> > potentially use them.
> > 
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > ---
> >  include/Makefile.am           |    3 +-
> >  include/internal_proto.h      |    2 +
> >  include/sockfd_cache.h        |   23 ++
> >  lib/Makefile.am               |    2 +-
> >  {sheep => lib}/sockfd_cache.c |  113 +++++----
> >  sheep/gateway.c               |   10 +-
> >  sheep/group.c                 |    2 +-
> >  sheep/sheep.c                 |    2 +
> >  sheep/sheep_priv.h            |   11 +-
> >  sheep/sockfd_cache.c          |  509 +----------------------------------------
> >  10 files changed, 93 insertions(+), 584 deletions(-)
> >  create mode 100644 include/sockfd_cache.h
> >  copy {sheep => lib}/sockfd_cache.c (90%)
> > 
> > diff --git a/include/Makefile.am b/include/Makefile.am
> > index 0acb76e..4d0c229 100644
> > --- a/include/Makefile.am
> > +++ b/include/Makefile.am
> > @@ -2,4 +2,5 @@ MAINTAINERCLEANFILES    = Makefile.in config.h.in
> >  
> >  noinst_HEADERS          = bitops.h event.h logger.h sheepdog_proto.h util.h \
> >  			  list.h net.h sheep.h exits.h strbuf.h rbtree.h \
> > -			  sha1.h option.h internal_proto.h shepherd.h work.h
> > +			  sha1.h option.h internal_proto.h shepherd.h work.h \
> > +			  sockfd_cache.h
> > diff --git a/include/internal_proto.h b/include/internal_proto.h
> > index 0463eae..0061007 100644
> > --- a/include/internal_proto.h
> > +++ b/include/internal_proto.h
> > @@ -20,6 +20,8 @@
> >  #include <stdint.h>
> >  #include <netinet/in.h>
> >  
> > +#include "sheepdog_proto.h"
> > +
> >  #define SD_SHEEP_PROTO_VER 0x08
> >  
> >  #define SD_DEFAULT_COPIES 3
> > diff --git a/include/sockfd_cache.h b/include/sockfd_cache.h
> > new file mode 100644
> > index 0000000..76154c9
> > --- /dev/null
> > +++ b/include/sockfd_cache.h
> > @@ -0,0 +1,23 @@
> > +#ifndef SOCKFD_CACHE_H
> > +#define SOCKFD_CACHE_H
> > +
> > +#include "internal_proto.h"
> > +#include "work.h"
> > +
> > +struct sockfd *sockfd_cache_get(const struct node_id *nid);
> > +void do_sockfd_cache_put(const struct node_id *nid, int idx);
> > +void sockfd_cache_put(const struct node_id *nid, struct sockfd *sfd);
> > +void sockfd_node_del(const struct node_id *nid);
> > +void sockfd_cache_del(const struct node_id *nid, struct sockfd *sfd);
> > +void sockfd_cache_add(const struct node_id *nid);
> > +void sockfd_cache_add_group(const struct sd_node *nodes, int nr);
> > +
> > +void init_sockfd(struct work_queue *wq);
> > +
> > +/* sockfd_cache */
> > +struct sockfd {
> > +	int fd;
> > +	int idx;
> > +};
> > +
> > +#endif	/* SOCKFD_CACHE_H */
> > diff --git a/lib/Makefile.am b/lib/Makefile.am
> > index a95ce4c..22edf6e 100644
> > --- a/lib/Makefile.am
> > +++ b/lib/Makefile.am
> > @@ -5,7 +5,7 @@ INCLUDES                = -I$(top_builddir)/include -I$(top_srcdir)/include
> >  noinst_LIBRARIES	= libsheepdog.a
> >  
> >  libsheepdog_a_SOURCES	= event.c logger.c net.c util.c rbtree.c strbuf.c \
> > -			  sha1.c option.c work.c
> > +			  sha1.c option.c work.c sockfd_cache.c
> >  
> >  # support for GNU Flymake
> >  check-syntax:
> > diff --git a/sheep/sockfd_cache.c b/lib/sockfd_cache.c
> > similarity index 90%
> > copy from sheep/sockfd_cache.c
> > copy to lib/sockfd_cache.c
> > index 13bb7f6..334ba16 100644
> > --- a/sheep/sockfd_cache.c
> > +++ b/lib/sockfd_cache.c
> > @@ -27,7 +27,13 @@
> >   *    7 support dual connections to a single node.
> >   */
> >  
> > -#include "sheep_priv.h"
> > +#include <pthread.h>
> > +
> > +#include "sockfd_cache.h"
> > +#include "work.h"
> > +#include "rbtree.h"
> > +#include "util.h"
> > +#include "sheep.h"
> >  
> >  struct sockfd_cache {
> >  	struct rb_root root;
> > @@ -208,20 +214,6 @@ false_out:
> >  	return false;
> >  }
> >  
> > -/* When node craches, we should delete it from the cache */
> > -void sockfd_cache_del(const struct node_id *nid)
> > -{
> > -	char name[INET6_ADDRSTRLEN];
> > -	int n;
> > -
> > -	if (!sockfd_cache_destroy(nid))
> > -		return;
> > -
> > -	n = uatomic_sub_return(&sockfd_cache.count, 1);
> > -	addr_to_str(name, sizeof(name), nid->addr, 0);
> > -	sd_dprintf("%s:%d, count %d", name, nid->port, n);
> > -}
> > -
> >  static void sockfd_cache_add_nolock(const struct node_id *nid)
> >  {
> >  	struct sockfd_cache_entry *new = xmalloc(sizeof(*new));
> > @@ -282,6 +274,8 @@ void sockfd_cache_add(const struct node_id *nid)
> >  static uatomic_bool fds_in_grow;
> >  static int fds_high_watermark = FDS_WATERMARK(DEFAULT_FDS_COUNT);
> >  
> > +static struct work_queue *grow_wq;
> > +
> >  static void do_grow_fds(struct work *work)
> >  {
> >  	struct sockfd_cache_entry *entry;
> > @@ -318,6 +312,15 @@ static inline void check_idx(int idx)
> >  {
> >  	struct work *w;
> >  
> > +	if (!grow_wq) {
> > +		sd_dprintf("Request for growing fds is issued, but"
> > +			" a work queue for this work is not registered yet.");
> > +		sd_dprintf("Initializing work queue in earlier stage"
> > +			" is suggested.");
> > +
> > +		return;
> > +	}
> > +
> >  	if (idx <= fds_high_watermark)
> >  		return;
> >  	if (!uatomic_set_true(&fds_in_grow))
> > @@ -326,7 +329,7 @@ static inline void check_idx(int idx)
> >  	w = xmalloc(sizeof(*w));
> >  	w->fn = do_grow_fds;
> >  	w->done = grow_fds_done;
> > -	queue_work(sys->sockfd_wqueue, w);
> > +	queue_work(grow_wq, w);
> >  }
> >  
> >  /* Add the node back if it is still alive */
> > @@ -350,7 +353,7 @@ alive:
> >  }
> >  
> >  /* Try to create/get cached IO connection. If failed, fallback to non-IO one */
> > -static struct sockfd *sockfd_cache_get(const struct node_id *nid)
> > +static struct sockfd *do_sockfd_cache_get(const struct node_id *nid)
> >  {
> >  	struct sockfd_cache_entry *entry;
> >  	struct sockfd *sfd;
> > @@ -402,7 +405,7 @@ out:
> >  	return sfd;
> >  }
> >  
> > -static void sockfd_cache_put(const struct node_id *nid, int idx)
> > +void do_sockfd_cache_put(const struct node_id *nid, int idx)
> >  {
> >  	bool use_io = nid->io_port ? true : false;
> >  	const uint8_t *addr = use_io ? nid->io_addr : nid->addr;
> > @@ -442,20 +445,29 @@ static void sockfd_cache_close(const struct node_id *nid, int idx)
> >  }
> >  
> >  /*
> > + * set work queue for growing fds
> > + * before this function called, growing cannot be done
> > + */
> > +void init_sockfd(struct work_queue *wq)
> > +{
> > +	grow_wq = wq;
> > +}
> 
> We use sockfd_ prefix as namespace. So use sockfd_init() instead. I think it is
> better to push work queue init in sockfd_init() and make wq variable private to
> sockfd cache.

OK, I agree with renaming and private wq.

> 
> You rely on the revalidate_node() to add the node, I think this assumption is
> wrong though it works for now.  Add a node explicitly please.

OK, calling revalidate_node() from do_sockfd_cache_get() would be
redundant. I'll remove it.

> 
> do_sockfd_cache_put()/do_sockfd_cache_get() are not good names.
> 
> sockfd_cache_{get, put}_short() is better.

I agree.

> 
> You should brief in the commit what you did, such as what kind of funtions you
> removed. You didn't clean up the .h files when you remove those functions.

Ah, sorry, I forgot to remove the prototype of collie_exec_req_nid().

Thanks,
Hitoshi