[sheepdog] [PATCH 3/6] lib: implement wrapper functions for handling EMFILE gracefully

[Hitoshi Mitake]

At Tue, 20 Aug 2013 11:19:14 +0800,
Liu Yuan wrote:
> 
> On Tue, Aug 20, 2013 at 10:13:02AM +0900, Hitoshi Mitake wrote:
> > At Mon, 19 Aug 2013 16:32:39 +0800,
> > Liu Yuan wrote:
> > > 
> > > On Mon, Aug 19, 2013 at 05:26:52PM +0900, Hitoshi Mitake wrote:
> > > > At Mon, 19 Aug 2013 16:19:53 +0800,
> > > > Liu Yuan wrote:
> > > > > 
> > > > > On Mon, Aug 19, 2013 at 05:16:38PM +0900, Hitoshi Mitake wrote:
> > > > > > At Mon, 19 Aug 2013 14:53:19 +0800,
> > > > > > Liu Yuan wrote:
> > > > > > > 
> > > > > > > On Wed, Aug 14, 2013 at 01:27:38AM +0900, Hitoshi Mitake wrote:
> > > > > > > > This patch adds wrapper functions of fd creaters for handling EMFILE
> > > > > > > > gracefully. When they face EMFILE, they reap unused sockfd and retry
> > > > > > > > creating new fds.
> > > > > > > > 
> > > > > > > > This patch also modifies Makefile of shepherd. Because this patch
> > > > > > > > creates a new dependency in libsheepdog, shepherd must linked with
> > > > > > > > pthread.
> > > > > > > > 
> > > > > > > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > > > > > > ---
> > > > > > > >  include/util.h       |  14 +++++++
> > > > > > > >  lib/util.c           | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  shepherd/Makefile.am |   2 +-
> > > > > > > >  3 files changed, 119 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/include/util.h b/include/util.h
> > > > > > > > index ff1f0c7..729037d 100644
> > > > > > > > --- a/include/util.h
> > > > > > > > +++ b/include/util.h
> > > > > > > > @@ -13,6 +13,9 @@
> > > > > > > >  #include <sys/eventfd.h>
> > > > > > > >  #include <pthread.h>
> > > > > > > >  #include <errno.h>
> > > > > > > > +#include <sys/types.h>
> > > > > > > > +#include <sys/socket.h>
> > > > > > > > +#include <fcntl.h>
> > > > > > > >  
> > > > > > > >  #include "logger.h"
> > > > > > > >  #include "list.h"
> > > > > > > > @@ -344,4 +347,15 @@ static inline bool is_stdout_console(void)
> > > > > > > >  extern mode_t sd_def_fmode;
> > > > > > > >  extern mode_t sd_def_dmode;
> > > > > > > >  
> > > > > > > > +/*
> > > > > > > > + * The below x- prefixed wrappers are for handling EMFILE gracefully.
> > > > > > > > + * They reap fds of sockfd cache and retry when they face EMFILE.
> > > > > > > > + */
> > > > > > > > +int xsocket(int domain, int type, int protocol);
> > > > > > > > +int xaccept(int sockfd, struct sockaddr *addr, socklen_t *addrlen);
> > > > > > > > +int xeventfd(int initval, int flags);
> > > > > > > > +int xopen(const char *pathname, int flags, ...);
> > > > > > > > +int xtimerfd_create(clockid_t clock_id, int flags);
> > > > > > > > +int xepoll_create(int size);
> > > > > > > > +
> > > > > > > >  #endif
> > > > > > > > diff --git a/lib/util.c b/lib/util.c
> > > > > > > > index 9781fd8..b88f297 100644
> > > > > > > > --- a/lib/util.c
> > > > > > > > +++ b/lib/util.c
> > > > > > > > @@ -22,8 +22,12 @@
> > > > > > > >  #include <signal.h>
> > > > > > > >  #include <sys/xattr.h>
> > > > > > > >  #include <fcntl.h>
> > > > > > > > +#include <stdarg.h>
> > > > > > > > +#include <sys/timerfd.h>
> > > > > > > > +#include <sys/epoll.h>
> > > > > > > >  
> > > > > > > >  #include "util.h"
> > > > > > > > +#include "sockfd_cache.h"
> > > > > > > >  
> > > > > > > >  mode_t sd_def_dmode = S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP | S_IXGRP;
> > > > > > > >  mode_t sd_def_fmode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP;
> > > > > > > > @@ -735,3 +739,103 @@ void list_sort(void *priv, struct list_head *head,
> > > > > > > >  
> > > > > > > >  	merge_and_restore_back_links(priv, cmp, head, part[max_lev], list);
> > > > > > > >  }
> > > > > > > > +
> > > > > > > > +int xsocket(int domain, int type, int protocol)
> > > > > > > > +{
> > > > > > > > +	int fd;
> > > > > > > > +
> > > > > > > > +retry:
> > > > > > > > +	fd = socket(domain, type, protocol);
> > > > > > > > +	if (fd < 0) {
> > > > > > > > +		if (errno == EINTR)
> > > > > > > > +			goto retry;
> > > > > > > > +
> > > > > > > > +		if (errno == EMFILE && sockfd_shrink())
> > > > > > > > +			goto retry;
> > > > > > > > +	}
> > > > > > > 
> > > > > > > I think we should loop until some fds are reclaimed in the sockfd_shrink. After
> > > > > > > it returns, it means we have free slots in the sockfd cache.
> > > > > > 
> > > > > > In theory, the situation that sockfd has no fds is possible and it can
> > > > > > block users forever. I think returning with boolean result is a better
> > > > > > way.
> > > > > 
> > > > > What exact sitatution it will block *forever*? If this happens, it is a bug to
> > > > > fix.
> > > > 
> > > > e.g. fds are exhausted by files. Of course the situation would be hard
> > > > to produce but I think it is a valid one.
> > > 
> > > This doesn't meant it loops *forever*.
> > > 
> > > In the case of all the fds are in use, we should actually wait, not error return
> > > to the caller and ask the caller to handle this. For e.g,
> > > 
> > > xeventfd_create should not return to the caller in the case of fds are all in
> > > use. What the caller would do might be panic the sheep.
> > 
> > I think panic is an overkill. e.g. when default_read() faces EMFILE,
> > it produces SD_RES_NETWORK_ERROR as a return value for making gateway
> > try again. So these x- prefixed functions should produce error simply
> > when there are no cached sock fds.
> 
> I didn't meant ask xeventfd to panic inside. For example,
> 
> find_object_cache will call eventfd(), where if eventfd() error returns, it
> should call panic().
> 
> For EMFILE, xeventfd() should loop until success.

Sorry for late reply. I agree with that xeventfd() should loop until
success. But simple retry will be ineffective. I'll add
pthread_yield() between retrying.

I'll send v2 later.

Thanks,
Hitoshi