[sheepdog] [PATCH v8 1/3] lib: move sockfd cache from sheep to lib
Hitoshi Mitake
mitake.hitoshi at gmail.com
Wed Jul 31 09:47:22 CEST 2013
At Wed, 31 Jul 2013 15:39:05 +0800,
Liu Yuan wrote:
>
> On Wed, Jul 31, 2013 at 04:33:00PM +0900, Hitoshi Mitake wrote:
> > At Wed, 31 Jul 2013 15:27:00 +0800,
> > Liu Yuan wrote:
> > >
> > > On Wed, Jul 31, 2013 at 04:11:46PM +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>
> > > > ---
> > > > v8:
> > > > - rename sockfd_node_del() -> sockfd_cache_del_node() for consistent
> > > > naming
> > > >
> > > > v7:
> > > > - fix a mistake in sockfd_cache_del() made by the previous patch
> > > >
> > > > v6:
> > > > - revive revalidate_node() because it is required for handling corner
> > > > case of sheep and doesn't harm collie's behavior
> > > > - trivial cleaning
> > > >
> > > > v5:
> > > > - make the workqueue for cachefd growth an internal thing in
> > > > lib/sockfd_cache.c
> > > > - remove revalidate_node(), because this relies on the assumption of
> > > > sheep
> > > > - better naming of functions
> > > > - misc cleanings
> > > >
> > > > include/Makefile.am | 3 +-
> > > > include/internal_proto.h | 2 +
> > > > include/sockfd_cache.h | 22 ++
> > > > lib/Makefile.am | 2 +-
> > > > {sheep => lib}/sockfd_cache.c | 125 +++++-----
> > > > sheep/gateway.c | 10 +-
> > > > sheep/group.c | 2 +-
> > > > sheep/sheep.c | 9 +-
> > > > sheep/sheep_priv.h | 12 +-
> > > > sheep/sockfd_cache.c | 509 +----------------------------------------
> > > > 10 files changed, 105 insertions(+), 591 deletions(-)
> > > > create mode 100644 include/sockfd_cache.h
> > > > copy {sheep => lib}/sockfd_cache.c (89%)
> > > >
> > > > 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..d91c56a
> > > > --- /dev/null
> > > > +++ b/include/sockfd_cache.h
> > > > @@ -0,0 +1,22 @@
> > > > +#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 sockfd_cache_put(const struct node_id *nid, struct sockfd *sfd);
> > > > +void sockfd_cache_del_node(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);
> > > > +
> > > > +int sockfd_init(void);
> > > > +
> > > > +/* 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 89%
> > > > copy from sheep/sockfd_cache.c
> > > > copy to lib/sockfd_cache.c
> > > > index 13bb7f6..98cea94 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;
> > > > + }
> > > > +
> > >
> > > Please drop this check. check_idx is in the hot patch, after sockfd_init, we are
> > > guaranteed that grow_wq is properly initialized.
> >
> > I think this check cannot be dropped. There is a possibility that
> > users of sockfd_cache implicitly call do_grow_fds() before calling
> > sockf_init(). It would be a bug. How about putting assert(grow_wq)?
>
> user, who? We (programmer) are the actual users, so do it on our own, not rely
> on the code for such obvious error that can't escape a glance. sockfd
> cache isn't as generic as glibc, let's keep it simple and efficient.
Of course "users" are programmers. My intention is not different from
yours.
OK, current users are only sheep and collie. Let's abandon the
possibility of do_grow_fds() before sockfd_init().
Thanks,
Hitoshi
More information about the sheepdog
mailing list