svn commit: r255716 - head/sbin/hastd

[Yamagi Burmeister]

Thank you for your hard work and help :)

On Thu, 19 Sep 2013 20:19:09 +0000 (UTC)
Mikolaj Golub <trociny at FreeBSD.org> wrote:

> Author: trociny
> Date: Thu Sep 19 20:19:08 2013
> New Revision: 255716
> URL: http://svnweb.freebsd.org/changeset/base/255716
> 
> Log:
>   When updating the map of dirty extents, most recently used extents are
>   kept dirty to reduce the number of on-disk metadata updates. The
>   sequence of operations is:
>   
>   1) acquire the activemap lock;
>   2) update in-memory map;
>   3) if the list of keepdirty extents is changed, update on-disk metadata;
>   4) release the lock.
>   
>   On-disk updates are not frequent in comparison with in-memory updates,
>   while require much more time. So situations are possible when one
>   thread is updating on-disk metadata and another one is waiting for the
>   activemap lock just to update the in-memory map.
>   
>   Improve this by introducing additional, on-disk map lock: when
>   in-memory map is updated and it is detected that the on-disk map needs
>   update too, the on-disk map lock is acquired and the on-memory lock is
>   released before flushing the map.
>   
>   Reported by:	Yamagi Burmeister yamagi.org
>   Tested by:	Yamagi Burmeister yamagi.org
>   Reviewed by:	pjd
>   Approved by:	re (marius)
>   MFC after:	2 weeks
> 
> Modified:
>   head/sbin/hastd/hast.h
>   head/sbin/hastd/primary.c
> 
> Modified: head/sbin/hastd/hast.h
> ==============================================================================
> --- head/sbin/hastd/hast.h	Thu Sep 19 20:17:50 2013	(r255715)
> +++ head/sbin/hastd/hast.h	Thu Sep 19 20:19:08 2013	(r255716)
> @@ -226,8 +226,10 @@ struct hast_resource {
>  
>  	/* Activemap structure. */
>  	struct activemap *hr_amp;
> -	/* Locked used to synchronize access to hr_amp. */
> +	/* Lock used to synchronize access to hr_amp. */
>  	pthread_mutex_t hr_amp_lock;
> +	/* Lock used to synchronize access to hr_amp diskmap. */
> +	pthread_mutex_t hr_amp_diskmap_lock;
>  
>  	/* Number of BIO_READ requests. */
>  	uint64_t	hr_stat_read;
> 
> Modified: head/sbin/hastd/primary.c
> ==============================================================================
> --- head/sbin/hastd/primary.c	Thu Sep 19 20:17:50 2013	(r255715)
> +++ head/sbin/hastd/primary.c	Thu Sep 19 20:19:08 2013	(r255716)
> @@ -291,22 +291,28 @@ primary_exitx(int exitcode, const char *
>  	exit(exitcode);
>  }
>  
> +/* Expects res->hr_amp locked, returns unlocked. */
>  static int
>  hast_activemap_flush(struct hast_resource *res)
>  {
>  	const unsigned char *buf;
>  	size_t size;
> +	int ret;
>  
> +	mtx_lock(&res->hr_amp_diskmap_lock);
>  	buf = activemap_bitmap(res->hr_amp, &size);
> +	mtx_unlock(&res->hr_amp_lock);
>  	PJDLOG_ASSERT(buf != NULL);
>  	PJDLOG_ASSERT((size % res->hr_local_sectorsize) == 0);
> +	ret = 0;
>  	if (pwrite(res->hr_localfd, buf, size, METADATA_SIZE) !=
>  	    (ssize_t)size) {
>  		pjdlog_errno(LOG_ERR, "Unable to flush activemap to disk");
>  		res->hr_stat_activemap_write_error++;
> -		return (-1);
> +		ret = -1;
>  	}
> -	if (res->hr_metaflush == 1 && g_flush(res->hr_localfd) == -1) {
> +	if (ret == 0 && res->hr_metaflush == 1 &&
> +	    g_flush(res->hr_localfd) == -1) {
>  		if (errno == EOPNOTSUPP) {
>  			pjdlog_warning("The %s provider doesn't support flushing write cache. Disabling it.",
>  			    res->hr_localpath);
> @@ -315,10 +321,11 @@ hast_activemap_flush(struct hast_resourc
>  			pjdlog_errno(LOG_ERR,
>  			    "Unable to flush disk cache on activemap update");
>  			res->hr_stat_activemap_flush_error++;
> -			return (-1);
> +			ret = -1;
>  		}
>  	}
> -	return (0);
> +	mtx_unlock(&res->hr_amp_diskmap_lock);
> +	return (ret);
>  }
>  
>  static bool
> @@ -783,6 +790,7 @@ init_remote(struct hast_resource *res, s
>  		 * Now that we merged bitmaps from both nodes, flush it to the
>  		 * disk before we start to synchronize.
>  		 */
> +		mtx_lock(&res->hr_amp_lock);
>  		(void)hast_activemap_flush(res);
>  	}
>  	nv_free(nvin);
> @@ -1288,8 +1296,9 @@ ggate_recv_thread(void *arg)
>  			    ggio->gctl_offset, ggio->gctl_length)) {
>  				res->hr_stat_activemap_update++;
>  				(void)hast_activemap_flush(res);
> +			} else {
> +				mtx_unlock(&res->hr_amp_lock);
>  			}
> -			mtx_unlock(&res->hr_amp_lock);
>  			break;
>  		case BIO_DELETE:
>  			res->hr_stat_delete++;
> @@ -1650,8 +1659,9 @@ done_queue:
>  			if (activemap_need_sync(res->hr_amp, ggio->gctl_offset,
>  			    ggio->gctl_length)) {
>  				(void)hast_activemap_flush(res);
> +			} else {
> +				mtx_unlock(&res->hr_amp_lock);
>  			}
> -			mtx_unlock(&res->hr_amp_lock);
>  			if (hio->hio_replication == HAST_REPLICATION_MEMSYNC)
>  				(void)refcnt_release(&hio->hio_countdown);
>  		}
> @@ -1918,8 +1928,9 @@ ggate_send_thread(void *arg)
>  			    ggio->gctl_offset, ggio->gctl_length)) {
>  				res->hr_stat_activemap_update++;
>  				(void)hast_activemap_flush(res);
> +			} else {
> +				mtx_unlock(&res->hr_amp_lock);
>  			}
> -			mtx_unlock(&res->hr_amp_lock);
>  		}
>  		if (ggio->gctl_cmd == BIO_WRITE) {
>  			/*
> @@ -2015,8 +2026,11 @@ sync_thread(void *arg __unused)
>  			 */
>  			if (activemap_extent_complete(res->hr_amp, syncext))
>  				(void)hast_activemap_flush(res);
> +			else
> +				mtx_unlock(&res->hr_amp_lock);
> +		} else {
> +			mtx_unlock(&res->hr_amp_lock);
>  		}
> -		mtx_unlock(&res->hr_amp_lock);
>  		if (dorewind) {
>  			dorewind = false;
>  			if (offset == -1)
> _______________________________________________
> svn-src-all at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscribe at freebsd.org"


-- 
Homepage:  www.yamagi.org
XMPP:      yamagi at yamagi.org
GnuPG/GPG: 0xEFBCCBCB