Re: [PATCH v5 5/6] nilfs-utils: add optimized version of nilfs_xreclaim_segments

[Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>]

On Thu,  6 Feb 2014 15:17:25 +0100, Andreas Rohner wrote:
> This patch adds an additional parameter min_reclaimable_blks to
> the nilfs_reclaim_params structure. This parameter specifies the
> minimum number of reclaimable blocks in a segment, before it can be
> cleaned. If a segment is below this threshold, it is considered to
> be not worth cleaning, because all the live blocks would need to be
> moved to a new segment, which is expensive, and the number of
> reclaimable blocks is too low. But it is still necessary to update
> the segment usage information to turn the old segment into a new
> one.
> 
> This is basically a shortcut to cleaning the segment. It is still
> necessary to read the segment summary information, but the writing
> of the live blocks can be skipped if it's not worth it.
> 
> This optimization can be enabled and disabled by the
> use_set_suinfo switch in the configuration file. If the kernel
> doesn't support the set_suinfo ioctl it returns the error
> number ENOTTY, which also permanently disables the optimization.
> 
> Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  include/nilfs_gc.h       | 11 ++++----
>  lib/gc.c                 | 73 ++++++++++++++++++++++++++++++++++++++++++++----
>  sbin/cleanerd/cleanerd.c | 24 +++++++++++++---
>  3 files changed, 93 insertions(+), 15 deletions(-)
> 
> diff --git a/include/nilfs_gc.h b/include/nilfs_gc.h
> index e49cbf4..8bd83d7 100644
> --- a/include/nilfs_gc.h
> +++ b/include/nilfs_gc.h
> @@ -15,20 +15,21 @@
>  #include "nilfs.h"
>  
>  /* flags for nilfs_reclaim_params struct */
> -#define NILFS_RECLAIM_PARAM_PROTSEQ	(1UL << 0)
> -#define NILFS_RECLAIM_PARAM_PROTCNO	(1UL << 1)
> -#define __NR_NILFS_RECLAIM_PARAMS	2
> +#define NILFS_RECLAIM_PARAM_PROTSEQ			(1UL << 0)
> +#define NILFS_RECLAIM_PARAM_PROTCNO			(1UL << 1)
> +#define NILFS_RECLAIM_PARAM_MIN_RECLAIMABLE_BLKS	(1UL << 2)
> +#define __NR_NILFS_RECLAIM_PARAMS	3
>  
>  /**
>   * struct nilfs_reclaim_params - structure to specify GC parameters
>   * @flags: flags of valid fields
> - * @reserved: padding bytes
> + * @min_reclaimable_blks: minimum number of reclaimable blocks
>   * @protseq: start of sequence number of protected segments
>   * @protcno: start number of checkpoint to be protected
>   */
>  struct nilfs_reclaim_params {
>  	unsigned long flags;
> -	unsigned long reserved;
> +	unsigned long min_reclaimable_blks;
>  	__u64 protseq;
>  	nilfs_cno_t protcno;
>  };
> diff --git a/lib/gc.c b/lib/gc.c
> index 71c7307..5a1c2fe 100644
> --- a/lib/gc.c
> +++ b/lib/gc.c
> @@ -29,6 +29,10 @@
>  #include <syslog.h>
>  #endif	/* HAVE_SYSLOG_H */
>  
> +#if HAVE_SYS_TIME_H
> +#include <sys/time.h>
> +#endif	/* HAVE_SYS_TIME */
> +
>  #include <errno.h>
>  #include <assert.h>
>  #include <stdarg.h>
> @@ -614,11 +618,14 @@ int nilfs_xreclaim_segment(struct nilfs *nilfs,
>  			   const struct nilfs_reclaim_params *params,
>  			   struct nilfs_reclaim_stat *stat)
>  {
> -	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv;
> +	struct nilfs_vector *vdescv, *bdescv, *periodv, *vblocknrv, *supv;
>  	sigset_t sigset, oldset, waitset;
>  	nilfs_cno_t protcno;
> -	ssize_t n, ret = -1;
> +	ssize_t n, i, ret = -1;
>  	size_t nblocks;
> +	__u32 reclaimable_blocks;
> +	struct nilfs_suinfo_update *sup;
> +	struct timeval tv;
>  
>  	if (!(params->flags & NILFS_RECLAIM_PARAM_PROTSEQ) ||
>  	    (params->flags & (~0UL << __NR_NILFS_RECLAIM_PARAMS))) {
> @@ -637,7 +644,8 @@ int nilfs_xreclaim_segment(struct nilfs *nilfs,
>  	bdescv = nilfs_vector_create(sizeof(struct nilfs_bdesc));
>  	periodv = nilfs_vector_create(sizeof(struct nilfs_period));
>  	vblocknrv = nilfs_vector_create(sizeof(__u64));
> -	if (!vdescv || !bdescv || !periodv || !vblocknrv)
> +	supv = nilfs_vector_create(sizeof(struct nilfs_suinfo_update));
> +	if (!vdescv || !bdescv || !periodv || !vblocknrv || !supv)
>  		goto out_vec;
>  
>  	sigemptyset(&sigset);
> @@ -703,13 +711,16 @@ int nilfs_xreclaim_segment(struct nilfs *nilfs,
>  	if (ret < 0)
>  		goto out_lock;
>  
> +	reclaimable_blocks = (nilfs_get_blocks_per_segment(nilfs) * n) -
> +			(nilfs_vector_get_size(vdescv) +
> +			nilfs_vector_get_size(bdescv));
> +
>  	if (stat) {
>  		stat->live_pblks = nilfs_vector_get_size(bdescv);
>  		stat->defunct_pblks = nblocks - stat->live_pblks;
>  
>  		stat->live_blks = stat->live_vblks + stat->live_pblks;
> -		stat->defunct_blks = n * nilfs_get_blocks_per_segment(nilfs) -
> -			stat->live_blks;
> +		stat->defunct_blks = reclaimable_blocks;
>  	}
>  	if (dryrun)
>  		goto out_lock;
> @@ -725,6 +736,54 @@ int nilfs_xreclaim_segment(struct nilfs *nilfs,
>  		goto out_lock;
>  	}
>  
> +	/*
> +	 * if there are less reclaimable blocks than the minimal
> +	 * threshold try to update suinfo instead of cleaning
> +	 */
> +	if ((params->flags & NILFS_RECLAIM_PARAM_MIN_RECLAIMABLE_BLKS) &&
> +			reclaimable_blocks < params->min_reclaimable_blks * n) {

I think nilfs_opt_test_set_suinfo(nilfs) should be tested here instead
of the nilfs_cleanerd_clean_segments() side.

Later, I will fix it.

> +		if (stat) {
> +			stat->deferred_segs = n;
> +			stat->cleaned_segs = 0;
> +		}
> +
> +		ret = gettimeofday(&tv, NULL);
> +		if (ret < 0)
> +			goto out_lock;
> +
> +		for (i = 0; i < n; ++i) {
> +			sup = nilfs_vector_get_new_element(supv);
> +			if (!sup)
> +				goto out_lock;
> +
> +			sup->sup_segnum = segnums[i];
> +			sup->sup_flags = 0;
> +			nilfs_suinfo_update_set_lastmod(sup);
> +			sup->sup_sui.sui_lastmod = tv.tv_sec;
> +		}
> +
> +		ret = nilfs_set_suinfo(nilfs, nilfs_vector_get_data(supv), n);
> +
> +		if (ret == 0)
> +			goto out_lock;
> +
> +		if (ret < 0 && errno != ENOTTY) {
> +			nilfs_gc_logger(LOG_ERR, "cannot set suinfo: %s",
> +					strerror(errno));
> +			goto out_lock;
> +		}
> +
> +		/* errno == ENOTTY */
> +		nilfs_gc_logger(LOG_WARNING,
> +				"set_suinfo ioctl is not supported");
> +		nilfs_opt_clear_set_suinfo(nilfs);
> +		if (stat) {
> +			stat->deferred_segs = 0;
> +			stat->cleaned_segs = n;
> +		}
> +		/* Try nilfs_clean_segments */
> +	}
> +
>  	ret = nilfs_clean_segments(nilfs,
>  				   nilfs_vector_get_data(vdescv),
>  				   nilfs_vector_get_size(vdescv),
> @@ -759,6 +818,8 @@ out_vec:
>  		nilfs_vector_destroy(periodv);
>  	if (vblocknrv != NULL)
>  		nilfs_vector_destroy(vblocknrv);
> +	if (supv != NULL)
> +		nilfs_vector_destroy(supv);
>  	/*
>  	 * Flags of valid fields in stat->exflags must be unset.
>  	 */
> @@ -783,7 +844,7 @@ ssize_t nilfs_reclaim_segment(struct nilfs *nilfs,
>  
>  	params.flags =
>  		NILFS_RECLAIM_PARAM_PROTSEQ | NILFS_RECLAIM_PARAM_PROTCNO;
> -	params.reserved = 0;
> +	params.min_reclaimable_blks = 0;
>  	params.protseq = protseq;
>  	params.protcno = protcno;
>  	memset(&stat, 0, sizeof(stat));
> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
> index 2896af8..4c9f69b 100644
> --- a/sbin/cleanerd/cleanerd.c
> +++ b/sbin/cleanerd/cleanerd.c
> @@ -1376,7 +1376,7 @@ static void nilfs_cleanerd_progress(struct nilfs_cleanerd *cleanerd, int nsegs)
>  static int nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
>  					 __u64 *segnums, size_t nsegs,
>  					 __u64 protseq, __u64 prottime,
> -					 size_t *ncleaned)
> +					 size_t *ndone)
>  {
>  	struct nilfs_reclaim_params params;
>  	struct nilfs_reclaim_stat stat;
> @@ -1384,7 +1384,11 @@ static int nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
>  
>  	params.flags =
>  		NILFS_RECLAIM_PARAM_PROTSEQ | NILFS_RECLAIM_PARAM_PROTCNO;
> -	params.reserved = 0;
> +	if (nilfs_opt_test_set_suinfo(cleanerd->nilfs))
> +		params.flags |= NILFS_RECLAIM_PARAM_MIN_RECLAIMABLE_BLKS;
> +
> +	params.min_reclaimable_blks =
> +			nilfs_cleanerd_min_reclaimable_blocks(cleanerd);
>  	params.protseq = protseq;
>  
>  	ret = nilfs_cnoconv_time2cno(cleanerd->cnoconv, prottime,
> @@ -1402,7 +1406,7 @@ static int nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
>  		if (errno == ENOMEM) {
>  			nilfs_cleanerd_reduce_ncleansegs_for_retry(cleanerd);
>  			cleanerd->fallback = 1;
> -			*ncleaned = 0;
> +			*ndone = 0;
>  			ret = 0;
>  		}
>  		goto out;
> @@ -1415,6 +1419,17 @@ static int nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
>  		nilfs_cleanerd_progress(cleanerd, stat.cleaned_segs);
>  		cleanerd->fallback = 0;
>  		cleanerd->retry_cleaning = 0;
> +
> +		*ndone = stat.cleaned_segs;
> +	} else if (stat.deferred_segs > 0) {
> +		for (i = 0; i < stat.deferred_segs; i++)
> +			syslog(LOG_DEBUG, "segment %llu deferred",
> +			       (unsigned long long)segnums[i]);
> +		nilfs_cleanerd_progress(cleanerd, stat.deferred_segs);
> +		cleanerd->fallback = 0;
> +		cleanerd->retry_cleaning = 0;
> +
> +		*ndone = stat.deferred_segs;
>  	} else {
>  		syslog(LOG_DEBUG, "no segments cleaned");
>  
> @@ -1428,8 +1443,9 @@ static int nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
>  		} else {
>  			cleanerd->retry_cleaning = 0;
>  		}
> +		*ndone = 0;

This condition judgment supposes that stat.deferred_segs and
stat.cleaned_segs are exclusive.  It is correct in the current
implementation, but I don't want to define the semantics of
nilfs_xreclaim_segment() as such.

Can you rewrite this patch ?
Or, I will change this post handling.

>  	}
> -	*ncleaned = stat.cleaned_segs;
> +
>  out:
>  	return ret;
>  }
> -- 
> 1.8.5.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html