Re: cleaner: run one cleaning pass based on minimum free space

[David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 8063 bytes --]

Hi,

here the revised version of the patch

As changelog description we could put:

add options for cleaning based on number of free segments

In order to pass different config files to cleaner while not increasing
mount options, another solution might be adding a mount option
nocleanerd to disable staring of cleanerd. I know, there is mount -i,
but this option would have the advantage that it could be used in
/etc/fstab. In this way, cleaner could be started manually with whatever
options are needed. What would you think about it ? Anyway I think this
should be part of a second patch as it is implementing different
functionality.

Thanks in advance
Bye,
David Arendt

On 03/27/10 18:48, Ryusuke Konishi wrote:
> Hi David,
> On Wed, 24 Mar 2010 06:35:00 +0100, David Arendt wrote:
>   
>> Hi,
>>
>> just for completeness, here is a re-post of the complete patch using
>> cleanerd->c_running instead of local variable "sleeping".
>>
>> Bye,
>> David Arendt
>>     
> Sorry for my late response.
>
> I'm planning to apply your patch.
>
> The patch looks reducible some more, for example, the preparation:
>
>   
>> +       if (cleanerd->c_config.cf_min_clean_segments > 0) {
>> +               syslog(LOG_INFO, "cleaner paused");
>> +               cleanerd->c_running = 0;
>> +               timeout.tv_sec = cleanerd->c_config.cf_clean_check_interval;
>> +               timeout.tv_nsec = 0;
>> +       }
>> +       else
>> +         cleanerd->c_running = 1;
>> +
>>     
> can be simplified as follows:
>
> 	if (cleanerd->c_config.cf_min_clean_segments == 0)
> 		cleanerd->c_running = 1;
>
> And, the status control using cleanerd->c_running seems to have room
> for improvement.  Except for these trivial matters, your change looks
> simple but effective, and is flawlessly keeping compatibility.
>
> If you have a revised patch, please send me for merge.  Also, I would
> appreciate it if you could write some changelog description.
>
> Thank you in advance,
> Ryusuke Konishi
>
>   
>> On 03/17/10 19:11, Ryusuke Konishi wrote:
>>     
>>> Hi,
>>> On Mon, 15 Mar 2010 22:24:28 +0100, David Arendt wrote:
>>>   
>>>       
>>>> Hi,
>>>>
>>>> Well I didn't know that a few days can pass as fast :-)
>>>>
>>>> I have attached the patch to this mail.
>>>>
>>>> Until now the patch has only been shortly tested on a loop device, so it
>>>> might contain bugs and destroy your data.
>>>>     
>>>>         
>>> Thank you for posting the patch!
>>>
>>> The patch looks rougly ok to me.
>>> I'll comment on it later.
>>>
>>> At first glance, I felt it would be nice if cleanerd->c_running is
>>> nicely used instead of adding a local variable "sleeping".
>>>
>>> Thanks,
>>> Ryusuke Konishi
>>>  
>>>   
>>>       
>>>> If you decide to apply it, please change the default values to the ones
>>>> you find the most appropriate.
>>>>
>>>> Thanks in advance,
>>>> Bye,
>>>> David Arendt
>>>>
>>>> On 03/15/10 16:58, Ryusuke Konishi wrote:
>>>>     
>>>>         
>>>>> Hi,
>>>>> On Mon, 15 Mar 2010 00:03:45 +0100, David Arendt wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> Hi,
>>>>>>
>>>>>> I am posting this again to the correct mailing list as I cc'ed it to the
>>>>>> old inactive one.
>>>>>>
>>>>>> Maybe I am understanding something wrong, but if I would use the count
>>>>>> of reclaimed segments, how could I determine if one cleaning pass has
>>>>>> finished as I don't know in advance how many segments could be reclaimed ?
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> For example, how about this?
>>>>>
>>>>>  nmax = (number of segments) - (number of clean segments)
>>>>>  nblk = (max_clean_segments - (number of clean segments)) *
>>>>>             (number of blocks per segment)
>>>>>
>>>>>  * If (number of clean segments) < min_clean_segments, then start reclamation
>>>>>  * Try to reclaim nmax segments (at a maximum).
>>>>>  * When the cleaner found and freed nblk blocks during the
>>>>>    reclamation, then end one cleaning pass.
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>>>> Another approach would be not basing cleaning on a whole cleaning pass
>>>>>> but instead creating these addtional configfile options:
>>>>>>
>>>>>> # start cleaning if less than 100 free segments
>>>>>> min_clean_segments 100
>>>>>>
>>>>>> # stop cleaning if more than 200 free segments
>>>>>> max_clean_segments 200
>>>>>>
>>>>>> # check free space once an hour
>>>>>> segment_check_interval 3600
>>>>>>
>>>>>> Basically in this example if less than 800mb are free cleaner is run
>>>>>> until 1600mb are free. If min_clean_segments is 0, the cleaner would do
>>>>>> normal operation.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> The first two parameters look Ok.
>>>>> (I've already referred to these in the above example.)
>>>>>
>>>>> We may well be able to make segment_check_interval more frequent.
>>>>> or do you have something in mind? 
>>>>>
>>>>> Do you mean interval of cleaning passes ?
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>>>> For this solution only changes in configfile loading and
>>>>>> nilfs_cleanerd_clean_loop would be necessary which would lower the risk
>>>>>> of introducing new bugs.
>>>>>>
>>>>>> If this solution is ok for you, I will implement it this way and send
>>>>>> you the patch in a few days. Also tell me if the names I have choosen
>>>>>> for the options are ok for you or if you would prefer other ones.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> The option names look fine to me.
>>>>> Or should we use percentage for them?
>>>>> (number of segments is device dependent)
>>>>>
>>>>> Is there anything else that isn't clear?
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>>>> Thanks in advance
>>>>>> Bye,
>>>>>> David Arendt
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> Thanks,
>>>>> Ryusuke Konishi 
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>>>> On 03/14/10 15:28, Ryusuke Konishi wrote:
>>>>>>     
>>>>>>         
>>>>>>             
>>>>>>> Hi,
>>>>>>> On Sun, 14 Mar 2010 14:00:19 +0100, admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org wrote:
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I will try to implement this myself then. Concerning the
>>>>>>>> nilfs_cleanerd_select segments function I was unclear in my post. In
>>>>>>>> fact I did not mean the return value but the first element from the
>>>>>>>> segnums array.
>>>>>>>>     
>>>>>>>>         
>>>>>>>>             
>>>>>>>>                 
>>>>>>> Ok. So you thought of determining termination of one cleaning pass by
>>>>>>> the segment number stored preliminarily.
>>>>>>>
>>>>>>> Why not just use count of processed (i.e. reclaimed) segments?
>>>>>>>
>>>>>>> Note that it's not guranteed that segments are selected in the order
>>>>>>> of segment number though this premise looks almost right.
>>>>>>>
>>>>>>> It depends on the behavior of segment allocator and the current
>>>>>>> "Select-oldest" algorithm used behind
>>>>>>> nilfs_cleanerd_select_segments().  Nilfs log writer occasionally
>>>>>>> behaves differently and disturbs this order.
>>>>>>>
>>>>>>> I think you can ignore the exceptional behavior of the segment
>>>>>>> allocator, and rotate target segments with skipping free or mostly
>>>>>>> in-use ones.  In that case, nilfs_cleanerd_select_segments() should be
>>>>>>> modified to select segments in the order of segment number.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Ryusuke Konishi
>>>>>>>
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> --
>>>>>> 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
>   


[-- Attachment #2: nilfs2-utils-min-free-segments.patch --]
[-- Type: text/plain, Size: 8665 bytes --]

diff -ur nilfs2-utils.orig/man/nilfs_cleanerd.conf.5 nilfs2-utils/man/nilfs_cleanerd.conf.5
--- nilfs2-utils.orig/man/nilfs_cleanerd.conf.5	2010-03-14 15:11:30.916690347 +0100
+++ nilfs2-utils/man/nilfs_cleanerd.conf.5	2010-03-15 22:15:58.320507660 +0100
@@ -25,6 +25,23 @@
 and their blocks whose duration time is less than the value.  The
 default value is 3600, meaning one hour.
 .TP
+.B min_clean_segments
+Specify the minimum number of clean segments. A value of 0 means
+normal cleaner operation. A value greater than 0 means pause cleaning
+until less than min_clean_segments are available. The default value
+is 100.
+.TP
+.B max_clean_segments
+Specify the maximum number of clean segments. If min_clean_segments is
+0, this value is ignored. If more than max_clean_segments are available
+cleaning is paused until less than min_clean_segments are available.
+The default value is 200.
+.TP
+.B clean_check_interval
+Specify the interval to wait between checks of min_clean_segments.
+If min_clean_segments is 0, this value is ignored.
+The default value is 60.
+.TP
 .B selection_policy
 Specify the GC policy. At present, only the `\fBtimestamp\fP' policy,
 which reclaims segments in order from oldest to newest, is support.
diff -ur nilfs2-utils.orig/sbin/cleanerd/cldconfig.c nilfs2-utils/sbin/cleanerd/cldconfig.c
--- nilfs2-utils.orig/sbin/cleanerd/cldconfig.c	2010-03-14 15:11:30.916690347 +0100
+++ nilfs2-utils/sbin/cleanerd/cldconfig.c	2010-03-15 20:20:22.364435601 +0100
@@ -90,6 +90,87 @@
 	return 0;
 }
 
+static int
+nilfs_cldconfig_handle_min_clean_segments(struct nilfs_cldconfig *config,
+					  char **tokens, size_t ntoks)
+{
+	__u64 n;
+	char *endptr;
+
+	if (check_tokens(tokens, ntoks, 2, 2) < 0)
+		return 0;
+
+	errno = 0;
+	n = strtoull(tokens[1], &endptr, 10);
+	if (*endptr != '\0') {
+		syslog(LOG_WARNING, "%s: %s: not a number",
+		       tokens[0], tokens[1]);
+		return 0;
+	}
+	if ((n == ULLONG_MAX) && (errno == ERANGE)) {
+		syslog(LOG_WARNING, "%s: %s: number too large",
+		       tokens[0], tokens[1]);
+		return 0;
+	}
+
+	config->cf_min_clean_segments = n;
+	return 0;
+}
+
+static int
+nilfs_cldconfig_handle_max_clean_segments(struct nilfs_cldconfig *config,
+					  char **tokens, size_t ntoks)
+{
+	__u64 n;
+	char *endptr;
+
+	if (check_tokens(tokens, ntoks, 2, 2) < 0)
+		return 0;
+
+	errno = 0;
+	n = strtoull(tokens[1], &endptr, 10);
+	if (*endptr != '\0') {
+		syslog(LOG_WARNING, "%s: %s: not a number",
+		       tokens[0], tokens[1]);
+		return 0;
+	}
+	if ((n == ULLONG_MAX) && (errno == ERANGE)) {
+		syslog(LOG_WARNING, "%s: %s: number too large",
+		       tokens[0], tokens[1]);
+		return 0;
+	}
+
+	config->cf_max_clean_segments = n;
+	return 0;
+}
+
+static int
+nilfs_cldconfig_handle_clean_check_interval(struct nilfs_cldconfig *config,
+					    char **tokens, size_t ntoks)
+{
+	time_t period;
+	char *endptr;
+
+	if (check_tokens(tokens, ntoks, 2, 2) < 0)
+		return 0;
+
+	errno = 0;
+	period = strtoul(tokens[1], &endptr, 10);
+	if (*endptr != '\0') {
+		syslog(LOG_WARNING, "%s: %s: not a number",
+		       tokens[0], tokens[1]);
+		return 0;
+	}
+	if ((period == ULONG_MAX) && (errno == ERANGE)) {
+		syslog(LOG_WARNING, "%s: %s: number too large",
+		       tokens[0], tokens[1]);
+		return 0;
+	}
+
+	config->cf_clean_check_interval = period;
+	return 0;
+}
+
 static unsigned long long
 nilfs_cldconfig_selection_policy_timestamp(const struct nilfs_suinfo *si)
 {
@@ -277,6 +358,9 @@
 static const struct nilfs_cldconfig_keyword
 nilfs_cldconfig_keyword_table[] = {
 	{"protection_period",	nilfs_cldconfig_handle_protection_period},
+	{"min_clean_segments",	nilfs_cldconfig_handle_min_clean_segments},
+	{"max_clean_segments",	nilfs_cldconfig_handle_max_clean_segments},
+	{"clean_check_interval",nilfs_cldconfig_handle_clean_check_interval},
 	{"selection_policy",	nilfs_cldconfig_handle_selection_policy},
 	{"nsegments_per_clean",	nilfs_cldconfig_handle_nsegments_per_clean},
 	{"cleaning_interval",	nilfs_cldconfig_handle_cleaning_interval},
@@ -313,6 +397,9 @@
 	config->cf_selection_policy.p_threshold =
 		NILFS_CLDCONFIG_SELECTION_POLICY_THRESHOLD;
 	config->cf_protection_period = NILFS_CLDCONFIG_PROTECTION_PERIOD;
+	config->cf_min_clean_segments = NILFS_CLDCONFIG_MIN_CLEAN_SEGMENTS;
+	config->cf_max_clean_segments = NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS;
+	config->cf_clean_check_interval = NILFS_CLDCONFIG_CLEAN_CHECK_INTERVAL;
 	config->cf_nsegments_per_clean = NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN;
 	config->cf_cleaning_interval = NILFS_CLDCONFIG_CLEANING_INTERVAL;
 	config->cf_retry_interval = NILFS_CLDCONFIG_RETRY_INTERVAL;
diff -ur nilfs2-utils.orig/sbin/cleanerd/cldconfig.h nilfs2-utils/sbin/cleanerd/cldconfig.h
--- nilfs2-utils.orig/sbin/cleanerd/cldconfig.h	2010-03-14 15:11:30.916690347 +0100
+++ nilfs2-utils/sbin/cleanerd/cldconfig.h	2010-03-15 21:58:16.208614224 +0100
@@ -42,6 +42,9 @@
  * struct nilfs_cldconfig -
  * @cf_selection_policy:
  * @cf_protection_period:
+ * @cf_min_clean_segments:
+ * @cf_max_clean_segments:
+ * @cf_clean_check_interval:
  * @cf_nsegments_per_clean
  * @cf_cleaning_interval:
  * @cf_use_mmap:
@@ -50,6 +53,9 @@
 struct nilfs_cldconfig {
 	struct nilfs_selection_policy cf_selection_policy;
 	time_t cf_protection_period;
+	__u64 cf_min_clean_segments;
+	__u64 cf_max_clean_segments;
+	time_t cf_clean_check_interval;
 	int cf_nsegments_per_clean;
 	time_t cf_cleaning_interval;
 	time_t cf_retry_interval;
@@ -61,6 +67,9 @@
 			nilfs_cldconfig_selection_policy_timestamp
 #define NILFS_CLDCONFIG_SELECTION_POLICY_THRESHOLD	0
 #define NILFS_CLDCONFIG_PROTECTION_PERIOD		3600
+#define	NILFS_CLDCONFIG_MIN_CLEAN_SEGMENTS		100
+#define	NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS		200
+#define	NILFS_CLDCONFIG_CLEAN_CHECK_INTERVAL		60
 #define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN		2
 #define NILFS_CLDCONFIG_CLEANING_INTERVAL		5
 #define NILFS_CLDCONFIG_RETRY_INTERVAL			60
diff -ur nilfs2-utils.orig/sbin/cleanerd/cleanerd.c nilfs2-utils/sbin/cleanerd/cleanerd.c
--- nilfs2-utils.orig/sbin/cleanerd/cleanerd.c	2010-03-14 15:11:30.916690347 +0100
+++ nilfs2-utils/sbin/cleanerd/cleanerd.c	2010-03-27 20:26:51.369060534 +0100
@@ -1163,6 +1163,21 @@
 	return 0;
 }
 
+static void nilfs_cleanerd_clean_check_pause(struct nilfs_cleanerd *cleanerd,
+					     struct timespec *timeout)
+{
+	cleanerd->c_running = 0;
+	timeout->tv_sec = cleanerd->c_config.cf_clean_check_interval;
+	timeout->tv_nsec = 0;
+	syslog(LOG_INFO, "pause (clean check)");
+}
+
+static void nilfs_cleanerd_clean_check_resume(struct nilfs_cleanerd *cleanerd)
+{
+	cleanerd->c_running = 1;
+	syslog(LOG_INFO, "resume (clean check)");
+}
+
 /**
  * nilfs_cleanerd_clean_loop - main loop of the cleaner daemon
  * @cleanerd: cleanerd object
@@ -1198,9 +1213,13 @@
 	if (ret < 0)
 		return -1;
 
-	cleanerd->c_running = 1;
 	cleanerd->c_ncleansegs = cleanerd->c_config.cf_nsegments_per_clean;
 
+	if (cleanerd->c_config.cf_min_clean_segments > 0)
+		nilfs_cleanerd_clean_check_pause(cleanerd, &timeout);
+	else
+		cleanerd->c_running = 1;
+
 	while (1) {
 		if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
 			syslog(LOG_ERR, "cannot set signal mask: %m");
@@ -1220,10 +1239,27 @@
 			syslog(LOG_ERR, "cannot get segment usage stat: %m");
 			return -1;
 		}
+
+		if (cleanerd->c_config.cf_min_clean_segments > 0) {
+			if (cleanerd->c_running) {
+				if (sustat.ss_ncleansegs > cleanerd->c_config.cf_max_clean_segments) {
+					nilfs_cleanerd_clean_check_pause(cleanerd, &timeout);
+					goto sleep;
+				}
+			}
+			else {
+				if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_clean_segments)
+					nilfs_cleanerd_clean_check_resume(cleanerd);
+				else
+					goto sleep;
+			}
+		}
+
 		if (sustat.ss_nongc_ctime != prev_nongc_ctime) {
 			cleanerd->c_running = 1;
 			prev_nongc_ctime = sustat.ss_nongc_ctime;
 		}
+
 		if (!cleanerd->c_running)
 			goto sleep;
 
diff -ur nilfs2-utils.orig/sbin/cleanerd/nilfs_cleanerd.conf nilfs2-utils/sbin/cleanerd/nilfs_cleanerd.conf
--- nilfs2-utils.orig/sbin/cleanerd/nilfs_cleanerd.conf	2010-03-14 15:11:30.916690347 +0100
+++ nilfs2-utils/sbin/cleanerd/nilfs_cleanerd.conf	2010-03-15 21:44:02.995587453 +0100
@@ -7,6 +7,17 @@
 # Protection period in second.
 protection_period	3600
 
+# Minium number of clean segements 
+# 0  = normal cleaner behaviour
+# >0 = start cleaning if less segments are available
+min_clean_segments	100
+
+# Maximum number of clean segments
+max_clean_segments	200
+
+# Clean segment check interval in seconds
+clean_check_interval	60
+
 # Segment selection policy.
 # In NILFS version 2.0.0, only the timestamp policy is supported.
 selection_policy	timestamp	# timestamp in ascend order