[sheepdog] [PATCH] introduce wrapper functions for pthread_rwlock

[Liu Yuan]

On Wed, Aug 07, 2013 at 11:46:32PM +0900, MORITA Kazutaka wrote:
> Currently, we don't check the return values of pthread_rwlock_rdlock()
> and pthread_rwlock_wrlock().  When they returns an error, we can call
> pthread_rwlock_unlock() even if another thread has the lock.  This can
> lead to a bug which is difficult to find.
> 
> Those functions can actually return error.  For example, when
> pthread_rwlock_rdlock() is called when a write lock is already owned,
> it returns EDEADLK without locking.  It is difficult to make sure that
> we don't have this kind of deadlock, so checking the return values
> strictly is important to ensure our codes is correct.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp>
> ---
>  collie/farm/farm.c        |  6 ++---
>  include/util.h            | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/sockfd_cache.c        | 36 +++++++++++++++---------------
>  sheep/cluster/zookeeper.c | 32 +++++++++++++--------------
>  sheep/md.c                | 42 +++++++++++++++++------------------
>  sheep/object_cache.c      | 44 ++++++++++++++++++-------------------
>  sheep/object_list_cache.c | 26 +++++++++++-----------
>  sheep/vdi.c               | 22 +++++++++----------
>  sheepfs/volume.c          | 30 ++++++++++++-------------
>  9 files changed, 175 insertions(+), 119 deletions(-)
> 
> diff --git a/collie/farm/farm.c b/collie/farm/farm.c
> index 2a4e307..3da9f8d 100644
> --- a/collie/farm/farm.c
> +++ b/collie/farm/farm.c
> @@ -21,7 +21,7 @@
>  static char farm_object_dir[PATH_MAX];
>  static char farm_dir[PATH_MAX];
>  
> -static pthread_rwlock_t vdi_list_lock = PTHREAD_RWLOCK_INITIALIZER;
> +static sd_mutex_t vdi_list_lock = SD_MUTEX_INITIALIZER;
>  struct vdi_entry {
>  	char name[SD_MAX_VDI_LEN];
>  	uint64_t vdi_size;
> @@ -350,9 +350,9 @@ static void do_load_object(struct work *work)
>  				   sw->entry.nr_copies) < 0)
>  			goto error;
>  
> -		pthread_rwlock_wrlock(&vdi_list_lock);
> +		sd_mutex_wrlock(&vdi_list_lock);
>  		insert_vdi(buffer);
> -		pthread_rwlock_unlock(&vdi_list_lock);
> +		sd_mutex_unlock(&vdi_list_lock);
>  	}
>  
>  	farm_show_progress(uatomic_add_return(&loaded, 1), trunk_get_count());
> diff --git a/include/util.h b/include/util.h
> index 0d50f4f..bfbe451 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -11,6 +11,8 @@
>  #include <search.h>
>  #include <urcu/uatomic.h>
>  #include <sys/eventfd.h>
> +#include <pthread.h>
> +#include <errno.h>
>  
>  #include "logger.h"
>  #include "bitops.h"
> @@ -257,6 +259,60 @@ static inline int refcount_dec(refcnt_t *rc)
>  	return uatomic_sub_return(&rc->val, 1);
>  }
>  
> +/* wrapper for pthread_rwlock */
> +
> +#define SD_MUTEX_INITIALIZER PTHREAD_RWLOCK_INITIALIZER
> +
> +typedef pthread_rwlock_t sd_mutex_t;
> +
> +static inline void sd_mutex_init(sd_mutex_t *mutex)
> +{
> +	int ret;
> +
> +	do {
> +		ret = pthread_rwlock_init(mutex, NULL);
> +	} while (ret == EAGAIN);
> +
> +	if (ret != 0)
> +		panic("failed to initialize a mutex, %s", strerror(ret));
> +}
> +
> +static inline void sd_mutex_destroy(sd_mutex_t *mutex)
> +{
> +	int ret = pthread_rwlock_destroy(mutex);
> +
> +	if (ret != 0)
> +		panic("failed to destroy a mutex, %s", strerror(ret));
> +}
> +
> +static inline void sd_mutex_rdlock(sd_mutex_t *mutex)
> +{
> +	int ret;
> +
> +	do {
> +		ret = pthread_rwlock_rdlock(mutex);
> +	} while (ret == EAGAIN);
> +
> +	if (ret != 0)
> +		panic("failed to lock for reading, %s", strerror(ret));
> +}
> +
> +static inline void sd_mutex_wrlock(sd_mutex_t *mutex)
> +{
> +	int ret = pthread_rwlock_wrlock(mutex);
> +
> +	if (ret != 0)
> +		panic("failed to lock for writing, %s", strerror(ret));
> +}
> +
> +static inline void sd_mutex_unlock(sd_mutex_t *mutex)
> +{
> +	int ret = pthread_rwlock_unlock(mutex);
> +
> +	if (ret != 0)
> +		panic("failed to unlock, %s", strerror(ret));
> +}

sd_mutex_t       -> sd_lock

sd_mutex_init    -> sd_init_lock
sd_mutex_unlock  -> sd_unlock
sd_mutex_rdlock  -> sd_read_lock
sd_mutex_wrlock  -> sd_write_lock
sd_mutex_destroy -> sd_destroy_lock

Thanks
Yuan