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

Liu Yuan namei.unix at gmail.com
Tue Aug 20 05:19:14 CEST 2013


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.

Thanks
Yuan

> I think my x- prefix naming would be bad. Because other x- prefixed
> wrappers panic simply (e.g. xmalloc()) or retry forever in the case of
> EINTR (e.g. xftruncate()). Should we use other prefix?

It is okay, and if we don't have any means to handle error and caller can't
handle the error either, we then need to panic inside the function.

Thanks
Yuan



More information about the sheepdog mailing list