cleaner: run one cleaning pass based on minimum free space

cleaner: run one cleaning pass based on minimum free space

[David Arendt]

Hi,

In order to reduce cleaner io, I am thinking it could be usefull to
implement a parameter where you can specify the minimum free space. If
this parameter is set, instead of normal cleaning operation, the cleaner
would wait until there is less than minimum free space available and
then run one cleaning pass (from first to last segment). If after that,
there is again more than minimum free space available, continue waiting,
otherwise run cleaning passes until there is more than minimum free
space available.

What would you think about this idea ?

Bye,
David Arendt
--
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

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

[Ryusuke Konishi]

Hi,
On Sat, 13 Mar 2010 21:49:43 +0100, David Arendt wrote:
> Hi,
> 
> In order to reduce cleaner io, I am thinking it could be usefull to
> implement a parameter where you can specify the minimum free space. If
> this parameter is set, instead of normal cleaning operation, the cleaner
> would wait until there is less than minimum free space available and
> then run one cleaning pass (from first to last segment). If after that,
> there is again more than minimum free space available, continue waiting,
> otherwise run cleaning passes until there is more than minimum free
> space available.
> 
> What would you think about this idea ?
> 
> Bye,
> David Arendt

Well, I think this is one of what cleaner should take in.  It can
prevent cleanerd from moving in-use blocks too often unless the actual
free space is less than the threshold.

It may be the first thing to do since it's not difficult in principle.

I recognize there are more fundamental defects in the current cleaner:

 * It moves blocks in selected segments even if all of their blocks
   are in-use.

 * It doesn't give priority to reclaiming segments which have many
   obsolete blocks.

 * It keeps working without considering io workload of the time.

But, I'd rather take a quick fix like your proposal.

Regards,
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

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

[David Arendt]

Hi,

In order to avoid working both at the same thing, do you think about
implementing this yourself in near future or do you prefer that I try
implementing it myself and send you a patch ?

For the case where you prefer that I try implementing it, here a quick
information in natural language how I would implement this in order to
see if I am thinking it correctly (line beginning with *** are changed):

1166 /**
1167  * nilfs_cleanerd_clean_loop - main loop of the cleaner daemon
1168  * @cleanerd: cleanerd object
1169  */
1170 static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
1171 {
1172         struct nilfs_sustat sustat;
1173         __u64 prev_nongc_ctime = 0, prottime = 0, oldest = 0;
1174         __u64 segnums[NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX];

***            _u64 previousfirstsegnum = max value of _u64;

1175         struct timespec timeout;
1176         sigset_t sigset;
1177         int ns, ret;
1178
1179         sigemptyset(&sigset);
1180         if (sigprocmask(SIG_SETMASK, &sigset, NULL) < 0) {
1181                 syslog(LOG_ERR, "cannot set signal mask: %m");
1182                 return -1;
1183         }
1184         sigaddset(&sigset, SIGHUP);
1185
1186         if (set_sigterm_handler() < 0) {
1187                 syslog(LOG_ERR, "cannot set SIGTERM signal handler:
%m");
1188                 return -1;
1189         }
1190         if (set_sighup_handler() < 0) {
1191                 syslog(LOG_ERR, "cannot set SIGHUP signal handler:
%m");
1192                 return -1;
1193         }
1194
1195         nilfs_cleanerd_reload_config = 0;
1196
1197         ret = nilfs_cleanerd_init_interval(cleanerd);
1198         if (ret < 0)
1199                 return -1;
1200
1201         cleanerd->c_running = 1;
1202         cleanerd->c_ncleansegs =
cleanerd->c_config.cf_nsegments_per_clean;
1203
1204         while (1) {
1205                 if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
1206                         syslog(LOG_ERR, "cannot set signal mask: %m");
1207                         return -1;
1208                 }
1209
1210                 if (nilfs_cleanerd_reload_config) {
1211                         if (nilfs_cleanerd_reconfig(cleanerd)) {
1212                                 syslog(LOG_ERR, "cannot configure:
%m");
1213                                 return -1;
1214                         }
1215                         nilfs_cleanerd_reload_config = 0;
1216                         syslog(LOG_INFO, "configuration file
reloaded");
1217                 }
1218
1219                 if (nilfs_get_sustat(cleanerd->c_nilfs, &sustat) < 0) {
1220                         syslog(LOG_ERR, "cannot get segment usage
stat: %m");
1221                         return -1;
1222                 }
1223                 if (sustat.ss_nongc_ctime != prev_nongc_ctime) {
1224                         cleanerd->c_running = 1;
1225                         prev_nongc_ctime = sustat.ss_nongc_ctime;
1226                 }
1227                 if (!cleanerd->c_running)
1228                         goto sleep;
1229
1230                 syslog(LOG_DEBUG, "ncleansegs = %llu",
1231                        (unsigned long long)sustat.ss_ncleansegs);
1232
1233                 ns = nilfs_cleanerd_select_segments(
1234                         cleanerd, &sustat, segnums, &prottime,
&oldest);
1235                 if (ns < 0) {
1236                         syslog(LOG_ERR, "cannot select segments: %m");
1237                         return -1;
1238                 }

***                    if ((in_configfile_defined_treshold > 0) &&
(segnums[0] < previousfirstsegnum)) // one full pass finished or first pass
                         {
                            if more than in_configfile_defined_treshold
space available (defined in configfile)
                            {
                               set timout to
in_configfile_definied_checktime secs;
                               goto sleep;
                            }
                         }

                         previousfirstsegum = segnums[0];

1239                 syslog(LOG_DEBUG, "%d segment%s selected to be
cleaned",
1240                        ns, (ns <= 1) ? "" : "s");
1241                 if (ns > 0) {
1242                         ret = nilfs_cleanerd_clean_segments(
1243                                 cleanerd, &sustat, segnums, ns,
prottime);
1244                         if (ret < 0)
1245                                 return -1;
1246                 }
1247
1248                 ret = nilfs_cleanerd_recalc_interval(
1249                         cleanerd, ns, prottime, oldest, &timeout);
1250                 if (ret < 0)
1251                         return -1;
1252                 else if (ret > 0)
1253                         continue;
1254  sleep:
1255                 if (sigprocmask(SIG_UNBLOCK, &sigset, NULL) < 0) {
1256                         syslog(LOG_ERR, "cannot set signal mask: %m");
1257                         return -1;
1258                 }
1259
1260                 ret = nilfs_cleanerd_sleep(cleanerd, &timeout);
1261                 if (ret < 0)
1262                         return -1;
1263         }
1264 }

I suppose that calling nilfs_get_sustat and
nilfs_cleanerd_select_segments without actually cleaning wouldn't cause
problems and nilfs_cleanerd_select_segments would always return the
first segment equal or less than the one from last call or am I wrong
with this ? Is there a nilfs specific function I should use to determine
the free space available or should I use statfs ?

Thanks in advance,
Bye,
David Arendt

On 03/14/10 06:26, Ryusuke Konishi wrote:
> Hi,
> On Sat, 13 Mar 2010 21:49:43 +0100, David Arendt wrote:
>   
>> Hi,
>>
>> In order to reduce cleaner io, I am thinking it could be usefull to
>> implement a parameter where you can specify the minimum free space. If
>> this parameter is set, instead of normal cleaning operation, the cleaner
>> would wait until there is less than minimum free space available and
>> then run one cleaning pass (from first to last segment). If after that,
>> there is again more than minimum free space available, continue waiting,
>> otherwise run cleaning passes until there is more than minimum free
>> space available.
>>
>> What would you think about this idea ?
>>
>> Bye,
>> David Arendt
>>     
> Well, I think this is one of what cleaner should take in.  It can
> prevent cleanerd from moving in-use blocks too often unless the actual
> free space is less than the threshold.
>
> It may be the first thing to do since it's not difficult in principle.
>
> I recognize there are more fundamental defects in the current cleaner:
>
>  * It moves blocks in selected segments even if all of their blocks
>    are in-use.
>
>  * It doesn't give priority to reclaiming segments which have many
>    obsolete blocks.
>
>  * It keeps working without considering io workload of the time.
>
> But, I'd rather take a quick fix like your proposal.
>
> Regards,
> 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

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

[Ryusuke Konishi]

Hi,
On Sun, 14 Mar 2010 09:47:55 +0100, David Arendt wrote:
> Hi,
> 
> In order to avoid working both at the same thing, do you think about
> implementing this yourself in near future or do you prefer that I try
> implementing it myself and send you a patch ?

I'd appreciate it if you try it yourself.  I cannot commit time to
this at least for a few weeks.

> For the case where you prefer that I try implementing it, here a quick
> information in natural language how I would implement this in order to
> see if I am thinking it correctly (line beginning with *** are changed):
> 
> 1166 /**
> 1167  * nilfs_cleanerd_clean_loop - main loop of the cleaner daemon
> 1168  * @cleanerd: cleanerd object
> 1169  */
> 1170 static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
> 1171 {
> 1172         struct nilfs_sustat sustat;
> 1173         __u64 prev_nongc_ctime = 0, prottime = 0, oldest = 0;
> 1174         __u64 segnums[NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX];
> 
> ***            _u64 previousfirstsegnum = max value of _u64;
> 
> 1175         struct timespec timeout;
> 1176         sigset_t sigset;
> 1177         int ns, ret;
> 1178
> 1179         sigemptyset(&sigset);
> 1180         if (sigprocmask(SIG_SETMASK, &sigset, NULL) < 0) {
> 1181                 syslog(LOG_ERR, "cannot set signal mask: %m");
> 1182                 return -1;
> 1183         }
> 1184         sigaddset(&sigset, SIGHUP);
> 1185
> 1186         if (set_sigterm_handler() < 0) {
> 1187                 syslog(LOG_ERR, "cannot set SIGTERM signal handler:
> %m");
> 1188                 return -1;
> 1189         }
> 1190         if (set_sighup_handler() < 0) {
> 1191                 syslog(LOG_ERR, "cannot set SIGHUP signal handler:
> %m");
> 1192                 return -1;
> 1193         }
> 1194
> 1195         nilfs_cleanerd_reload_config = 0;
> 1196
> 1197         ret = nilfs_cleanerd_init_interval(cleanerd);
> 1198         if (ret < 0)
> 1199                 return -1;
> 1200
> 1201         cleanerd->c_running = 1;
> 1202         cleanerd->c_ncleansegs =
> cleanerd->c_config.cf_nsegments_per_clean;
> 1203
> 1204         while (1) {
> 1205                 if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
> 1206                         syslog(LOG_ERR, "cannot set signal mask: %m");
> 1207                         return -1;
> 1208                 }
> 1209
> 1210                 if (nilfs_cleanerd_reload_config) {
> 1211                         if (nilfs_cleanerd_reconfig(cleanerd)) {
> 1212                                 syslog(LOG_ERR, "cannot configure:
> %m");
> 1213                                 return -1;
> 1214                         }
> 1215                         nilfs_cleanerd_reload_config = 0;
> 1216                         syslog(LOG_INFO, "configuration file
> reloaded");
> 1217                 }
> 1218
> 1219                 if (nilfs_get_sustat(cleanerd->c_nilfs, &sustat) < 0) {
> 1220                         syslog(LOG_ERR, "cannot get segment usage
> stat: %m");
> 1221                         return -1;
> 1222                 }
> 1223                 if (sustat.ss_nongc_ctime != prev_nongc_ctime) {
> 1224                         cleanerd->c_running = 1;
> 1225                         prev_nongc_ctime = sustat.ss_nongc_ctime;
> 1226                 }
> 1227                 if (!cleanerd->c_running)
> 1228                         goto sleep;
> 1229
> 1230                 syslog(LOG_DEBUG, "ncleansegs = %llu",
> 1231                        (unsigned long long)sustat.ss_ncleansegs);
> 1232
> 1233                 ns = nilfs_cleanerd_select_segments(
> 1234                         cleanerd, &sustat, segnums, &prottime,
> &oldest);
> 1235                 if (ns < 0) {
> 1236                         syslog(LOG_ERR, "cannot select segments: %m");
> 1237                         return -1;
> 1238                 }
> 
> ***                    if ((in_configfile_defined_treshold > 0) &&
> (segnums[0] < previousfirstsegnum)) // one full pass finished or first pass
>                          {
>                             if more than in_configfile_defined_treshold
> space available (defined in configfile)
>                             {
>                                set timout to
> in_configfile_definied_checktime secs;
>                                goto sleep;
>                             }
>                          }
> 
>                          previousfirstsegum = segnums[0];
> 
> 1239                 syslog(LOG_DEBUG, "%d segment%s selected to be
> cleaned",
> 1240                        ns, (ns <= 1) ? "" : "s");
> 1241                 if (ns > 0) {
> 1242                         ret = nilfs_cleanerd_clean_segments(
> 1243                                 cleanerd, &sustat, segnums, ns,
> prottime);
> 1244                         if (ret < 0)
> 1245                                 return -1;
> 1246                 }
> 1247
> 1248                 ret = nilfs_cleanerd_recalc_interval(
> 1249                         cleanerd, ns, prottime, oldest, &timeout);
> 1250                 if (ret < 0)
> 1251                         return -1;
> 1252                 else if (ret > 0)
> 1253                         continue;
> 1254  sleep:
> 1255                 if (sigprocmask(SIG_UNBLOCK, &sigset, NULL) < 0) {
> 1256                         syslog(LOG_ERR, "cannot set signal mask: %m");
> 1257                         return -1;
> 1258                 }
> 1259
> 1260                 ret = nilfs_cleanerd_sleep(cleanerd, &timeout);
> 1261                 if (ret < 0)
> 1262                         return -1;
> 1263         }
> 1264 }
> 
> I suppose that calling nilfs_get_sustat and
> nilfs_cleanerd_select_segments without actually cleaning wouldn't cause
> problems

Yes, this is correct.

> and nilfs_cleanerd_select_segments would always return the
> first segment equal or less than the one from last call
> or am I wrong with this ?

Seems misunderstanding.

nilfs_cleanerd_select_segments returns the number of selected segments
for a shot of cleaning; if you set nsegments_per_clean = 2, this
function returns equal or less than two.

> Is there a nilfs specific function I should use to determine the
> free space available or should I use statfs ?

Actually, the free space reported by statfs is calculated from the
number of free segments.  So it's internally equivalent.

You can refer to sustat.ss_ncleansegs for the number of free segments
and sustat.ss_nsegs for the total number of segments.  I think
nilfs_get_sustat() is appropriate for your purpose because it's
already used in the loop function.

> Thanks in advance,
> Bye,
> David Arendt

With regards,
Ryusuke Konishi

 
> On 03/14/10 06:26, Ryusuke Konishi wrote:
> > Hi,
> > On Sat, 13 Mar 2010 21:49:43 +0100, David Arendt wrote:
> >   
> >> Hi,
> >>
> >> In order to reduce cleaner io, I am thinking it could be usefull to
> >> implement a parameter where you can specify the minimum free space. If
> >> this parameter is set, instead of normal cleaning operation, the cleaner
> >> would wait until there is less than minimum free space available and
> >> then run one cleaning pass (from first to last segment). If after that,
> >> there is again more than minimum free space available, continue waiting,
> >> otherwise run cleaning passes until there is more than minimum free
> >> space available.
> >>
> >> What would you think about this idea ?
> >>
> >> Bye,
> >> David Arendt
> >>     
> > Well, I think this is one of what cleaner should take in.  It can
> > prevent cleanerd from moving in-use blocks too often unless the actual
> > free space is less than the threshold.
> >
> > It may be the first thing to do since it's not difficult in principle.
> >
> > I recognize there are more fundamental defects in the current cleaner:
> >
> >  * It moves blocks in selected segments even if all of their blocks
> >    are in-use.
> >
> >  * It doesn't give priority to reclaiming segments which have many
> >    obsolete blocks.
> >
> >  * It keeps working without considering io workload of the time.
> >
> > But, I'd rather take a quick fix like your proposal.
> >
> > Regards,
> > 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

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

[admin-/LHdS3kC8BfYtjvyW6yDsg]

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.

Bye,
David Arendt

-original message-
Subject: Re: cleaner: run one cleaning pass based on minimum free space
From: Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org>
Date: 14/03/2010 12:59

Hi,
On Sun, 14 Mar 2010 09:47:55 +0100, David Arendt wrote:
> Hi,
> 
> In order to avoid working both at the same thing, do you think about
> implementing this yourself in near future or do you prefer that I try
> implementing it myself and send you a patch ?

I'd appreciate it if you try it yourself.  I cannot commit time to
this at least for a few weeks.

> For the case where you prefer that I try implementing it, here a quick
> information in natural language how I would implement this in order to
> see if I am thinking it correctly (line beginning with *** are changed):
> 
> 1166 /**
> 1167  * nilfs_cleanerd_clean_loop - main loop of the cleaner daemon
> 1168  * @cleanerd: cleanerd object
> 1169  */
> 1170 static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
> 1171 {
> 1172         struct nilfs_sustat sustat;
> 1173         __u64 prev_nongc_ctime = 0, prottime = 0, oldest = 0;
> 1174         __u64 segnums[NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX];
> 
> ***            _u64 previousfirstsegnum = max value of _u64;
> 
> 1175         struct timespec timeout;
> 1176         sigset_t sigset;
> 1177         int ns, ret;
> 1178
> 1179         sigemptyset(&sigset);
> 1180         if (sigprocmask(SIG_SETMASK, &sigset, NULL) < 0) {
> 1181                 syslog(LOG_ERR, "cannot set signal mask: %m");
> 1182                 return -1;
> 1183         }
> 1184         sigaddset(&sigset, SIGHUP);
> 1185
> 1186         if (set_sigterm_handler() < 0) {
> 1187                 syslog(LOG_ERR, "cannot set SIGTERM signal handler:
> %m");
> 1188                 return -1;
> 1189         }
> 1190         if (set_sighup_handler() < 0) {
> 1191                 syslog(LOG_ERR, "cannot set SIGHUP signal handler:
> %m");
> 1192                 return -1;
> 1193         }
> 1194
> 1195         nilfs_cleanerd_reload_config = 0;
> 1196
> 1197         ret = nilfs_cleanerd_init_interval(cleanerd);
> 1198         if (ret < 0)
> 1199                 return -1;
> 1200
> 1201         cleanerd->c_running = 1;
> 1202         cleanerd->c_ncleansegs =
> cleanerd->c_config.cf_nsegments_per_clean;
> 1203
> 1204         while (1) {
> 1205                 if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
> 1206                         syslog(LOG_ERR, "cannot set signal mask: %m");
> 1207                         return -1;
> 1208                 }
> 1209
> 1210                 if (nilfs_cleanerd_reload_config) {
> 1211                         if (nilfs_cleanerd_reconfig(cleanerd)) {
> 1212                                 syslog(LOG_ERR, "cannot configure:
> %m");
> 1213                                 return -1;
> 1214                         }
> 1215                         nilfs_cleanerd_reload_config = 0;
> 1216                         syslog(LOG_INFO, "configuration file
> reloaded");
> 1217                 }
> 1218
> 1219                 if (nilfs_get_sustat(cleanerd->c_nilfs, &sustat) < 0) {
> 1220                         syslog(LOG_ERR, "cannot get segment usage
> stat: %m");
> 1221                         return -1;
> 1222                 }
> 1223                 if (sustat.ss_nongc_ctime != prev_nongc_ctime) {
> 1224                         cleanerd->c_running = 1;
> 1225                         prev_nongc_ctime = sustat.ss_nongc_ctime;
> 1226                 }
> 1227                 if (!cleanerd->c_running)
> 1228                         goto sleep;
> 1229
> 1230                 syslog(LOG_DEBUG, "ncleansegs = %llu",
> 1231                        (unsigned long long)sustat.ss_ncleansegs);
> 1232
> 1233                 ns = nilfs_cleanerd_select_segments(
> 1234                         cleanerd, &sustat, segnums, &prottime,
> &oldest);
> 1235                 if (ns < 0) {
> 1236                         syslog(LOG_ERR, "cannot select segments: %m");
> 1237                         return -1;
> 1238                 }
> 
> ***                    if ((in_configfile_defined_treshold > 0) &&
> (segnums[0] < previousfirstsegnum)) // one full pass finished or first pass
>                          {
>                             if more than in_configfile_defined_treshold
> space available (defined in configfile)
>                             {
>                                set timout to
> in_configfile_definied_checktime secs;
>                                goto sleep;
>                             }
>                          }
> 
>                          previousfirstsegum = segnums[0];
> 
> 1239                 syslog(LOG_DEBUG, "%d segment%s selected to be
> cleaned",
> 1240                        ns, (ns <= 1) ? "" : "s");
> 1241                 if (ns > 0) {
> 1242                         ret = nilfs_cleanerd_clean_segments(
> 1243                                 cleanerd, &sustat, segnums, ns,
> prottime);
> 1244                         if (ret < 0)
> 1245                                 return -1;
> 1246                 }
> 1247
> 1248                 ret = nilfs_cleanerd_recalc_interval(
> 1249                         cleanerd, ns, prottime, oldest, &timeout);
> 1250                 if (ret < 0)
> 1251                         return -1;
> 1252                 else if (ret > 0)
> 1253                         continue;
> 1254  sleep:
> 1255                 if (sigprocmask(SIG_UNBLOCK, &sigset, NULL) < 0) {
> 1256                         syslog(LOG_ERR, "cannot set signal mask: %m");
> 1257                         return -1;
> 1258                 }
> 1259
> 1260                 ret = nilfs_cleanerd_sleep(cleanerd, &timeout);
> 1261                 if (ret < 0)
> 1262                         return -1;
> 1263         }
> 1264 }
> 
> I suppose that calling nilfs_get_sustat and
> nilfs_cleanerd_select_segments without actually cleaning wouldn't cause
> problems

Yes, this is correct.

> and nilfs_cleanerd_select_segments would always return the
> first segment equal or less than the one from last call
> or am I wrong with this ?

Seems misunderstanding.

nilfs_cleanerd_select_segments returns the number of selected segments
for a shot of cleaning; if you set nsegments_per_clean = 2, this
function returns equal or less than two.

> Is there a nilfs specific function I should use to determine the
> free space available or should I use statfs ?

Actually, the free space reported by statfs is calculated from the
number of free segments.  So it's internally equivalent.

You can refer to sustat.ss_ncleansegs for the number of free segments
and sustat.ss_nsegs for the total number of segments.  I think
nilfs_get_sustat() is appropriate for your purpose because it's
already used in the loop function.

> Thanks in advance,
> Bye,
> David Arendt

With regards,
Ryusuke Konishi

 
> On 03/14/10 06:26, Ryusuke Konishi wrote:
> > Hi,
> > On Sat, 13 Mar 2010 21:49:43 +0100, David Arendt wrote:
> >   
> >> Hi,
> >>
> >> In order to reduce cleaner io, I am thinking it could be usefull to
> >> implement a parameter where you can specify the minimum free space. If
> >> this parameter is set, instead of normal cleaning operation, the cleaner
> >> would wait until there is less than minimum free space available and
> >> then run one cleaning pass (from first to last segment). If after that,
> >> there is again more than minimum free space available, continue waiting,
> >> otherwise run cleaning passes until there is more than minimum free
> >> space available.
> >>
> >> What would you think about this idea ?
> >>
> >> Bye,
> >> David Arendt
> >>     
> > Well, I think this is one of what cleaner should take in.  It can
> > prevent cleanerd from moving in-use blocks too often unless the actual
> > free space is less than the threshold.
> >
> > It may be the first thing to do since it's not difficult in principle.
> >
> > I recognize there are more fundamental defects in the current cleaner:
> >
> >  * It moves blocks in selected segments even if all of their blocks
> >    are in-use.
> >
> >  * It doesn't give priority to reclaiming segments which have many
> >    obsolete blocks.
> >
> >  * It keeps working without considering io workload of the time.
> >
> > But, I'd rather take a quick fix like your proposal.
> >
> > Regards,
> > 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

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

[Ryusuke Konishi]

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

> Bye,
> David Arendt
> 
> -original message-
> Subject: Re: cleaner: run one cleaning pass based on minimum free space
> From: Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org>
> Date: 14/03/2010 12:59
> 
> Hi,
> On Sun, 14 Mar 2010 09:47:55 +0100, David Arendt wrote:
> > Hi,
> > 
> > In order to avoid working both at the same thing, do you think about
> > implementing this yourself in near future or do you prefer that I try
> > implementing it myself and send you a patch ?
> 
> I'd appreciate it if you try it yourself.  I cannot commit time to
> this at least for a few weeks.
> 
> > For the case where you prefer that I try implementing it, here a quick
> > information in natural language how I would implement this in order to
> > see if I am thinking it correctly (line beginning with *** are changed):
> > 
> > 1166 /**
> > 1167  * nilfs_cleanerd_clean_loop - main loop of the cleaner daemon
> > 1168  * @cleanerd: cleanerd object
> > 1169  */
> > 1170 static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
> > 1171 {
> > 1172         struct nilfs_sustat sustat;
> > 1173         __u64 prev_nongc_ctime = 0, prottime = 0, oldest = 0;
> > 1174         __u64 segnums[NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX];
> > 
> > ***            _u64 previousfirstsegnum = max value of _u64;
> > 
> > 1175         struct timespec timeout;
> > 1176         sigset_t sigset;
> > 1177         int ns, ret;
> > 1178
> > 1179         sigemptyset(&sigset);
> > 1180         if (sigprocmask(SIG_SETMASK, &sigset, NULL) < 0) {
> > 1181                 syslog(LOG_ERR, "cannot set signal mask: %m");
> > 1182                 return -1;
> > 1183         }
> > 1184         sigaddset(&sigset, SIGHUP);
> > 1185
> > 1186         if (set_sigterm_handler() < 0) {
> > 1187                 syslog(LOG_ERR, "cannot set SIGTERM signal handler:
> > %m");
> > 1188                 return -1;
> > 1189         }
> > 1190         if (set_sighup_handler() < 0) {
> > 1191                 syslog(LOG_ERR, "cannot set SIGHUP signal handler:
> > %m");
> > 1192                 return -1;
> > 1193         }
> > 1194
> > 1195         nilfs_cleanerd_reload_config = 0;
> > 1196
> > 1197         ret = nilfs_cleanerd_init_interval(cleanerd);
> > 1198         if (ret < 0)
> > 1199                 return -1;
> > 1200
> > 1201         cleanerd->c_running = 1;
> > 1202         cleanerd->c_ncleansegs =
> > cleanerd->c_config.cf_nsegments_per_clean;
> > 1203
> > 1204         while (1) {
> > 1205                 if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
> > 1206                         syslog(LOG_ERR, "cannot set signal mask: %m");
> > 1207                         return -1;
> > 1208                 }
> > 1209
> > 1210                 if (nilfs_cleanerd_reload_config) {
> > 1211                         if (nilfs_cleanerd_reconfig(cleanerd)) {
> > 1212                                 syslog(LOG_ERR, "cannot configure:
> > %m");
> > 1213                                 return -1;
> > 1214                         }
> > 1215                         nilfs_cleanerd_reload_config = 0;
> > 1216                         syslog(LOG_INFO, "configuration file
> > reloaded");
> > 1217                 }
> > 1218
> > 1219                 if (nilfs_get_sustat(cleanerd->c_nilfs, &sustat) < 0) {
> > 1220                         syslog(LOG_ERR, "cannot get segment usage
> > stat: %m");
> > 1221                         return -1;
> > 1222                 }
> > 1223                 if (sustat.ss_nongc_ctime != prev_nongc_ctime) {
> > 1224                         cleanerd->c_running = 1;
> > 1225                         prev_nongc_ctime = sustat.ss_nongc_ctime;
> > 1226                 }
> > 1227                 if (!cleanerd->c_running)
> > 1228                         goto sleep;
> > 1229
> > 1230                 syslog(LOG_DEBUG, "ncleansegs = %llu",
> > 1231                        (unsigned long long)sustat.ss_ncleansegs);
> > 1232
> > 1233                 ns = nilfs_cleanerd_select_segments(
> > 1234                         cleanerd, &sustat, segnums, &prottime,
> > &oldest);
> > 1235                 if (ns < 0) {
> > 1236                         syslog(LOG_ERR, "cannot select segments: %m");
> > 1237                         return -1;
> > 1238                 }
> > 
> > ***                    if ((in_configfile_defined_treshold > 0) &&
> > (segnums[0] < previousfirstsegnum)) // one full pass finished or first pass
> >                          {
> >                             if more than in_configfile_defined_treshold
> > space available (defined in configfile)
> >                             {
> >                                set timout to
> > in_configfile_definied_checktime secs;
> >                                goto sleep;
> >                             }
> >                          }
> > 
> >                          previousfirstsegum = segnums[0];
> > 
> > 1239                 syslog(LOG_DEBUG, "%d segment%s selected to be
> > cleaned",
> > 1240                        ns, (ns <= 1) ? "" : "s");
> > 1241                 if (ns > 0) {
> > 1242                         ret = nilfs_cleanerd_clean_segments(
> > 1243                                 cleanerd, &sustat, segnums, ns,
> > prottime);
> > 1244                         if (ret < 0)
> > 1245                                 return -1;
> > 1246                 }
> > 1247
> > 1248                 ret = nilfs_cleanerd_recalc_interval(
> > 1249                         cleanerd, ns, prottime, oldest, &timeout);
> > 1250                 if (ret < 0)
> > 1251                         return -1;
> > 1252                 else if (ret > 0)
> > 1253                         continue;
> > 1254  sleep:
> > 1255                 if (sigprocmask(SIG_UNBLOCK, &sigset, NULL) < 0) {
> > 1256                         syslog(LOG_ERR, "cannot set signal mask: %m");
> > 1257                         return -1;
> > 1258                 }
> > 1259
> > 1260                 ret = nilfs_cleanerd_sleep(cleanerd, &timeout);
> > 1261                 if (ret < 0)
> > 1262                         return -1;
> > 1263         }
> > 1264 }
> > 
> > I suppose that calling nilfs_get_sustat and
> > nilfs_cleanerd_select_segments without actually cleaning wouldn't cause
> > problems
> 
> Yes, this is correct.
> 
> > and nilfs_cleanerd_select_segments would always return the
> > first segment equal or less than the one from last call
> > or am I wrong with this ?
> 
> Seems misunderstanding.
> 
> nilfs_cleanerd_select_segments returns the number of selected segments
> for a shot of cleaning; if you set nsegments_per_clean = 2, this
> function returns equal or less than two.
> 
> > Is there a nilfs specific function I should use to determine the
> > free space available or should I use statfs ?
> 
> Actually, the free space reported by statfs is calculated from the
> number of free segments.  So it's internally equivalent.
> 
> You can refer to sustat.ss_ncleansegs for the number of free segments
> and sustat.ss_nsegs for the total number of segments.  I think
> nilfs_get_sustat() is appropriate for your purpose because it's
> already used in the loop function.
> 
> > Thanks in advance,
> > Bye,
> > David Arendt
> 
> With regards,
> Ryusuke Konishi
> 
>  
> > On 03/14/10 06:26, Ryusuke Konishi wrote:
> > > Hi,
> > > On Sat, 13 Mar 2010 21:49:43 +0100, David Arendt wrote:
> > >   
> > >> Hi,
> > >>
> > >> In order to reduce cleaner io, I am thinking it could be usefull to
> > >> implement a parameter where you can specify the minimum free space. If
> > >> this parameter is set, instead of normal cleaning operation, the cleaner
> > >> would wait until there is less than minimum free space available and
> > >> then run one cleaning pass (from first to last segment). If after that,
> > >> there is again more than minimum free space available, continue waiting,
> > >> otherwise run cleaning passes until there is more than minimum free
> > >> space available.
> > >>
> > >> What would you think about this idea ?
> > >>
> > >> Bye,
> > >> David Arendt
> > >>     
> > > Well, I think this is one of what cleaner should take in.  It can
> > > prevent cleanerd from moving in-use blocks too often unless the actual
> > > free space is less than the threshold.
> > >
> > > It may be the first thing to do since it's not difficult in principle.
> > >
> > > I recognize there are more fundamental defects in the current cleaner:
> > >
> > >  * It moves blocks in selected segments even if all of their blocks
> > >    are in-use.
> > >
> > >  * It doesn't give priority to reclaiming segments which have many
> > >    obsolete blocks.
> > >
> > >  * It keeps working without considering io workload of the time.
> > >
> > > But, I'd rather take a quick fix like your proposal.
> > >
> > > Regards,
> > > 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
--
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

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

[David Arendt]

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 ?

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.

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.

Thanks in advance
Bye,
David Arendt


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

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

[Ryusuke Konishi]

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

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

[David Arendt]

Hi,

In fact by segment check interval I mean the time to sleep before
checking again for free space. I used 3600 in the example as this is
would be suitable for my workload, but 60 might be a safer default value.

Specifying a percentage would also be an idea. I thought about segments
as nsegments_per_clean is also referring to segments. I think I will
start implementing this now using segments, as changing it to percent
wouldn't be big changes. Another idea that comes me to mind would be
that specifying for example 10 in any configfile option accepting
segments would be 10 segments and 10% ten percent of total segments.

For the rest, I think everything should be clear and I should have a
patch ready in a few days.

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

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

[David Arendt]

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

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.

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
>>     


[-- Attachment #2: nilfs2-utils-min-free-segments.patch --]
[-- Type: text/plain, Size: 8382 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-15 21:41:32.956273102 +0100
@@ -1175,6 +1175,7 @@
 	struct timespec timeout;
 	sigset_t sigset;
 	int ns, ret;
+	int sleeping = 1;
 
 	sigemptyset(&sigset);
 	if (sigprocmask(SIG_SETMASK, &sigset, NULL) < 0) {
@@ -1201,6 +1202,12 @@
 	cleanerd->c_running = 1;
 	cleanerd->c_ncleansegs = cleanerd->c_config.cf_nsegments_per_clean;
 
+	if (cleanerd->c_config.cf_min_clean_segments > 0) {
+		syslog(LOG_INFO, "cleaner paused");
+		timeout.tv_sec = cleanerd->c_config.cf_clean_check_interval;
+		timeout.tv_nsec = 0;
+	}
+
 	while (1) {
 		if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
 			syslog(LOG_ERR, "cannot set signal mask: %m");
@@ -1220,10 +1227,32 @@
 			syslog(LOG_ERR, "cannot get segment usage stat: %m");
 			return -1;
 		}
+
+		if (cleanerd->c_config.cf_min_clean_segments > 0) {
+			if (sleeping) {
+				if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_clean_segments) {
+					syslog(LOG_INFO, "cleaner resumed");
+					sleeping = 0;
+				}
+				else
+					goto sleep;
+			}
+			else {
+				if (sustat.ss_ncleansegs > cleanerd->c_config.cf_max_clean_segments) {
+					syslog(LOG_INFO, "cleaner paused");
+					sleeping = 1;
+					timeout.tv_sec = cleanerd->c_config.cf_clean_check_interval;
+					timeout.tv_nsec = 0;
+					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

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

[admin-/LHdS3kC8BfYtjvyW6yDsg]

Hi,

if it is ok for you, I will create a second patch to add the following
mount options: minfree, maxfree (or do you prefer other names ?). So
different values can be specified for different mount points.

What do you think ?

Thanks,
Arendt David

> 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

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

[Ryusuke Konishi]

Hi,
On Mon, 15 Mar 2010 18:09:38 +0100, David Arendt wrote:
> Hi,
> 
> In fact by segment check interval I mean the time to sleep before
> checking again for free space. I used 3600 in the example as this is
> would be suitable for my workload, but 60 might be a safer default value.
> 
> Specifying a percentage would also be an idea. I thought about segments
> as nsegments_per_clean is also referring to segments. I think I will
> start implementing this now using segments, as changing it to percent
> wouldn't be big changes. Another idea that comes me to mind would be
> that specifying for example 10 in any configfile option accepting
> segments would be 10 segments and 10% ten percent of total segments.

Sounds nice.

As for nsegments_per_clean, seems like the percentage notation should
not be applied because it has a bad effect on memory usage of the
nilfs GC cache in kernel. (So, the parameter is limited by a ceiling.)

But, I agree that the notation is suited for such threshold
parameters.

Regards,
Ryusuke Konishi

> For the rest, I think everything should be clear and I should have a
> patch ready in a few days.
> 
> 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

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

[Ryusuke Konishi]

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

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

[Ryusuke Konishi]

On Tue, 16 Mar 2010 12:17:53 +0100, admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org wrote:
> Hi,
> 
> if it is ok for you, I will create a second patch to add the following
> mount options: minfree, maxfree (or do you prefer other names ?). So
> different values can be specified for different mount points.
> 
> What do you think ?

Hmm, I'd like to keep such pseudo mount option to a minimum necessary.

Previous -pp option was adopted since it was helpful for emergency
use, for dead-end situations like a disk full.  Do you think the same
is true of these parameters ?

Thanks,
Ryusuke Konishi

> Thanks,
> Arendt David
> 
> > 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
--
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

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

[David Arendt]

Hi,

I have changed cleanerd.c to use cleanerd->c_running instead of sleeping.

Here the changed function for review. I will post a new complete patch
after receiving your comments.

static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
{
    struct nilfs_sustat sustat;
    __u64 prev_nongc_ctime = 0, prottime = 0, oldest = 0;
    __u64 segnums[NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX];
    struct timespec timeout;
    sigset_t sigset;
    int ns, ret;

    sigemptyset(&sigset);
    if (sigprocmask(SIG_SETMASK, &sigset, NULL) < 0) {
        syslog(LOG_ERR, "cannot set signal mask: %m");
        return -1;
    }
    sigaddset(&sigset, SIGHUP);

    if (set_sigterm_handler() < 0) {
        syslog(LOG_ERR, "cannot set SIGTERM signal handler: %m");
        return -1;
    }
    if (set_sighup_handler() < 0) {
        syslog(LOG_ERR, "cannot set SIGHUP signal handler: %m");
        return -1;
    }

    nilfs_cleanerd_reload_config = 0;

    ret = nilfs_cleanerd_init_interval(cleanerd);
    if (ret < 0)
        return -1;

    cleanerd->c_ncleansegs = cleanerd->c_config.cf_nsegments_per_clean;

    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;

    while (1) {
        if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
            syslog(LOG_ERR, "cannot set signal mask: %m");
            return -1;
        }

        if (nilfs_cleanerd_reload_config) {
            if (nilfs_cleanerd_reconfig(cleanerd)) {
                syslog(LOG_ERR, "cannot configure: %m");
                return -1;
            }
            nilfs_cleanerd_reload_config = 0;
            syslog(LOG_INFO, "configuration file reloaded");
        }

        if (nilfs_get_sustat(cleanerd->c_nilfs, &sustat) < 0) {
            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) {
                    syslog(LOG_INFO, "cleaner paused");
                    cleanerd->c_running = 0;
                    timeout.tv_sec =
cleanerd->c_config.cf_clean_check_interval;
                    timeout.tv_nsec = 0;
                    goto sleep;
                }
            }
            else {
                if (sustat.ss_ncleansegs <
cleanerd->c_config.cf_min_clean_segments) {
                    syslog(LOG_INFO, "cleaner resumed");
                    cleanerd->c_running = 1;
                }
                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;

        syslog(LOG_DEBUG, "ncleansegs = %llu",
               (unsigned long long)sustat.ss_ncleansegs);

        ns = nilfs_cleanerd_select_segments(
            cleanerd, &sustat, segnums, &prottime, &oldest);
        if (ns < 0) {
            syslog(LOG_ERR, "cannot select segments: %m");
            return -1;
        }
        syslog(LOG_DEBUG, "%d segment%s selected to be cleaned",
               ns, (ns <= 1) ? "" : "s");
        if (ns > 0) {
            ret = nilfs_cleanerd_clean_segments(
                cleanerd, &sustat, segnums, ns, prottime);
            if (ret < 0)
                return -1;
        }

        ret = nilfs_cleanerd_recalc_interval(
            cleanerd, ns, prottime, oldest, &timeout);
        if (ret < 0)
            return -1;
        else if (ret > 0)
            continue;
 sleep:
        if (sigprocmask(SIG_UNBLOCK, &sigset, NULL) < 0) {
            syslog(LOG_ERR, "cannot set signal mask: %m");
            return -1;
        }

        ret = nilfs_cleanerd_sleep(cleanerd, &timeout);
        if (ret < 0)
            return -1;
    }
}

Thanks in advance
Bye,
David Arendt



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

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

[David Arendt]

Hi,

Well I think this parameters never need to be changed as emergency.

However I can imagine that for some situations, it would be useful to
have different parameters for different drives when file systems are
mounted from /etc/fstab.

There was a proposal of another person to modify the configuration file
to handle multiple devices which might be a better solution.

Another solution migh be to leave the configuration file as it is and
add a mount option to specify which configuration file to use.

Bye,
David Arendt

On 03/17/10 19:32, Ryusuke Konishi wrote:
> On Tue, 16 Mar 2010 12:17:53 +0100, admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org wrote:
>   
>> Hi,
>>
>> if it is ok for you, I will create a second patch to add the following
>> mount options: minfree, maxfree (or do you prefer other names ?). So
>> different values can be specified for different mount points.
>>
>> What do you think ?
>>     
> Hmm, I'd like to keep such pseudo mount option to a minimum necessary.
>
> Previous -pp option was adopted since it was helpful for emergency
> use, for dead-end situations like a disk full.  Do you think the same
> is true of these parameters ?
>
> Thanks,
> Ryusuke Konishi
>
>   
>> Thanks,
>> Arendt David
>>
>>     
>>> 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
>>     

--
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

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

[David Arendt]

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

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

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
>>>>     
>>>>         
>>     


[-- Attachment #2: nilfs2-utils-min-free-segments.patch --]
[-- Type: text/plain, Size: 8328 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-17 19:59:36.845402863 +0100
@@ -1198,9 +1198,17 @@
 	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) {
+		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;
+
 	while (1) {
 		if (sigprocmask(SIG_BLOCK, &sigset, NULL) < 0) {
 			syslog(LOG_ERR, "cannot set signal mask: %m");
@@ -1220,10 +1228,32 @@
 			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) {
+					syslog(LOG_INFO, "cleaner paused");
+					cleanerd->c_running = 0;
+					timeout.tv_sec = cleanerd->c_config.cf_clean_check_interval;
+					timeout.tv_nsec = 0;
+					goto sleep;
+				}
+			}
+			else {
+				if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_clean_segments) {
+					syslog(LOG_INFO, "cleaner resumed");
+					cleanerd->c_running = 1;
+				}
+				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

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

[Ryusuke Konishi]

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

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

[David Arendt]

Hi,

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;
>
>   

Well, the cleanerd->c_running=0 would not be needed, but I thought the
code would be more understandable if putting it once again here. As I
start in paused state, I thought it would also be good to log this
paused state. The time initialisation is done here as otherwise it would
have always to be done inside the loop using a very very little bit more
resources. It think it could be better readable if I would make to
functions pause and resume and calling the respective functions. I will
make a try and send you the revised patch, so you can tell me what you
think about it.
> 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
>
>   
Bye,
David Arendt
>> 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
>   

--
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

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

[David Arendt]

[-- 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

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

[Ryusuke Konishi]

Hi,

On Sat, 27 Mar 2010 21:00:52 +0100, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> Hi,
> 
> here the revised version of the patch
> 
> As changelog description we could put:
> 
> add options for cleaning based on number of free segments

Thanks.

Ok, it looks fine to me.
 
> 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 ?

Agreed.

Maybe name of the mount option should be "nocleaner" or "nogc" because
"nocleanerd" implies how it is implemented.

> Anyway I think this should be part of a second patch as it is
> implementing different functionality.

Yes, it should be separate from the first one.

Thanks,
Ryusuke Konishi

> 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
> >   
> 
--
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

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

[David Arendt]

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

Hi,

here the nogc patch

As changelog description for this one, we could put:

add mount option to disable garbage collection

Thanks in advance
Bye,
David Arendt

On 03/28/10 03:55, Ryusuke Konishi wrote:
> Hi,
>
> On Sat, 27 Mar 2010 21:00:52 +0100, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>   
>> Hi,
>>
>> here the revised version of the patch
>>
>> As changelog description we could put:
>>
>> add options for cleaning based on number of free segments
>>     
> Thanks.
>
> Ok, it looks fine to me.
>  
>   
>> 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 ?
>>     
> Agreed.
>
> Maybe name of the mount option should be "nocleaner" or "nogc" because
> "nocleanerd" implies how it is implemented.
>
>   
>> Anyway I think this should be part of a second patch as it is
>> implementing different functionality.
>>     
> Yes, it should be separate from the first one.
>
> Thanks,
> Ryusuke Konishi
>
>   
>> 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-nogc.patch --]
[-- Type: text/plain, Size: 3338 bytes --]

diff -ur nilfs2-utils.orig/man/mount.nilfs2.8 nilfs2-utils/man/mount.nilfs2.8
--- nilfs2-utils.orig/man/mount.nilfs2.8	2010-03-14 15:11:30.916690347 +0100
+++ nilfs2-utils/man/mount.nilfs2.8	2010-03-28 12:16:49.785942470 +0200
@@ -108,6 +108,11 @@
 elapsed time from its creation is smaller than
 \fIprotection-period\fP.
 .TP
+.BR nogc
+Disable garbage collection. The cleaner daemon will not be started.
+It can be be started manually, but in that case it must also be
+stopped manually before unmounting.
+.TP
 .BR order=relaxed " / " order=strict
 Specify order semantics for file data.  Metadata is always written to
 follow the POSIX semantics about the order of filesystem operations.
diff -ur nilfs2-utils.orig/sbin/mount/mount.nilfs2.c nilfs2-utils/sbin/mount/mount.nilfs2.c
--- nilfs2-utils.orig/sbin/mount/mount.nilfs2.c	2010-03-14 15:11:30.918691251 +0100
+++ nilfs2-utils/sbin/mount/mount.nilfs2.c	2010-03-28 14:05:28.861362988 +0200
@@ -74,6 +74,9 @@
 const char pp_opt_fmt[] = PPOPT_NAME "=%lu";
 typedef unsigned long pp_opt_t;
 
+const char nogc_opt_fmt[] = NOGCOPT_NAME;
+typedef int nogc_opt_t;
+
 struct mount_options {
 	char *fstype;
 	char *opts;
@@ -329,6 +332,7 @@
 	int type;
 	int mounted;
 	pp_opt_t protperiod;
+	nogc_opt_t nogc;
 };
 
 static int check_mtab(void)
@@ -391,6 +395,8 @@
 	if (find_opt(mc->m.mnt_opts, pp_opt_fmt, &prot_period) >= 0)
 		mi->protperiod = prot_period;
 
+	mi->nogc = (find_opt(mc->m.mnt_opts, nogc_opt_fmt, NULL) >= 0);
+
 	switch (mo->flags & (MS_RDONLY | MS_REMOUNT)) {
 	case 0: /* overlapping rw-mount */
 		error(_("%s: the device already has a rw-mount on %s."
@@ -426,11 +432,13 @@
 static int
 do_mount_one(struct nilfs_mount_info *mi, const struct mount_options *mo)
 {
-	int res, errsv;
-	char *exopts;
+	int res, errsv, mtab_ok;
+	char *tmpexopts, *exopts;
 	pp_opt_t prot_period;
 
-	exopts = change_opt(mo->extra_opts, pp_opt_fmt, &prot_period, "");
+	tmpexopts = change_opt(mo->extra_opts, pp_opt_fmt, &prot_period, "");
+	exopts = change_opt(tmpexopts, nogc_opt_fmt, NULL, "");
+	my_free(tmpexopts);
 
 	res = mount(mi->device, mi->mntdir, fstype, mo->flags & ~MS_NOSYS,
 		    exopts);
@@ -450,9 +458,12 @@
 	}
 	if (mi->type != RW2RO_REMOUNT && mi->type != RW2RW_REMOUNT)
 		goto out;
+
+        mtab_ok = check_mtab();
+
 	/* Cleaner daemon was stopped and it needs to run */
 	/* because filesystem is still mounted */
-	if (check_mtab()) {
+	if (!mi->nogc && mtab_ok) {
 		/* Restarting cleaner daemon */
 		if (start_cleanerd(mi->device, mi->mntdir, mi->protperiod,
 				   &mi->gcpid) == 0) {
@@ -481,7 +492,7 @@
 	char *exopts;
 	int rungc;
 
-	rungc = !(mo->flags & MS_RDONLY) && !(mo->flags & MS_BIND);
+	rungc = (find_opt(mo->extra_opts, nogc_opt_fmt, NULL) < 0) && !(mo->flags & MS_RDONLY) && !(mo->flags & MS_BIND);
 
 	if (!check_mtab()) {
 		if (rungc)
diff -ur nilfs2-utils.orig/sbin/mount/mount.nilfs2.h nilfs2-utils/sbin/mount/mount.nilfs2.h
--- nilfs2-utils.orig/sbin/mount/mount.nilfs2.h	2010-03-14 15:11:30.918691251 +0100
+++ nilfs2-utils/sbin/mount/mount.nilfs2.h	2010-03-28 11:05:38.717647856 +0200
@@ -14,6 +14,7 @@
 #define CLEANERD_NAME		"nilfs_cleanerd"
 #define PIDOPT_NAME		"gcpid"
 #define PPOPT_NAME		"pp"
+#define NOGCOPT_NAME		"nogc"
 
 #define CLEANERD_WAIT_RETRY_COUNT	3
 #define CLEANERD_WAIT_RETRY_INTERVAL	2  /* in seconds */

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

[Ryusuke Konishi]

Hi,
On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> Hi,
> 
> here the nogc patch
> 
> As changelog description for this one, we could put:
> 
> add mount option to disable garbage collection
> 
> Thanks in advance
> Bye,
> David Arendt

Hmm, the patch looks perfect.

Will queue both in the git tree of utils.

Thanks,
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

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

[David Arendt]

Hi,

thanks for applying the patches. I did all my tests on 2 gbyte loop
devices and now that it is officially in git, I deployed it to some
production systems with big disks. Here I have noticed, that I have
completely forgotten the reserved segments. Technically this is not a
problem, but I think people changing configuration files will tend to
forget about it. I'm thinking it might be useful to add them internally
to min_free_segments and max_free_segments so users don't need to worry
about them. What do you think ? If you like to change the current
behavior to this behavior, I will submit a short update patch.

I am thinking about getting the number of reserved segments this way:

(nilfs_cleanerd->c_nilfs->n_sb->s_nsegments *
nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) / 100

or do you know any better way ?

Thanks in advance,
David Arendt

On 03/28/10 17:26, Ryusuke Konishi wrote:
> Hi,
> On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>   
>> Hi,
>>
>> here the nogc patch
>>
>> As changelog description for this one, we could put:
>>
>> add mount option to disable garbage collection
>>
>> Thanks in advance
>> Bye,
>> David Arendt
>>     
> Hmm, the patch looks perfect.
>
> Will queue both in the git tree of utils.
>
> Thanks,
> 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

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

[Ryusuke Konishi]

Hi,
On Sun, 28 Mar 2010 23:52:52 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> Hi,
> 
> thanks for applying the patches. I did all my tests on 2 gbyte loop
> devices and now that it is officially in git, I deployed it to some
> production systems with big disks. Here I have noticed, that I have
> completely forgotten the reserved segments. Technically this is not a
> problem, but I think people changing configuration files will tend to
> forget about it. I'm thinking it might be useful to add them internally
> to min_free_segments and max_free_segments so users don't need to worry
> about them. What do you think ?

Ahh, we should take into account the number of reserved segments.  If
not so, cleaner control with the two threshold values will not work
properly for large drives.

> If you like to change the current behavior to this behavior, I will
> submit a short update patch.

Yes, please do.

> I am thinking about getting the number of reserved segments this way:
> 
> (nilfs_cleanerd->c_nilfs->n_sb->s_nsegments *
> nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) / 100
> 
> or do you know any better way ?

The kernel code calulates the number by:

  = max(NILFS_MIN_NRSVSEGS,
        DIV_ROUND_UP(nsegments * r_segments_percentage, 100))

  where NILFS_MIN_NRSVSEGS is defined in include/nilfs2_fs.h, and
  DIV_ROUND_UP is defined as follows:

 #define DIV_ROUND_UP(n,d)    (((n) +  (d) - 1) / (d))

The same or some equivelent calculation seems preferable.
 
With regards,
Ryusuke Konishi

> On 03/28/10 17:26, Ryusuke Konishi wrote:
> > Hi,
> > On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >   
> >> Hi,
> >>
> >> here the nogc patch
> >>
> >> As changelog description for this one, we could put:
> >>
> >> add mount option to disable garbage collection
> >>
> >> Thanks in advance
> >> Bye,
> >> David Arendt
> >>     
> > Hmm, the patch looks perfect.
> >
> > Will queue both in the git tree of utils.
> >
> > Thanks,
> > 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

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

[David Arendt]

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

Hi,

here the changes

Thank in advance,
David Arendt

On 03/29/10 05:59, Ryusuke Konishi wrote:
> Hi,
> On Sun, 28 Mar 2010 23:52:52 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>   
>> Hi,
>>
>> thanks for applying the patches. I did all my tests on 2 gbyte loop
>> devices and now that it is officially in git, I deployed it to some
>> production systems with big disks. Here I have noticed, that I have
>> completely forgotten the reserved segments. Technically this is not a
>> problem, but I think people changing configuration files will tend to
>> forget about it. I'm thinking it might be useful to add them internally
>> to min_free_segments and max_free_segments so users don't need to worry
>> about them. What do you think ?
>>     
> Ahh, we should take into account the number of reserved segments.  If
> not so, cleaner control with the two threshold values will not work
> properly for large drives.
>
>   
>> If you like to change the current behavior to this behavior, I will
>> submit a short update patch.
>>     
> Yes, please do.
>
>   
>> I am thinking about getting the number of reserved segments this way:
>>
>> (nilfs_cleanerd->c_nilfs->n_sb->s_nsegments *
>> nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) / 100
>>
>> or do you know any better way ?
>>     
> The kernel code calulates the number by:
>
>   = max(NILFS_MIN_NRSVSEGS,
>         DIV_ROUND_UP(nsegments * r_segments_percentage, 100))
>
>   where NILFS_MIN_NRSVSEGS is defined in include/nilfs2_fs.h, and
>   DIV_ROUND_UP is defined as follows:
>
>  #define DIV_ROUND_UP(n,d)    (((n) +  (d) - 1) / (d))
>
> The same or some equivelent calculation seems preferable.
>  
> With regards,
> Ryusuke Konishi
>
>   
>> On 03/28/10 17:26, Ryusuke Konishi wrote:
>>     
>>> Hi,
>>> On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>>>   
>>>       
>>>> Hi,
>>>>
>>>> here the nogc patch
>>>>
>>>> As changelog description for this one, we could put:
>>>>
>>>> add mount option to disable garbage collection
>>>>
>>>> Thanks in advance
>>>> Bye,
>>>> David Arendt
>>>>     
>>>>         
>>> Hmm, the patch looks perfect.
>>>
>>> Will queue both in the git tree of utils.
>>>
>>> Thanks,
>>> Ryusuke Konishi
>>>   
>>>       
>>     


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

diff -ur nilfs2-utils.orig/sbin/cleanerd/cleanerd.c nilfs2-utils/sbin/cleanerd/cleanerd.c
--- nilfs2-utils.orig/sbin/cleanerd/cleanerd.c	2010-03-29 06:05:51.382126765 +0200
+++ nilfs2-utils/sbin/cleanerd/cleanerd.c	2010-03-29 06:32:09.129775882 +0200
@@ -1185,7 +1185,7 @@
 static int nilfs_cleanerd_clean_loop(struct nilfs_cleanerd *cleanerd)
 {
 	struct nilfs_sustat sustat;
-	__u64 prev_nongc_ctime = 0, prottime = 0, oldest = 0;
+	__u64 r_segments, prev_nongc_ctime = 0, prottime = 0, oldest = 0;
 	__u64 segnums[NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX];
 	struct timespec timeout;
 	sigset_t sigset;
@@ -1215,6 +1215,10 @@
 
 	cleanerd->c_ncleansegs = cleanerd->c_config.cf_nsegments_per_clean;
 
+	r_segments = ((nilfs_cleanerd->c_nilfs->n_sb->s_nsegments * nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) + 99) / 100;
+	if (r_segments < NILFS_MIN_NRSVSEGS)
+		r_segments = NILFS_MIN_NRSVSEGS;
+
 	if (cleanerd->c_config.cf_min_clean_segments > 0)
 		nilfs_cleanerd_clean_check_pause(cleanerd, &timeout);
 	else
@@ -1242,13 +1246,13 @@
 
 		if (cleanerd->c_config.cf_min_clean_segments > 0) {
 			if (cleanerd->c_running) {
-				if (sustat.ss_ncleansegs > cleanerd->c_config.cf_max_clean_segments) {
+				if (sustat.ss_ncleansegs > cleanerd->c_config.cf_max_clean_segments + r_segments) {
 					nilfs_cleanerd_clean_check_pause(cleanerd, &timeout);
 					goto sleep;
 				}
 			}
 			else {
-				if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_clean_segments)
+				if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_clean_segments + r_segments)
 					nilfs_cleanerd_clean_check_resume(cleanerd);
 				else
 					goto sleep;

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

[Ryusuke Konishi]

On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> Hi,
> 
> here the changes
> 
> Thank in advance,
> David Arendt

Looks fine to me.  Will apply later.

Thanks for your quick work.

Ryusuke Konishi
 
> On 03/29/10 05:59, Ryusuke Konishi wrote:
> > Hi,
> > On Sun, 28 Mar 2010 23:52:52 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >   
> >> Hi,
> >>
> >> thanks for applying the patches. I did all my tests on 2 gbyte loop
> >> devices and now that it is officially in git, I deployed it to some
> >> production systems with big disks. Here I have noticed, that I have
> >> completely forgotten the reserved segments. Technically this is not a
> >> problem, but I think people changing configuration files will tend to
> >> forget about it. I'm thinking it might be useful to add them internally
> >> to min_free_segments and max_free_segments so users don't need to worry
> >> about them. What do you think ?
> >>     
> > Ahh, we should take into account the number of reserved segments.  If
> > not so, cleaner control with the two threshold values will not work
> > properly for large drives.
> >
> >   
> >> If you like to change the current behavior to this behavior, I will
> >> submit a short update patch.
> >>     
> > Yes, please do.
> >
> >   
> >> I am thinking about getting the number of reserved segments this way:
> >>
> >> (nilfs_cleanerd->c_nilfs->n_sb->s_nsegments *
> >> nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) / 100
> >>
> >> or do you know any better way ?
> >>     
> > The kernel code calulates the number by:
> >
> >   = max(NILFS_MIN_NRSVSEGS,
> >         DIV_ROUND_UP(nsegments * r_segments_percentage, 100))
> >
> >   where NILFS_MIN_NRSVSEGS is defined in include/nilfs2_fs.h, and
> >   DIV_ROUND_UP is defined as follows:
> >
> >  #define DIV_ROUND_UP(n,d)    (((n) +  (d) - 1) / (d))
> >
> > The same or some equivelent calculation seems preferable.
> >  
> > With regards,
> > Ryusuke Konishi
> >
> >   
> >> On 03/28/10 17:26, Ryusuke Konishi wrote:
> >>     
> >>> Hi,
> >>> On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >>>   
> >>>       
> >>>> Hi,
> >>>>
> >>>> here the nogc patch
> >>>>
> >>>> As changelog description for this one, we could put:
> >>>>
> >>>> add mount option to disable garbage collection
> >>>>
> >>>> Thanks in advance
> >>>> Bye,
> >>>> David Arendt
> >>>>     
> >>>>         
> >>> Hmm, the patch looks perfect.
> >>>
> >>> Will queue both in the git tree of utils.
> >>>
> >>> Thanks,
> >>> 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

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

[Ryusuke Konishi]

Hi!
On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> > Hi,
> > 
> > here the changes
> > 
> > Thank in advance,
> > David Arendt
> 
> Looks fine to me.  Will apply later.
> 
> Thanks for your quick work.
> 
> Ryusuke Konishi

I enhanced your change so that min_clean_segments and
max_clean_segments can be specified with a ratio (%) or an absolute
value (MB, GB, and so on) of capacity.

The change is available on the head of util's git repo.

Now, my question is how we should set the default value of these
parameters.  During test, I got disk full several times, and I feel
min_free_segments = 100 is a bit tight.

Of course this depends on the usage of each, but I think that the
default values are desirable to have some generality (when possible).

The following setting is my current idea for this.  How does it look?

 min_clean_segments      10%
 max_clean_segments      20%
 clean_check_interval    10

I also feel GC speed should be accelerated than now while the
filesystem is close to disk full.  One simple method is adding
optional nsegments_per_clean and cleaning_interval parameters for <
min_clean_segments.  Or, some sort of adaptive acceleration should be
applied.

I'm planning to make the next util release after this settles down.

Any idea?

Thanks,
Ryusuke Konishi
  
> > On 03/29/10 05:59, Ryusuke Konishi wrote:
> > > Hi,
> > > On Sun, 28 Mar 2010 23:52:52 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> > >   
> > >> Hi,
> > >>
> > >> thanks for applying the patches. I did all my tests on 2 gbyte loop
> > >> devices and now that it is officially in git, I deployed it to some
> > >> production systems with big disks. Here I have noticed, that I have
> > >> completely forgotten the reserved segments. Technically this is not a
> > >> problem, but I think people changing configuration files will tend to
> > >> forget about it. I'm thinking it might be useful to add them internally
> > >> to min_free_segments and max_free_segments so users don't need to worry
> > >> about them. What do you think ?
> > >>     
> > > Ahh, we should take into account the number of reserved segments.  If
> > > not so, cleaner control with the two threshold values will not work
> > > properly for large drives.
> > >
> > >   
> > >> If you like to change the current behavior to this behavior, I will
> > >> submit a short update patch.
> > >>     
> > > Yes, please do.
> > >
> > >   
> > >> I am thinking about getting the number of reserved segments this way:
> > >>
> > >> (nilfs_cleanerd->c_nilfs->n_sb->s_nsegments *
> > >> nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) / 100
> > >>
> > >> or do you know any better way ?
> > >>     
> > > The kernel code calulates the number by:
> > >
> > >   = max(NILFS_MIN_NRSVSEGS,
> > >         DIV_ROUND_UP(nsegments * r_segments_percentage, 100))
> > >
> > >   where NILFS_MIN_NRSVSEGS is defined in include/nilfs2_fs.h, and
> > >   DIV_ROUND_UP is defined as follows:
> > >
> > >  #define DIV_ROUND_UP(n,d)    (((n) +  (d) - 1) / (d))
> > >
> > > The same or some equivelent calculation seems preferable.
> > >  
> > > With regards,
> > > Ryusuke Konishi
> > >
> > >   
> > >> On 03/28/10 17:26, Ryusuke Konishi wrote:
> > >>     
> > >>> Hi,
> > >>> On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> > >>>   
> > >>>       
> > >>>> Hi,
> > >>>>
> > >>>> here the nogc patch
> > >>>>
> > >>>> As changelog description for this one, we could put:
> > >>>>
> > >>>> add mount option to disable garbage collection
> > >>>>
> > >>>> Thanks in advance
> > >>>> Bye,
> > >>>> David Arendt
> > >>>>     
> > >>>>         
> > >>> Hmm, the patch looks perfect.
> > >>>
> > >>> Will queue both in the git tree of utils.
> > >>>
> > >>> Thanks,
> > >>> 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

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

[David Arendt]

Hi,

Actually I run with min_clean_segments at 250 and found that to be a
good value. However for example for a 2 gbyte usb key, this value would
not work at all, therefor I find it a good idea to set the default at
10% as it would be more general for any device size as lots of people
simply try the defaults without changing configuration files. I really
like your idea with having a second set of nsegments_per_clean and
cleaning_interval_parameters for < min_clean_segments. I am wondering if
adaptive optimization will be good, as I think different people will
expect different behavior.  Someones might prefer the system using 100%
io usage for cleaning and the disk not getting full. Other ones might
prefer the disk getting full and using less io usage. Therefor I think
it would add a lot of parameters to the configuration file for giving
people the ability to tune it correctly, and this would possibly
complicate the configuration to much.

If you decide for the second set of nsegments_per_clean and
cleaning_interval_parameters, please tell me if I should implement it or
if you will implement it, not that we are working on the same
functionality at the same time.

I think good names might be

mc_nsegments_per_clean and
mc_cleaning_interval

as this would be compatible with the current indentation in
nilfs_cleanerd.conf.

What would you take as default values ? As you always told that it would
be preferable to reduce cleaning_interval instead of increasing
nsegments_per_clean would you set cleaning interval to 0 in this case
causing permanent cleaning and leave nsegements_per_clean at or which
values would you choose ?

Thanks in advance
Bye,
David Arendt

On 04/05/10 05:02, Ryusuke Konishi wrote:
> Hi!
> On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
>   
>> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>>     
>>> Hi,
>>>
>>> here the changes
>>>
>>> Thank in advance,
>>> David Arendt
>>>       
>> Looks fine to me.  Will apply later.
>>
>> Thanks for your quick work.
>>
>> Ryusuke Konishi
>>     
> I enhanced your change so that min_clean_segments and
> max_clean_segments can be specified with a ratio (%) or an absolute
> value (MB, GB, and so on) of capacity.
>
> The change is available on the head of util's git repo.
>
> Now, my question is how we should set the default value of these
> parameters.  During test, I got disk full several times, and I feel
> min_free_segments = 100 is a bit tight.
>
> Of course this depends on the usage of each, but I think that the
> default values are desirable to have some generality (when possible).
>
> The following setting is my current idea for this.  How does it look?
>
>  min_clean_segments      10%
>  max_clean_segments      20%
>  clean_check_interval    10
>
> I also feel GC speed should be accelerated than now while the
> filesystem is close to disk full.  One simple method is adding
> optional nsegments_per_clean and cleaning_interval parameters for <
> min_clean_segments.  Or, some sort of adaptive acceleration should be
> applied.
>
> I'm planning to make the next util release after this settles down.
>
> Any idea?
>
> Thanks,
> Ryusuke Konishi
>   
>   
>>> On 03/29/10 05:59, Ryusuke Konishi wrote:
>>>       
>>>> Hi,
>>>> On Sun, 28 Mar 2010 23:52:52 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>>>>   
>>>>         
>>>>> Hi,
>>>>>
>>>>> thanks for applying the patches. I did all my tests on 2 gbyte loop
>>>>> devices and now that it is officially in git, I deployed it to some
>>>>> production systems with big disks. Here I have noticed, that I have
>>>>> completely forgotten the reserved segments. Technically this is not a
>>>>> problem, but I think people changing configuration files will tend to
>>>>> forget about it. I'm thinking it might be useful to add them internally
>>>>> to min_free_segments and max_free_segments so users don't need to worry
>>>>> about them. What do you think ?
>>>>>     
>>>>>           
>>>> Ahh, we should take into account the number of reserved segments.  If
>>>> not so, cleaner control with the two threshold values will not work
>>>> properly for large drives.
>>>>
>>>>   
>>>>         
>>>>> If you like to change the current behavior to this behavior, I will
>>>>> submit a short update patch.
>>>>>     
>>>>>           
>>>> Yes, please do.
>>>>
>>>>   
>>>>         
>>>>> I am thinking about getting the number of reserved segments this way:
>>>>>
>>>>> (nilfs_cleanerd->c_nilfs->n_sb->s_nsegments *
>>>>> nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) / 100
>>>>>
>>>>> or do you know any better way ?
>>>>>     
>>>>>           
>>>> The kernel code calulates the number by:
>>>>
>>>>   = max(NILFS_MIN_NRSVSEGS,
>>>>         DIV_ROUND_UP(nsegments * r_segments_percentage, 100))
>>>>
>>>>   where NILFS_MIN_NRSVSEGS is defined in include/nilfs2_fs.h, and
>>>>   DIV_ROUND_UP is defined as follows:
>>>>
>>>>  #define DIV_ROUND_UP(n,d)    (((n) +  (d) - 1) / (d))
>>>>
>>>> The same or some equivelent calculation seems preferable.
>>>>  
>>>> With regards,
>>>> Ryusuke Konishi
>>>>
>>>>   
>>>>         
>>>>> On 03/28/10 17:26, Ryusuke Konishi wrote:
>>>>>     
>>>>>           
>>>>>> Hi,
>>>>>> On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>>>>>>   
>>>>>>       
>>>>>>             
>>>>>>> Hi,
>>>>>>>
>>>>>>> here the nogc patch
>>>>>>>
>>>>>>> As changelog description for this one, we could put:
>>>>>>>
>>>>>>> add mount option to disable garbage collection
>>>>>>>
>>>>>>> Thanks in advance
>>>>>>> Bye,
>>>>>>> David Arendt
>>>>>>>     
>>>>>>>         
>>>>>>>               
>>>>>> Hmm, the patch looks perfect.
>>>>>>
>>>>>> Will queue both in the git tree of utils.
>>>>>>
>>>>>> Thanks,
>>>>>> 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

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

[Ryusuke Konishi]

Hi,
On Mon, 05 Apr 2010 09:50:11 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> Hi,
> 
> Actually I run with min_clean_segments at 250 and found that to be a
> good value. However for example for a 2 gbyte usb key, this value would
> not work at all, therefor I find it a good idea to set the default at
> 10% as it would be more general for any device size as lots of people
> simply try the defaults without changing configuration files.

Ok, let's start with the 10% min_clean_segments for the default
values.

> I really like your idea with having a second set of
> nsegments_per_clean and cleaning_interval_parameters for <
> min_clean_segments. I am wondering if adaptive optimization will be
> good, as I think different people will expect different behavior.
> Someones might prefer the system using 100% io usage for cleaning
> and the disk not getting full. Other ones might prefer the disk
> getting full and using less io usage.

Well, that's true.  Netbook users would prefer suspending the cleaner
wherever possible to avoid their SSD worn out. Meanwhile, NAS users
would expect it to operate safely to avoid trouble.

For the present, let's try to handle the disk full issue by adding a
few parameters effectively.

> Therefor I think it would add a lot of parameters to the
> configuration file for giving people the ability to tune it
> correctly, and this would possibly complicate the configuration to
> much.

> If you decide for the second set of nsegments_per_clean and
> cleaning_interval_parameters, please tell me if I should implement it or
> if you will implement it, not that we are working on the same
> functionality at the same time.

I'll do the change of default values.  And will give over to you for
this extension ;)

> I think good names might be
> 
> mc_nsegments_per_clean and
> mc_cleaning_interval
> 
> as this would be compatible with the current indentation in
> nilfs_cleanerd.conf.

All right.
 
> What would you take as default values ? As you always told that it would
> be preferable to reduce cleaning_interval instead of increasing
> nsegments_per_clean

Yes, adjusting mc_cleaning_interval should come before increasing
mc_nsegments_per_clean.  My image is, for example, as follows:

  mc_cleaning_interval  1 or 2
  mc_nsegments_per_clean  2 ~ 4 (as needed)

> would you set cleaning interval to 0 in this case
> causing permanent cleaning and leave nsegements_per_clean at or which
> values would you choose ?

The permanent cleaning may spoil responsiveness of the system.  I
don't know.  Seems that we need some test.

Thanks,
Ryusuke Konishi

> On 04/05/10 05:02, Ryusuke Konishi wrote:
> > Hi!
> > On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
> >   
> >> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >>     
> >>> Hi,
> >>>
> >>> here the changes
> >>>
> >>> Thank in advance,
> >>> David Arendt
> >>>       
> >> Looks fine to me.  Will apply later.
> >>
> >> Thanks for your quick work.
> >>
> >> Ryusuke Konishi
> >>     
> > I enhanced your change so that min_clean_segments and
> > max_clean_segments can be specified with a ratio (%) or an absolute
> > value (MB, GB, and so on) of capacity.
> >
> > The change is available on the head of util's git repo.
> >
> > Now, my question is how we should set the default value of these
> > parameters.  During test, I got disk full several times, and I feel
> > min_free_segments = 100 is a bit tight.
> >
> > Of course this depends on the usage of each, but I think that the
> > default values are desirable to have some generality (when possible).
> >
> > The following setting is my current idea for this.  How does it look?
> >
> >  min_clean_segments      10%
> >  max_clean_segments      20%
> >  clean_check_interval    10
> >
> > I also feel GC speed should be accelerated than now while the
> > filesystem is close to disk full.  One simple method is adding
> > optional nsegments_per_clean and cleaning_interval parameters for <
> > min_clean_segments.  Or, some sort of adaptive acceleration should be
> > applied.
> >
> > I'm planning to make the next util release after this settles down.
> >
> > Any idea?
> >
> > Thanks,
> > 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

cleaner: run one cleaning pass based on minimum free space

[Jan de Kruyf]

Hallo,
May I break into this discussion with something perhaps partially
connected to the thread:
I had my usual disk full problem again on a half full partition after
a power crash, that so far has not been explained yet.

Did any of you pick up anything in the cleanerd code that might point
somewhere in the direction of a solution?

Regards

Jan de Kruyf.

Its a beautiful quiet rainy day in Johannesburg. The earth is full of glory!



On Mon, Apr 5, 2010 at 9:50 AM, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>
> Hi,
>
> Actually I run with min_clean_segments at 250 and found that to be a
> good value. However for example for a 2 gbyte usb key, this value would
> not work at all, therefor I find it a good idea to set the default at
> 10% as it would be more general for any device size as lots of people
> simply try the defaults without changing configuration files. I really
> like your idea with having a second set of nsegments_per_clean and
> cleaning_interval_parameters for < min_clean_segments. I am wondering if
> adaptive optimization will be good, as I think different people will
> expect different behavior.  Someones might prefer the system using 100%
> io usage for cleaning and the disk not getting full. Other ones might
> prefer the disk getting full and using less io usage. Therefor I think
> it would add a lot of parameters to the configuration file for giving
> people the ability to tune it correctly, and this would possibly
> complicate the configuration to much.
>
> If you decide for the second set of nsegments_per_clean and
> cleaning_interval_parameters, please tell me if I should implement it or
> if you will implement it, not that we are working on the same
> functionality at the same time.
>
> I think good names might be
>
> mc_nsegments_per_clean and
> mc_cleaning_interval
>
> as this would be compatible with the current indentation in
> nilfs_cleanerd.conf.
>
> What would you take as default values ? As you always told that it would
> be preferable to reduce cleaning_interval instead of increasing
> nsegments_per_clean would you set cleaning interval to 0 in this case
> causing permanent cleaning and leave nsegements_per_clean at or which
> values would you choose ?
>
> Thanks in advance
> Bye,
> David Arendt
>
> On 04/05/10 05:02, Ryusuke Konishi wrote:
> > Hi!
> > On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke@osrg.net> wrote:
> >
> >> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >>
> >>> Hi,
> >>>
> >>> here the changes
> >>>
> >>> Thank in advance,
> >>> David Arendt
> >>>
> >> Looks fine to me.  Will apply later.
> >>
> >> Thanks for your quick work.
> >>
> >> Ryusuke Konishi
> >>
> > I enhanced your change so that min_clean_segments and
> > max_clean_segments can be specified with a ratio (%) or an absolute
> > value (MB, GB, and so on) of capacity.
> >
> > The change is available on the head of util's git repo.
> >
> > Now, my question is how we should set the default value of these
> > parameters.  During test, I got disk full several times, and I feel
> > min_free_segments = 100 is a bit tight.
> >
> > Of course this depends on the usage of each, but I think that the
> > default values are desirable to have some generality (when possible).
> >
> > The following setting is my current idea for this.  How does it look?
> >
> >  min_clean_segments      10%
> >  max_clean_segments      20%
> >  clean_check_interval    10
> >
> > I also feel GC speed should be accelerated than now while the
> > filesystem is close to disk full.  One simple method is adding
> > optional nsegments_per_clean and cleaning_interval parameters for <
> > min_clean_segments.  Or, some sort of adaptive acceleration should be
> > applied.
> >
> > I'm planning to make the next util release after this settles down.
> >
> > Any idea?
> >
> > Thanks,
> > Ryusuke Konishi
> >
> >
> >>> On 03/29/10 05:59, Ryusuke Konishi wrote:
> >>>
> >>>> Hi,
> >>>> On Sun, 28 Mar 2010 23:52:52 +0200, David Arendt <admin-/LHdS3kC8Bc@public.gmane.orgg> wrote:
> >>>>
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> thanks for applying the patches. I did all my tests on 2 gbyte loop
> >>>>> devices and now that it is officially in git, I deployed it to some
> >>>>> production systems with big disks. Here I have noticed, that I have
> >>>>> completely forgotten the reserved segments. Technically this is not a
> >>>>> problem, but I think people changing configuration files will tend to
> >>>>> forget about it. I'm thinking it might be useful to add them internally
> >>>>> to min_free_segments and max_free_segments so users don't need to worry
> >>>>> about them. What do you think ?
> >>>>>
> >>>>>
> >>>> Ahh, we should take into account the number of reserved segments.  If
> >>>> not so, cleaner control with the two threshold values will not work
> >>>> properly for large drives.
> >>>>
> >>>>
> >>>>
> >>>>> If you like to change the current behavior to this behavior, I will
> >>>>> submit a short update patch.
> >>>>>
> >>>>>
> >>>> Yes, please do.
> >>>>
> >>>>
> >>>>
> >>>>> I am thinking about getting the number of reserved segments this way:
> >>>>>
> >>>>> (nilfs_cleanerd->c_nilfs->n_sb->s_nsegments *
> >>>>> nilfs_cleanerd->c_nilfs->n_sb->s_r_segments_percentage) / 100
> >>>>>
> >>>>> or do you know any better way ?
> >>>>>
> >>>>>
> >>>> The kernel code calulates the number by:
> >>>>
> >>>>   = max(NILFS_MIN_NRSVSEGS,
> >>>>         DIV_ROUND_UP(nsegments * r_segments_percentage, 100))
> >>>>
> >>>>   where NILFS_MIN_NRSVSEGS is defined in include/nilfs2_fs.h, and
> >>>>   DIV_ROUND_UP is defined as follows:
> >>>>
> >>>>  #define DIV_ROUND_UP(n,d)    (((n) +  (d) - 1) / (d))
> >>>>
> >>>> The same or some equivelent calculation seems preferable.
> >>>>
> >>>> With regards,
> >>>> Ryusuke Konishi
> >>>>
> >>>>
> >>>>
> >>>>> On 03/28/10 17:26, Ryusuke Konishi wrote:
> >>>>>
> >>>>>
> >>>>>> Hi,
> >>>>>> On Sun, 28 Mar 2010 14:17:00 +0200, David Arendt <admin@prnet.org> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> here the nogc patch
> >>>>>>>
> >>>>>>> As changelog description for this one, we could put:
> >>>>>>>
> >>>>>>> add mount option to disable garbage collection
> >>>>>>>
> >>>>>>> Thanks in advance
> >>>>>>> Bye,
> >>>>>>> David Arendt
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>> Hmm, the patch looks perfect.
> >>>>>>
> >>>>>> Will queue both in the git tree of utils.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> 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
--
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

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

[David Arendt]

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

Hi,

here is the patch

Thanks in advance
Bye,
David Arendt

On 04/05/10 13:34, Ryusuke Konishi wrote:
> Hi,
> On Mon, 05 Apr 2010 09:50:11 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>   
>> Hi,
>>
>> Actually I run with min_clean_segments at 250 and found that to be a
>> good value. However for example for a 2 gbyte usb key, this value would
>> not work at all, therefor I find it a good idea to set the default at
>> 10% as it would be more general for any device size as lots of people
>> simply try the defaults without changing configuration files.
>>     
> Ok, let's start with the 10% min_clean_segments for the default
> values.
>
>   
>> I really like your idea with having a second set of
>> nsegments_per_clean and cleaning_interval_parameters for <
>> min_clean_segments. I am wondering if adaptive optimization will be
>> good, as I think different people will expect different behavior.
>> Someones might prefer the system using 100% io usage for cleaning
>> and the disk not getting full. Other ones might prefer the disk
>> getting full and using less io usage.
>>     
> Well, that's true.  Netbook users would prefer suspending the cleaner
> wherever possible to avoid their SSD worn out. Meanwhile, NAS users
> would expect it to operate safely to avoid trouble.
>
> For the present, let's try to handle the disk full issue by adding a
> few parameters effectively.
>
>   
>> Therefor I think it would add a lot of parameters to the
>> configuration file for giving people the ability to tune it
>> correctly, and this would possibly complicate the configuration to
>> much.
>>     
>   
>> If you decide for the second set of nsegments_per_clean and
>> cleaning_interval_parameters, please tell me if I should implement it or
>> if you will implement it, not that we are working on the same
>> functionality at the same time.
>>     
> I'll do the change of default values.  And will give over to you for
> this extension ;)
>
>   
>> I think good names might be
>>
>> mc_nsegments_per_clean and
>> mc_cleaning_interval
>>
>> as this would be compatible with the current indentation in
>> nilfs_cleanerd.conf.
>>     
> All right.
>  
>   
>> What would you take as default values ? As you always told that it would
>> be preferable to reduce cleaning_interval instead of increasing
>> nsegments_per_clean
>>     
> Yes, adjusting mc_cleaning_interval should come before increasing
> mc_nsegments_per_clean.  My image is, for example, as follows:
>
>   mc_cleaning_interval  1 or 2
>   mc_nsegments_per_clean  2 ~ 4 (as needed)
>
>   
>> would you set cleaning interval to 0 in this case
>> causing permanent cleaning and leave nsegements_per_clean at or which
>> values would you choose ?
>>     
> The permanent cleaning may spoil responsiveness of the system.  I
> don't know.  Seems that we need some test.
>
> Thanks,
> Ryusuke Konishi
>
>   
>> On 04/05/10 05:02, Ryusuke Konishi wrote:
>>     
>>> Hi!
>>> On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
>>>   
>>>       
>>>> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>>>>     
>>>>         
>>>>> Hi,
>>>>>
>>>>> here the changes
>>>>>
>>>>> Thank in advance,
>>>>> David Arendt
>>>>>       
>>>>>           
>>>> Looks fine to me.  Will apply later.
>>>>
>>>> Thanks for your quick work.
>>>>
>>>> Ryusuke Konishi
>>>>     
>>>>         
>>> I enhanced your change so that min_clean_segments and
>>> max_clean_segments can be specified with a ratio (%) or an absolute
>>> value (MB, GB, and so on) of capacity.
>>>
>>> The change is available on the head of util's git repo.
>>>
>>> Now, my question is how we should set the default value of these
>>> parameters.  During test, I got disk full several times, and I feel
>>> min_free_segments = 100 is a bit tight.
>>>
>>> Of course this depends on the usage of each, but I think that the
>>> default values are desirable to have some generality (when possible).
>>>
>>> The following setting is my current idea for this.  How does it look?
>>>
>>>  min_clean_segments      10%
>>>  max_clean_segments      20%
>>>  clean_check_interval    10
>>>
>>> I also feel GC speed should be accelerated than now while the
>>> filesystem is close to disk full.  One simple method is adding
>>> optional nsegments_per_clean and cleaning_interval parameters for <
>>> min_clean_segments.  Or, some sort of adaptive acceleration should be
>>> applied.
>>>
>>> I'm planning to make the next util release after this settles down.
>>>
>>> Any idea?
>>>
>>> Thanks,
>>> Ryusuke Konishi
>>>   
>>>   
>>>       


[-- Attachment #2: mc.patch --]
[-- Type: text/plain, Size: 8256 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-04-05 13:51:08.598423423 +0200
+++ nilfs2-utils/man/nilfs_cleanerd.conf.5	2010-04-05 15:24:47.066610242 +0200
@@ -56,9 +56,18 @@
 Specify the number of segments reclaimed by a single cleaning step.
 The default value is 2.
 .TP
+.B mc_nsegments_per_clean
+Specify the number of segments reclaimed by a single cleaning step
+if clean segments < min_clean_segments.
+The default value is 4.
+.TP
 .B cleaning_interval
 Specify the cleaning interval in seconds.  The default value is 5.
 .TP
+.B mc_cleaning_interval
+Specify the cleaning interval in seconds
+if clean segments < min_clean_segments.  The default value is 1.
+.TP
 .B retry_interval
 Specify retry interval in seconds.  This value provides the retry
 interval of GC in case of resource shortages.  The default value is
diff -ur nilfs2-utils.orig/sbin/cleanerd/cldconfig.c nilfs2-utils/sbin/cleanerd/cldconfig.c
--- nilfs2-utils.orig/sbin/cleanerd/cldconfig.c	2010-04-05 13:51:08.599423399 +0200
+++ nilfs2-utils/sbin/cleanerd/cldconfig.c	2010-04-05 15:12:59.090435567 +0200
@@ -405,6 +405,26 @@
 }
 
 static int
+nilfs_cldconfig_handle_mc_nsegments_per_clean(struct nilfs_cldconfig *config,
+					      char **tokens, size_t ntoks,
+					      struct nilfs *nilfs)
+{
+	unsigned long n;
+
+	if (nilfs_cldconfig_get_ulong_argument(tokens, ntoks, &n) < 0)
+		return 0;
+
+	if (n > NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX) {
+		syslog(LOG_WARNING, "%s: %s: too large, use the maximum value",
+		       tokens[0], tokens[1]);
+		n = NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX;
+	}
+
+	config->cf_mc_nsegments_per_clean = n;
+	return 0;
+}
+
+static int
 nilfs_cldconfig_handle_cleaning_interval(struct nilfs_cldconfig *config,
 					 char **tokens, size_t ntoks,
 					 struct nilfs *nilfs)
@@ -417,6 +437,18 @@
 }
 
 static int
+nilfs_cldconfig_handle_mc_cleaning_interval(struct nilfs_cldconfig *config,
+					    char **tokens, size_t ntoks,
+					    struct nilfs *nilfs)
+{
+	unsigned long sec;
+
+	if (nilfs_cldconfig_get_ulong_argument(tokens, ntoks, &sec) == 0)
+		config->cf_mc_cleaning_interval = sec;
+	return 0;
+}
+
+static int
 nilfs_cldconfig_handle_retry_interval(struct nilfs_cldconfig *config,
 				      char **tokens, size_t ntoks,
 				      struct nilfs *nilfs)
@@ -499,10 +531,18 @@
 		nilfs_cldconfig_handle_nsegments_per_clean
 	},
 	{
+		"mc_nsegments_per_clean", 2, 2,
+		nilfs_cldconfig_handle_mc_nsegments_per_clean
+	},
+	{
 		"cleaning_interval", 2, 2,
 		nilfs_cldconfig_handle_cleaning_interval
 	},
 	{
+		"mc_cleaning_interval", 2, 2,
+		nilfs_cldconfig_handle_mc_cleaning_interval
+	},
+	{
 		"retry_interval", 2, 2,
 		nilfs_cldconfig_handle_retry_interval
 	},
@@ -555,7 +595,9 @@
 	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_mc_nsegments_per_clean = NILFS_CLDCONFIG_MC_NSEGMENTS_PER_CLEAN;
 	config->cf_cleaning_interval = NILFS_CLDCONFIG_CLEANING_INTERVAL;
+	config->cf_mc_cleaning_interval = NILFS_CLDCONFIG_MC_CLEANING_INTERVAL;
 	config->cf_retry_interval = NILFS_CLDCONFIG_RETRY_INTERVAL;
 	config->cf_use_mmap = NILFS_CLDCONFIG_USE_MMAP;
 	config->cf_log_priority = NILFS_CLDCONFIG_LOG_PRIORITY;
diff -ur nilfs2-utils.orig/sbin/cleanerd/cldconfig.h nilfs2-utils/sbin/cleanerd/cldconfig.h
--- nilfs2-utils.orig/sbin/cleanerd/cldconfig.h	2010-04-05 13:51:08.599423399 +0200
+++ nilfs2-utils/sbin/cleanerd/cldconfig.h	2010-04-05 14:39:54.355158920 +0200
@@ -78,7 +78,9 @@
  * @cf_max_clean_segments: high threshold on the number of free segments
  * @cf_clean_check_interval: cleaner check interval
  * @cf_nsegments_per_clean number of segments reclaimed per clean cycle
+ * @cf_mc_nsegments_per_clean: number of segments reclaimed per clean cycle if clean segments < min_clean_segments
  * @cf_cleaning_interval: cleaning interval
+ * @cf_mc_cleaning_interval: cleaning interval if clean segments < min_clean_segments
  * @cf_use_mmap: flag that indicate using mmap
  * @cf_log_priority: log priority level
  */
@@ -89,7 +91,9 @@
 	__u64 cf_max_clean_segments;
 	time_t cf_clean_check_interval;
 	int cf_nsegments_per_clean;
+	int cf_mc_nsegments_per_clean;
 	time_t cf_cleaning_interval;
+	time_t cf_mc_cleaning_interval;
 	time_t cf_retry_interval;
 	int cf_use_mmap;
 	int cf_log_priority;
@@ -103,7 +107,9 @@
 #define	NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS		200
 #define	NILFS_CLDCONFIG_CLEAN_CHECK_INTERVAL		60
 #define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN		2
+#define NILFS_CLDCONFIG_MC_NSEGMENTS_PER_CLEAN		4
 #define NILFS_CLDCONFIG_CLEANING_INTERVAL		5
+#define NILFS_CLDCONFIG_MC_CLEANING_INTERVAL		1
 #define NILFS_CLDCONFIG_RETRY_INTERVAL			60
 #define NILFS_CLDCONFIG_USE_MMAP			1
 #define NILFS_CLDCONFIG_LOG_PRIORITY			LOG_INFO
diff -ur nilfs2-utils.orig/sbin/cleanerd/cleanerd.c nilfs2-utils/sbin/cleanerd/cleanerd.c
--- nilfs2-utils.orig/sbin/cleanerd/cleanerd.c	2010-04-05 13:51:08.599423399 +0200
+++ nilfs2-utils/sbin/cleanerd/cleanerd.c	2010-04-05 15:14:15.516259856 +0200
@@ -158,6 +158,8 @@
 		}
 		cleanerd->c_ncleansegs =
 			cleanerd->c_config.cf_nsegments_per_clean;
+		cleanerd->c_cleaning_interval =
+			cleanerd->c_config.cf_cleaning_interval;
 	}
 	return ret;
 }
@@ -1136,13 +1138,13 @@
 	/* curr >= target */
 	if (!timercmp(&curr, &cleanerd->c_target, <)) {
 		cleanerd->c_target = curr;
-		cleanerd->c_target.tv_sec += config->cf_cleaning_interval;
+		cleanerd->c_target.tv_sec += cleanerd->c_cleaning_interval;
 		syslog(LOG_DEBUG, "adjust interval");
 		return 1; /* skip a sleep */
 	}
 	timersub(&cleanerd->c_target, &curr, &diff);
 	timeval_to_timespec(&diff, timeout);
-	cleanerd->c_target.tv_sec += config->cf_cleaning_interval;
+	cleanerd->c_target.tv_sec += cleanerd->c_cleaning_interval;
 	return 0;
 }
 
@@ -1218,6 +1220,7 @@
 		return -1;
 
 	cleanerd->c_ncleansegs = cleanerd->c_config.cf_nsegments_per_clean;
+	cleanerd->c_cleaning_interval = cleanerd->c_config.cf_cleaning_interval;
 
 	r_segments = nilfs_get_reserved_segments(cleanerd->c_nilfs);
 
@@ -1257,6 +1260,15 @@
 				else
 					goto sleep;
 			}
+
+			if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_clean_segments + r_segments) {
+				cleanerd->c_ncleansegs = cleanerd->c_config.cf_mc_nsegments_per_clean;
+				cleanerd->c_cleaning_interval = cleanerd->c_config.cf_mc_cleaning_interval;
+			}
+			else {
+				cleanerd->c_ncleansegs = cleanerd->c_config.cf_nsegments_per_clean;
+				cleanerd->c_cleaning_interval = cleanerd->c_config.cf_cleaning_interval;
+			}
 		}
 
 		if (sustat.ss_nongc_ctime != prev_nongc_ctime) {
diff -ur nilfs2-utils.orig/sbin/cleanerd/cleanerd.h nilfs2-utils/sbin/cleanerd/cleanerd.h
--- nilfs2-utils.orig/sbin/cleanerd/cleanerd.h	2010-04-05 13:51:08.599423399 +0200
+++ nilfs2-utils/sbin/cleanerd/cleanerd.h	2010-04-05 14:43:00.874482811 +0200
@@ -43,6 +43,7 @@
  * @c_running: running state
  * @c_fallback: fallback state
  * @c_ncleansegs: number of semgents cleaned per cycle
+ * @c_cleaning_interval: cleaning interval
  * @c_protcno: the minimum of checkpoint numbers within protection period
  * @c_prottime: start time of protection period
  * @c_target: target time for sleeping
@@ -54,6 +55,7 @@
 	int c_running;
 	int c_fallback;
 	int c_ncleansegs;
+	time_t c_cleaning_interval;
 	nilfs_cno_t c_protcno;
 	__u64 c_prottime;
 	struct timeval c_target;
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-04-05 13:51:08.599423399 +0200
+++ nilfs2-utils/sbin/cleanerd/nilfs_cleanerd.conf	2010-04-05 15:33:22.608600554 +0200
@@ -37,9 +37,17 @@
 # The maximum number of segments to be cleaned at a time.
 nsegments_per_clean	2
 
+# The maximum number of segments to be cleaned at a time
+# if clean segments < min_clean_segments
+mc_nsegments_per_clean	4
+
 # Cleaning interval in second.
 cleaning_interval	5
 
+# Cleaning interval in seconds
+# if clean segments < min_clean_segments
+mc_cleaning_interval	1
+
 # Retry interval in seconds.
 retry_interval		60
 

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

[Ryusuke Konishi]

Hi,
On Mon, 05 Apr 2010 15:35:34 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> Hi,
> 
> here is the patch
> 
> Thanks in advance
> Bye,
> David Arendt

Thanks.

Looks functionally fine. Just a matter of coding style:

The following part of cleaner loop looks bloated.  I think it's a good
opportunity to divide it from the loop.

Also, the length of their lines looks too long.  It's preferable if
they're naturally wrapped within 80-columns.  This is not a must
especially for userland tools, however kernel developers often prefer
this conventional coding rule.  Separating the routine would make this
easy.

                if (cleanerd->c_config.cf_min_clean_segments > 0) {
---> from
                        if (cleanerd->c_running) {
                                if (sustat.ss_ncleansegs > cleanerd->c_config.c\
f_max_clean_segments + r_segments) {
                                        nilfs_cleanerd_clean_check_pause(cleane\
rd, &timeout);
                                        goto sleep;
                                }
                        }
                        else {
                                if (sustat.ss_ncleansegs < cleanerd->c_config.c\
f_min_clean_segments + r_segments)
                                        nilfs_cleanerd_clean_check_resume(clean\
erd);
                                else
                                        goto sleep;
                        }

                        if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_cl\
ean_segments + r_segments) {
                                cleanerd->c_ncleansegs = cleanerd->c_config.cf_\
mc_nsegments_per_clean;
                                cleanerd->c_cleaning_interval = cleanerd->c_con\
fig.cf_mc_cleaning_interval;
                        }
                        else {
                                cleanerd->c_ncleansegs = cleanerd->c_config.cf_\
nsegments_per_clean;
                                cleanerd->c_cleaning_interval = cleanerd->c_con\
fig.cf_cleaning_interval;
                        }
<---- to
                }

With regards,
Ryusuke Konishi


> On 04/05/10 13:34, Ryusuke Konishi wrote:
> > Hi,
> > On Mon, 05 Apr 2010 09:50:11 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >   
> >> Hi,
> >>
> >> Actually I run with min_clean_segments at 250 and found that to be a
> >> good value. However for example for a 2 gbyte usb key, this value would
> >> not work at all, therefor I find it a good idea to set the default at
> >> 10% as it would be more general for any device size as lots of people
> >> simply try the defaults without changing configuration files.
> >>     
> > Ok, let's start with the 10% min_clean_segments for the default
> > values.
> >
> >   
> >> I really like your idea with having a second set of
> >> nsegments_per_clean and cleaning_interval_parameters for <
> >> min_clean_segments. I am wondering if adaptive optimization will be
> >> good, as I think different people will expect different behavior.
> >> Someones might prefer the system using 100% io usage for cleaning
> >> and the disk not getting full. Other ones might prefer the disk
> >> getting full and using less io usage.
> >>     
> > Well, that's true.  Netbook users would prefer suspending the cleaner
> > wherever possible to avoid their SSD worn out. Meanwhile, NAS users
> > would expect it to operate safely to avoid trouble.
> >
> > For the present, let's try to handle the disk full issue by adding a
> > few parameters effectively.
> >
> >   
> >> Therefor I think it would add a lot of parameters to the
> >> configuration file for giving people the ability to tune it
> >> correctly, and this would possibly complicate the configuration to
> >> much.
> >>     
> >   
> >> If you decide for the second set of nsegments_per_clean and
> >> cleaning_interval_parameters, please tell me if I should implement it or
> >> if you will implement it, not that we are working on the same
> >> functionality at the same time.
> >>     
> > I'll do the change of default values.  And will give over to you for
> > this extension ;)
> >
> >   
> >> I think good names might be
> >>
> >> mc_nsegments_per_clean and
> >> mc_cleaning_interval
> >>
> >> as this would be compatible with the current indentation in
> >> nilfs_cleanerd.conf.
> >>     
> > All right.
> >  
> >   
> >> What would you take as default values ? As you always told that it would
> >> be preferable to reduce cleaning_interval instead of increasing
> >> nsegments_per_clean
> >>     
> > Yes, adjusting mc_cleaning_interval should come before increasing
> > mc_nsegments_per_clean.  My image is, for example, as follows:
> >
> >   mc_cleaning_interval  1 or 2
> >   mc_nsegments_per_clean  2 ~ 4 (as needed)
> >
> >   
> >> would you set cleaning interval to 0 in this case
> >> causing permanent cleaning and leave nsegements_per_clean at or which
> >> values would you choose ?
> >>     
> > The permanent cleaning may spoil responsiveness of the system.  I
> > don't know.  Seems that we need some test.
> >
> > Thanks,
> > Ryusuke Konishi
> >
> >   
> >> On 04/05/10 05:02, Ryusuke Konishi wrote:
> >>     
> >>> Hi!
> >>> On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
> >>>   
> >>>       
> >>>> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >>>>     
> >>>>         
> >>>>> Hi,
> >>>>>
> >>>>> here the changes
> >>>>>
> >>>>> Thank in advance,
> >>>>> David Arendt
> >>>>>       
> >>>>>           
> >>>> Looks fine to me.  Will apply later.
> >>>>
> >>>> Thanks for your quick work.
> >>>>
> >>>> Ryusuke Konishi
> >>>>     
> >>>>         
> >>> I enhanced your change so that min_clean_segments and
> >>> max_clean_segments can be specified with a ratio (%) or an absolute
> >>> value (MB, GB, and so on) of capacity.
> >>>
> >>> The change is available on the head of util's git repo.
> >>>
> >>> Now, my question is how we should set the default value of these
> >>> parameters.  During test, I got disk full several times, and I feel
> >>> min_free_segments = 100 is a bit tight.
> >>>
> >>> Of course this depends on the usage of each, but I think that the
> >>> default values are desirable to have some generality (when possible).
> >>>
> >>> The following setting is my current idea for this.  How does it look?
> >>>
> >>>  min_clean_segments      10%
> >>>  max_clean_segments      20%
> >>>  clean_check_interval    10
> >>>
> >>> I also feel GC speed should be accelerated than now while the
> >>> filesystem is close to disk full.  One simple method is adding
> >>> optional nsegments_per_clean and cleaning_interval parameters for <
> >>> min_clean_segments.  Or, some sort of adaptive acceleration should be
> >>> applied.
> >>>
> >>> I'm planning to make the next util release after this settles down.
> >>>
> >>> Any idea?
> >>>
> >>> Thanks,
> >>> 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

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

[Ryusuke Konishi]

On Mon, 05 Apr 2010 20:34:50 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
> > If you decide for the second set of nsegments_per_clean and
> > cleaning_interval_parameters, please tell me if I should implement it or
> > if you will implement it, not that we are working on the same
> > functionality at the same time.
> 
> I'll do the change of default values.  And will give over to you for
> this extension ;)

The following patch is my part.

Ryusuke Konishi
--
From: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>

nilfs2-utils: adjust default values of min/max clean segments

This adjusts the default values of thresholds and check interval of GC
execution control to decrease dependency to the capacity and class of
disk devices.

The new default value are as follows:

min_clean_segments     10%
max_clean_segments     20%
clean_check_interval   10

Signed-off-by: Ryusuke Konishi <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
---
 man/nilfs_cleanerd.conf.5         |    9 +++++----
 sbin/cleanerd/cldconfig.c         |   20 ++++++++++++++++----
 sbin/cleanerd/cldconfig.h         |    8 +++++---
 sbin/cleanerd/nilfs_cleanerd.conf |    6 +++---
 4 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/man/nilfs_cleanerd.conf.5 b/man/nilfs_cleanerd.conf.5
index 9d5e807..ad99857 100644
--- a/man/nilfs_cleanerd.conf.5
+++ b/man/nilfs_cleanerd.conf.5
@@ -28,25 +28,26 @@ default value is 3600, meaning one hour.
 .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.
+until less than min_clean_segments are available.
 .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.
 .PP
 \fBmin_clean_segments\fP and \fBmax_clean_segments\fP may be followed
 by a percent sign or the following multiplicative suffixes: kB 1000,
 K 1024, MB 1000*1000, M 1024*1024, GB 1000*1000*1000, G
 1024*1024*1024, and so on for T, P, E.  If the argument is followed by
 a percent sign, it represents a disk capacity ratio.
+.PP
+The default values of \fBmin_clean_segments\fP and
+\fBmax_clean_segments\fP are 10 percent and 20 percent respectively.
 .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.
+The default value is 10.
 .TP
 .B selection_policy
 Specify the GC policy. At present, only the `\fBtimestamp\fP' policy,
diff --git a/sbin/cleanerd/cldconfig.c b/sbin/cleanerd/cldconfig.c
index e7552b6..369cc22 100644
--- a/sbin/cleanerd/cldconfig.c
+++ b/sbin/cleanerd/cldconfig.c
@@ -584,15 +584,27 @@ static int nilfs_cldconfig_handle_keyword(struct nilfs_cldconfig *config,
 	return 0;
 }
 
-static void nilfs_cldconfig_set_default(struct nilfs_cldconfig *config)
+static void nilfs_cldconfig_set_default(struct nilfs_cldconfig *config,
+					struct nilfs *nilfs)
 {
+	struct nilfs_param param;
+
 	config->cf_selection_policy.p_importance =
 		NILFS_CLDCONFIG_SELECTION_POLICY_IMPORTANCE;
 	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;
+
+	param.num = NILFS_CLDCONFIG_MIN_CLEAN_SEGMENTS;
+	param.unit = NILFS_CLDCONFIG_MIN_CLEAN_SEGMENTS_UNIT;
+	config->cf_min_clean_segments =
+		nilfs_convert_size_to_nsegments(nilfs, &param);
+
+	param.num = NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS;
+	param.unit = NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS_UNIT;
+	config->cf_max_clean_segments =
+		nilfs_convert_size_to_nsegments(nilfs, &param);
+
 	config->cf_clean_check_interval = NILFS_CLDCONFIG_CLEAN_CHECK_INTERVAL;
 	config->cf_nsegments_per_clean = NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN;
 	config->cf_mc_nsegments_per_clean = NILFS_CLDCONFIG_MC_NSEGMENTS_PER_CLEAN;
@@ -671,7 +683,7 @@ static int nilfs_cldconfig_do_read(struct nilfs_cldconfig *config,
 int nilfs_cldconfig_read(struct nilfs_cldconfig *config, const char *path,
 			 struct nilfs *nilfs)
 {
-	nilfs_cldconfig_set_default(config);
+	nilfs_cldconfig_set_default(config, nilfs);
 	if (nilfs_cldconfig_do_read(config, path, nilfs) < 0)
 		syslog(LOG_WARNING, "%s: cannot read", path);
 	return 0;
diff --git a/sbin/cleanerd/cldconfig.h b/sbin/cleanerd/cldconfig.h
index 6985594..aec6aaa 100644
--- a/sbin/cleanerd/cldconfig.h
+++ b/sbin/cleanerd/cldconfig.h
@@ -103,9 +103,11 @@ struct nilfs_cldconfig {
 			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_MIN_CLEAN_SEGMENTS		10
+#define NILFS_CLDCONFIG_MIN_CLEAN_SEGMENTS_UNIT		NILFS_SIZE_UNIT_PERCENT
+#define NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS		20
+#define NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS_UNIT		NILFS_SIZE_UNIT_PERCENT
+#define NILFS_CLDCONFIG_CLEAN_CHECK_INTERVAL		10
 #define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN		2
 #define NILFS_CLDCONFIG_MC_NSEGMENTS_PER_CLEAN		4
 #define NILFS_CLDCONFIG_CLEANING_INTERVAL		5
diff --git a/sbin/cleanerd/nilfs_cleanerd.conf b/sbin/cleanerd/nilfs_cleanerd.conf
index 83c941a..151bef0 100644
--- a/sbin/cleanerd/nilfs_cleanerd.conf
+++ b/sbin/cleanerd/nilfs_cleanerd.conf
@@ -14,10 +14,10 @@ protection_period	3600
 # Minium number of clean segments 
 # 0  = normal cleaner behaviour
 # >0 = start cleaning if less segments are available
-min_clean_segments	100
+min_clean_segments	10%
 
 # Maximum number of clean segments
-max_clean_segments	200
+max_clean_segments	20%
 
 # The argument of min_clean_segments and max_clean_segments can be
 # followed by a percent sign (%) or one of the following
@@ -28,7 +28,7 @@ max_clean_segments	200
 # ratio.
 
 # Clean segment check interval in seconds
-clean_check_interval	60
+clean_check_interval	10
 
 # Segment selection policy.
 # In NILFS version 2.0.0, only the timestamp policy is supported.
-- 
1.6.3.4

--
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

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

[David Arendt]

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

Hi,

here the updated patch

Thanks in advance
Bye,
David Arendt

On 04/06/10 18:06, Ryusuke Konishi wrote:
> Hi,
> On Mon, 05 Apr 2010 15:35:34 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>   
>> Hi,
>>
>> here is the patch
>>
>> Thanks in advance
>> Bye,
>> David Arendt
>>     
> Thanks.
>
> Looks functionally fine. Just a matter of coding style:
>
> The following part of cleaner loop looks bloated.  I think it's a good
> opportunity to divide it from the loop.
>
> Also, the length of their lines looks too long.  It's preferable if
> they're naturally wrapped within 80-columns.  This is not a must
> especially for userland tools, however kernel developers often prefer
> this conventional coding rule.  Separating the routine would make this
> easy.
>
>                 if (cleanerd->c_config.cf_min_clean_segments > 0) {
> ---> from
>                         if (cleanerd->c_running) {
>                                 if (sustat.ss_ncleansegs > cleanerd->c_config.c\
> f_max_clean_segments + r_segments) {
>                                         nilfs_cleanerd_clean_check_pause(cleane\
> rd, &timeout);
>                                         goto sleep;
>                                 }
>                         }
>                         else {
>                                 if (sustat.ss_ncleansegs < cleanerd->c_config.c\
> f_min_clean_segments + r_segments)
>                                         nilfs_cleanerd_clean_check_resume(clean\
> erd);
>                                 else
>                                         goto sleep;
>                         }
>
>                         if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_cl\
> ean_segments + r_segments) {
>                                 cleanerd->c_ncleansegs = cleanerd->c_config.cf_\
> mc_nsegments_per_clean;
>                                 cleanerd->c_cleaning_interval = cleanerd->c_con\
> fig.cf_mc_cleaning_interval;
>                         }
>                         else {
>                                 cleanerd->c_ncleansegs = cleanerd->c_config.cf_\
> nsegments_per_clean;
>                                 cleanerd->c_cleaning_interval = cleanerd->c_con\
> fig.cf_cleaning_interval;
>                         }
> <---- to
>                 }
>
> With regards,
> Ryusuke Konishi
>
>
>   
>> On 04/05/10 13:34, Ryusuke Konishi wrote:
>>     
>>> Hi,
>>> On Mon, 05 Apr 2010 09:50:11 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>>>   
>>>       
>>>> Hi,
>>>>
>>>> Actually I run with min_clean_segments at 250 and found that to be a
>>>> good value. However for example for a 2 gbyte usb key, this value would
>>>> not work at all, therefor I find it a good idea to set the default at
>>>> 10% as it would be more general for any device size as lots of people
>>>> simply try the defaults without changing configuration files.
>>>>     
>>>>         
>>> Ok, let's start with the 10% min_clean_segments for the default
>>> values.
>>>
>>>   
>>>       
>>>> I really like your idea with having a second set of
>>>> nsegments_per_clean and cleaning_interval_parameters for <
>>>> min_clean_segments. I am wondering if adaptive optimization will be
>>>> good, as I think different people will expect different behavior.
>>>> Someones might prefer the system using 100% io usage for cleaning
>>>> and the disk not getting full. Other ones might prefer the disk
>>>> getting full and using less io usage.
>>>>     
>>>>         
>>> Well, that's true.  Netbook users would prefer suspending the cleaner
>>> wherever possible to avoid their SSD worn out. Meanwhile, NAS users
>>> would expect it to operate safely to avoid trouble.
>>>
>>> For the present, let's try to handle the disk full issue by adding a
>>> few parameters effectively.
>>>
>>>   
>>>       
>>>> Therefor I think it would add a lot of parameters to the
>>>> configuration file for giving people the ability to tune it
>>>> correctly, and this would possibly complicate the configuration to
>>>> much.
>>>>     
>>>>         
>>>   
>>>       
>>>> If you decide for the second set of nsegments_per_clean and
>>>> cleaning_interval_parameters, please tell me if I should implement it or
>>>> if you will implement it, not that we are working on the same
>>>> functionality at the same time.
>>>>     
>>>>         
>>> I'll do the change of default values.  And will give over to you for
>>> this extension ;)
>>>
>>>   
>>>       
>>>> I think good names might be
>>>>
>>>> mc_nsegments_per_clean and
>>>> mc_cleaning_interval
>>>>
>>>> as this would be compatible with the current indentation in
>>>> nilfs_cleanerd.conf.
>>>>     
>>>>         
>>> All right.
>>>  
>>>   
>>>       
>>>> What would you take as default values ? As you always told that it would
>>>> be preferable to reduce cleaning_interval instead of increasing
>>>> nsegments_per_clean
>>>>     
>>>>         
>>> Yes, adjusting mc_cleaning_interval should come before increasing
>>> mc_nsegments_per_clean.  My image is, for example, as follows:
>>>
>>>   mc_cleaning_interval  1 or 2
>>>   mc_nsegments_per_clean  2 ~ 4 (as needed)
>>>
>>>   
>>>       
>>>> would you set cleaning interval to 0 in this case
>>>> causing permanent cleaning and leave nsegements_per_clean at or which
>>>> values would you choose ?
>>>>     
>>>>         
>>> The permanent cleaning may spoil responsiveness of the system.  I
>>> don't know.  Seems that we need some test.
>>>
>>> Thanks,
>>> Ryusuke Konishi
>>>
>>>   
>>>       
>>>> On 04/05/10 05:02, Ryusuke Konishi wrote:
>>>>     
>>>>         
>>>>> Hi!
>>>>> On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>>>>>>     
>>>>>>         
>>>>>>             
>>>>>>> Hi,
>>>>>>>
>>>>>>> here the changes
>>>>>>>
>>>>>>> Thank in advance,
>>>>>>> David Arendt
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> Looks fine to me.  Will apply later.
>>>>>>
>>>>>> Thanks for your quick work.
>>>>>>
>>>>>> Ryusuke Konishi
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> I enhanced your change so that min_clean_segments and
>>>>> max_clean_segments can be specified with a ratio (%) or an absolute
>>>>> value (MB, GB, and so on) of capacity.
>>>>>
>>>>> The change is available on the head of util's git repo.
>>>>>
>>>>> Now, my question is how we should set the default value of these
>>>>> parameters.  During test, I got disk full several times, and I feel
>>>>> min_free_segments = 100 is a bit tight.
>>>>>
>>>>> Of course this depends on the usage of each, but I think that the
>>>>> default values are desirable to have some generality (when possible).
>>>>>
>>>>> The following setting is my current idea for this.  How does it look?
>>>>>
>>>>>  min_clean_segments      10%
>>>>>  max_clean_segments      20%
>>>>>  clean_check_interval    10
>>>>>
>>>>> I also feel GC speed should be accelerated than now while the
>>>>> filesystem is close to disk full.  One simple method is adding
>>>>> optional nsegments_per_clean and cleaning_interval parameters for <
>>>>> min_clean_segments.  Or, some sort of adaptive acceleration should be
>>>>> applied.
>>>>>
>>>>> I'm planning to make the next util release after this settles down.
>>>>>
>>>>> Any idea?
>>>>>
>>>>> Thanks,
>>>>> Ryusuke Konishi
>>>>>   
>>>>>   
>>>>>       
>>>>>           
>>     


[-- Attachment #2: mc.patch --]
[-- Type: text/plain, Size: 9567 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-04-06 19:19:32.874081924 +0200
+++ nilfs2-utils/man/nilfs_cleanerd.conf.5	2010-04-06 19:24:23.027596397 +0200
@@ -56,9 +56,18 @@
 Specify the number of segments reclaimed by a single cleaning step.
 The default value is 2.
 .TP
+.B mc_nsegments_per_clean
+Specify the number of segments reclaimed by a single cleaning step
+if clean segments < min_clean_segments.
+The default value is 4.
+.TP
 .B cleaning_interval
 Specify the cleaning interval in seconds.  The default value is 5.
 .TP
+.B mc_cleaning_interval
+Specify the cleaning interval in seconds
+if clean segments < min_clean_segments.  The default value is 1.
+.TP
 .B retry_interval
 Specify retry interval in seconds.  This value provides the retry
 interval of GC in case of resource shortages.  The default value is
diff -ur nilfs2-utils.orig/sbin/cleanerd/cldconfig.c nilfs2-utils/sbin/cleanerd/cldconfig.c
--- nilfs2-utils.orig/sbin/cleanerd/cldconfig.c	2010-04-06 19:19:32.877083206 +0200
+++ nilfs2-utils/sbin/cleanerd/cldconfig.c	2010-04-06 19:24:23.027596397 +0200
@@ -405,6 +405,26 @@
 }
 
 static int
+nilfs_cldconfig_handle_mc_nsegments_per_clean(struct nilfs_cldconfig *config,
+					      char **tokens, size_t ntoks,
+					      struct nilfs *nilfs)
+{
+	unsigned long n;
+
+	if (nilfs_cldconfig_get_ulong_argument(tokens, ntoks, &n) < 0)
+		return 0;
+
+	if (n > NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX) {
+		syslog(LOG_WARNING, "%s: %s: too large, use the maximum value",
+		       tokens[0], tokens[1]);
+		n = NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN_MAX;
+	}
+
+	config->cf_mc_nsegments_per_clean = n;
+	return 0;
+}
+
+static int
 nilfs_cldconfig_handle_cleaning_interval(struct nilfs_cldconfig *config,
 					 char **tokens, size_t ntoks,
 					 struct nilfs *nilfs)
@@ -417,6 +437,18 @@
 }
 
 static int
+nilfs_cldconfig_handle_mc_cleaning_interval(struct nilfs_cldconfig *config,
+					    char **tokens, size_t ntoks,
+					    struct nilfs *nilfs)
+{
+	unsigned long sec;
+
+	if (nilfs_cldconfig_get_ulong_argument(tokens, ntoks, &sec) == 0)
+		config->cf_mc_cleaning_interval = sec;
+	return 0;
+}
+
+static int
 nilfs_cldconfig_handle_retry_interval(struct nilfs_cldconfig *config,
 				      char **tokens, size_t ntoks,
 				      struct nilfs *nilfs)
@@ -499,10 +531,18 @@
 		nilfs_cldconfig_handle_nsegments_per_clean
 	},
 	{
+		"mc_nsegments_per_clean", 2, 2,
+		nilfs_cldconfig_handle_mc_nsegments_per_clean
+	},
+	{
 		"cleaning_interval", 2, 2,
 		nilfs_cldconfig_handle_cleaning_interval
 	},
 	{
+		"mc_cleaning_interval", 2, 2,
+		nilfs_cldconfig_handle_mc_cleaning_interval
+	},
+	{
 		"retry_interval", 2, 2,
 		nilfs_cldconfig_handle_retry_interval
 	},
@@ -555,7 +595,10 @@
 	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_mc_nsegments_per_clean =
+		NILFS_CLDCONFIG_MC_NSEGMENTS_PER_CLEAN;
 	config->cf_cleaning_interval = NILFS_CLDCONFIG_CLEANING_INTERVAL;
+	config->cf_mc_cleaning_interval = NILFS_CLDCONFIG_MC_CLEANING_INTERVAL;
 	config->cf_retry_interval = NILFS_CLDCONFIG_RETRY_INTERVAL;
 	config->cf_use_mmap = NILFS_CLDCONFIG_USE_MMAP;
 	config->cf_log_priority = NILFS_CLDCONFIG_LOG_PRIORITY;
diff -ur nilfs2-utils.orig/sbin/cleanerd/cldconfig.h nilfs2-utils/sbin/cleanerd/cldconfig.h
--- nilfs2-utils.orig/sbin/cleanerd/cldconfig.h	2010-04-06 19:19:32.877083206 +0200
+++ nilfs2-utils/sbin/cleanerd/cldconfig.h	2010-04-06 19:24:23.028596802 +0200
@@ -78,7 +78,11 @@
  * @cf_max_clean_segments: high threshold on the number of free segments
  * @cf_clean_check_interval: cleaner check interval
  * @cf_nsegments_per_clean: number of segments reclaimed per clean cycle
+ * @cf_mc_nsegments_per_clean: number of segments reclaimed per clean cycle
+ * if clean segments < min_clean_segments
  * @cf_cleaning_interval: cleaning interval
+ * @cf_mc_cleaning_interval: cleaning interval
+ * if clean segments < min_clean_segments
  * @cf_use_mmap: flag that indicate using mmap
  * @cf_log_priority: log priority level
  */
@@ -89,7 +93,9 @@
 	__u64 cf_max_clean_segments;
 	time_t cf_clean_check_interval;
 	int cf_nsegments_per_clean;
+	int cf_mc_nsegments_per_clean;
 	time_t cf_cleaning_interval;
+	time_t cf_mc_cleaning_interval;
 	time_t cf_retry_interval;
 	int cf_use_mmap;
 	int cf_log_priority;
@@ -103,7 +109,9 @@
 #define	NILFS_CLDCONFIG_MAX_CLEAN_SEGMENTS		200
 #define	NILFS_CLDCONFIG_CLEAN_CHECK_INTERVAL		60
 #define NILFS_CLDCONFIG_NSEGMENTS_PER_CLEAN		2
+#define NILFS_CLDCONFIG_MC_NSEGMENTS_PER_CLEAN		4
 #define NILFS_CLDCONFIG_CLEANING_INTERVAL		5
+#define NILFS_CLDCONFIG_MC_CLEANING_INTERVAL		1
 #define NILFS_CLDCONFIG_RETRY_INTERVAL			60
 #define NILFS_CLDCONFIG_USE_MMAP			1
 #define NILFS_CLDCONFIG_LOG_PRIORITY			LOG_INFO
diff -ur nilfs2-utils.orig/sbin/cleanerd/cleanerd.c nilfs2-utils/sbin/cleanerd/cleanerd.c
--- nilfs2-utils.orig/sbin/cleanerd/cleanerd.c	2010-04-06 19:19:32.877083206 +0200
+++ nilfs2-utils/sbin/cleanerd/cleanerd.c	2010-04-06 19:24:23.050595046 +0200
@@ -158,6 +158,8 @@
 		}
 		cleanerd->c_ncleansegs =
 			cleanerd->c_config.cf_nsegments_per_clean;
+		cleanerd->c_cleaning_interval =
+			cleanerd->c_config.cf_cleaning_interval;
 	}
 	return ret;
 }
@@ -1136,13 +1138,13 @@
 	/* curr >= target */
 	if (!timercmp(&curr, &cleanerd->c_target, <)) {
 		cleanerd->c_target = curr;
-		cleanerd->c_target.tv_sec += config->cf_cleaning_interval;
+		cleanerd->c_target.tv_sec += cleanerd->c_cleaning_interval;
 		syslog(LOG_DEBUG, "adjust interval");
 		return 1; /* skip a sleep */
 	}
 	timersub(&cleanerd->c_target, &curr, &diff);
 	timeval_to_timespec(&diff, timeout);
-	cleanerd->c_target.tv_sec += config->cf_cleaning_interval;
+	cleanerd->c_target.tv_sec += cleanerd->c_cleaning_interval;
 	return 0;
 }
 
@@ -1177,6 +1179,41 @@
 	syslog(LOG_INFO, "resume (clean check)");
 }
 
+static int nilfs_cleanerd_handle_clean_check(struct nilfs_cleanerd *cleanerd,
+					     struct nilfs_sustat *sustat,
+					     int r_segments,
+					     struct timespec *timeout)
+{
+	if (cleanerd->c_running) {
+		if (sustat->ss_ncleansegs >
+		    cleanerd->c_config.cf_max_clean_segments + r_segments) {
+			nilfs_cleanerd_clean_check_pause(cleanerd, timeout);
+			return -1;
+		}
+	} else {
+		if (sustat->ss_ncleansegs <
+		    cleanerd->c_config.cf_min_clean_segments + r_segments)
+			nilfs_cleanerd_clean_check_resume(cleanerd);
+		else
+			return -1;
+	}
+
+	if (sustat->ss_ncleansegs <
+	    cleanerd->c_config.cf_min_clean_segments + r_segments) {
+		cleanerd->c_ncleansegs =
+			cleanerd->c_config.cf_mc_nsegments_per_clean;
+		cleanerd->c_cleaning_interval =
+			cleanerd->c_config.cf_mc_cleaning_interval;
+	} else {
+		cleanerd->c_ncleansegs =
+			cleanerd->c_config.cf_nsegments_per_clean;
+		cleanerd->c_cleaning_interval =
+			cleanerd->c_config.cf_cleaning_interval;
+	}
+
+	return 0;
+}
+
 /**
  * nilfs_cleanerd_clean_loop - main loop of the cleaner daemon
  * @cleanerd: cleanerd object
@@ -1218,6 +1255,7 @@
 		return -1;
 
 	cleanerd->c_ncleansegs = cleanerd->c_config.cf_nsegments_per_clean;
+	cleanerd->c_cleaning_interval = cleanerd->c_config.cf_cleaning_interval;
 
 	r_segments = nilfs_get_reserved_segments(cleanerd->c_nilfs);
 
@@ -1245,18 +1283,9 @@
 		}
 
 		if (cleanerd->c_config.cf_min_clean_segments > 0) {
-			if (cleanerd->c_running) {
-				if (sustat.ss_ncleansegs > cleanerd->c_config.cf_max_clean_segments + r_segments) {
-					nilfs_cleanerd_clean_check_pause(cleanerd, &timeout);
-					goto sleep;
-				}
-			}
-			else {
-				if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_clean_segments + r_segments)
-					nilfs_cleanerd_clean_check_resume(cleanerd);
-				else
-					goto sleep;
-			}
+			if (nilfs_cleanerd_handle_clean_check(
+				cleanerd, &sustat, r_segments, &timeout) < 0)
+				goto sleep;
 		}
 
 		if (sustat.ss_nongc_ctime != prev_nongc_ctime) {
diff -ur nilfs2-utils.orig/sbin/cleanerd/cleanerd.h nilfs2-utils/sbin/cleanerd/cleanerd.h
--- nilfs2-utils.orig/sbin/cleanerd/cleanerd.h	2010-04-06 19:19:32.878083926 +0200
+++ nilfs2-utils/sbin/cleanerd/cleanerd.h	2010-04-06 19:24:23.050595046 +0200
@@ -43,6 +43,7 @@
  * @c_running: running state
  * @c_fallback: fallback state
  * @c_ncleansegs: number of semgents cleaned per cycle
+ * @c_cleaning_interval: cleaning interval
  * @c_protcno: the minimum of checkpoint numbers within protection period
  * @c_prottime: start time of protection period
  * @c_target: target time for sleeping
@@ -54,6 +55,7 @@
 	int c_running;
 	int c_fallback;
 	int c_ncleansegs;
+	time_t c_cleaning_interval;
 	nilfs_cno_t c_protcno;
 	__u64 c_prottime;
 	struct timeval c_target;
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-04-06 19:19:32.878083926 +0200
+++ nilfs2-utils/sbin/cleanerd/nilfs_cleanerd.conf	2010-04-06 19:24:23.050595046 +0200
@@ -37,9 +37,17 @@
 # The maximum number of segments to be cleaned at a time.
 nsegments_per_clean	2
 
+# The maximum number of segments to be cleaned at a time
+# if clean segments < min_clean_segments
+mc_nsegments_per_clean	4
+
 # Cleaning interval in seconds.
 cleaning_interval	5
 
+# Cleaning interval in seconds
+# if clean segments < min_clean_segments
+mc_cleaning_interval	1
+
 # Retry interval in seconds.
 retry_interval		60
 

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

[Ryusuke Konishi]

Hi,
On Mon, 5 Apr 2010 14:35:51 +0200, Jan de Kruyf <jan.de.kruyf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hallo,
> May I break into this discussion with something perhaps partially
> connected to the thread:
> I had my usual disk full problem again on a half full partition after
> a power crash, that so far has not been explained yet.
> 
> Did any of you pick up anything in the cleanerd code that might point
> somewhere in the direction of a solution?
> 
> Regards
> 
> Jan de Kruyf.

Sorry for this.  Can you resend the 'crashedVar1.tgz' to this list (or
my CC'ed address) ?

Feel insufficient in clue to tweak cleanerd.

Regards,
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

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

[Ryusuke Konishi]

Hi,
On Tue, 06 Apr 2010 19:41:33 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> Hi,
> 
> here the updated patch
> 
> Thanks in advance
> Bye,
> David Arendt

Quick work ;)
Looks fine to me.

I'll apply it after some tests.

Thanks.
Ryusuke Konishi

 
> On 04/06/10 18:06, Ryusuke Konishi wrote:
> > Hi,
> > On Mon, 05 Apr 2010 15:35:34 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >   
> >> Hi,
> >>
> >> here is the patch
> >>
> >> Thanks in advance
> >> Bye,
> >> David Arendt
> >>     
> > Thanks.
> >
> > Looks functionally fine. Just a matter of coding style:
> >
> > The following part of cleaner loop looks bloated.  I think it's a good
> > opportunity to divide it from the loop.
> >
> > Also, the length of their lines looks too long.  It's preferable if
> > they're naturally wrapped within 80-columns.  This is not a must
> > especially for userland tools, however kernel developers often prefer
> > this conventional coding rule.  Separating the routine would make this
> > easy.
> >
> >                 if (cleanerd->c_config.cf_min_clean_segments > 0) {
> > ---> from
> >                         if (cleanerd->c_running) {
> >                                 if (sustat.ss_ncleansegs > cleanerd->c_config.c\
> > f_max_clean_segments + r_segments) {
> >                                         nilfs_cleanerd_clean_check_pause(cleane\
> > rd, &timeout);
> >                                         goto sleep;
> >                                 }
> >                         }
> >                         else {
> >                                 if (sustat.ss_ncleansegs < cleanerd->c_config.c\
> > f_min_clean_segments + r_segments)
> >                                         nilfs_cleanerd_clean_check_resume(clean\
> > erd);
> >                                 else
> >                                         goto sleep;
> >                         }
> >
> >                         if (sustat.ss_ncleansegs < cleanerd->c_config.cf_min_cl\
> > ean_segments + r_segments) {
> >                                 cleanerd->c_ncleansegs = cleanerd->c_config.cf_\
> > mc_nsegments_per_clean;
> >                                 cleanerd->c_cleaning_interval = cleanerd->c_con\
> > fig.cf_mc_cleaning_interval;
> >                         }
> >                         else {
> >                                 cleanerd->c_ncleansegs = cleanerd->c_config.cf_\
> > nsegments_per_clean;
> >                                 cleanerd->c_cleaning_interval = cleanerd->c_con\
> > fig.cf_cleaning_interval;
> >                         }
> > <---- to
> >                 }
> >
> > With regards,
> > Ryusuke Konishi
> >
> >
> >   
> >> On 04/05/10 13:34, Ryusuke Konishi wrote:
> >>     
> >>> Hi,
> >>> On Mon, 05 Apr 2010 09:50:11 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >>>   
> >>>       
> >>>> Hi,
> >>>>
> >>>> Actually I run with min_clean_segments at 250 and found that to be a
> >>>> good value. However for example for a 2 gbyte usb key, this value would
> >>>> not work at all, therefor I find it a good idea to set the default at
> >>>> 10% as it would be more general for any device size as lots of people
> >>>> simply try the defaults without changing configuration files.
> >>>>     
> >>>>         
> >>> Ok, let's start with the 10% min_clean_segments for the default
> >>> values.
> >>>
> >>>   
> >>>       
> >>>> I really like your idea with having a second set of
> >>>> nsegments_per_clean and cleaning_interval_parameters for <
> >>>> min_clean_segments. I am wondering if adaptive optimization will be
> >>>> good, as I think different people will expect different behavior.
> >>>> Someones might prefer the system using 100% io usage for cleaning
> >>>> and the disk not getting full. Other ones might prefer the disk
> >>>> getting full and using less io usage.
> >>>>     
> >>>>         
> >>> Well, that's true.  Netbook users would prefer suspending the cleaner
> >>> wherever possible to avoid their SSD worn out. Meanwhile, NAS users
> >>> would expect it to operate safely to avoid trouble.
> >>>
> >>> For the present, let's try to handle the disk full issue by adding a
> >>> few parameters effectively.
> >>>
> >>>   
> >>>       
> >>>> Therefor I think it would add a lot of parameters to the
> >>>> configuration file for giving people the ability to tune it
> >>>> correctly, and this would possibly complicate the configuration to
> >>>> much.
> >>>>     
> >>>>         
> >>>   
> >>>       
> >>>> If you decide for the second set of nsegments_per_clean and
> >>>> cleaning_interval_parameters, please tell me if I should implement it or
> >>>> if you will implement it, not that we are working on the same
> >>>> functionality at the same time.
> >>>>     
> >>>>         
> >>> I'll do the change of default values.  And will give over to you for
> >>> this extension ;)
> >>>
> >>>   
> >>>       
> >>>> I think good names might be
> >>>>
> >>>> mc_nsegments_per_clean and
> >>>> mc_cleaning_interval
> >>>>
> >>>> as this would be compatible with the current indentation in
> >>>> nilfs_cleanerd.conf.
> >>>>     
> >>>>         
> >>> All right.
> >>>  
> >>>   
> >>>       
> >>>> What would you take as default values ? As you always told that it would
> >>>> be preferable to reduce cleaning_interval instead of increasing
> >>>> nsegments_per_clean
> >>>>     
> >>>>         
> >>> Yes, adjusting mc_cleaning_interval should come before increasing
> >>> mc_nsegments_per_clean.  My image is, for example, as follows:
> >>>
> >>>   mc_cleaning_interval  1 or 2
> >>>   mc_nsegments_per_clean  2 ~ 4 (as needed)
> >>>
> >>>   
> >>>       
> >>>> would you set cleaning interval to 0 in this case
> >>>> causing permanent cleaning and leave nsegements_per_clean at or which
> >>>> values would you choose ?
> >>>>     
> >>>>         
> >>> The permanent cleaning may spoil responsiveness of the system.  I
> >>> don't know.  Seems that we need some test.
> >>>
> >>> Thanks,
> >>> Ryusuke Konishi
> >>>
> >>>   
> >>>       
> >>>> On 04/05/10 05:02, Ryusuke Konishi wrote:
> >>>>     
> >>>>         
> >>>>> Hi!
> >>>>> On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
> >>>>>   
> >>>>>       
> >>>>>           
> >>>>>> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
> >>>>>>     
> >>>>>>         
> >>>>>>             
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> here the changes
> >>>>>>>
> >>>>>>> Thank in advance,
> >>>>>>> David Arendt
> >>>>>>>       
> >>>>>>>           
> >>>>>>>               
> >>>>>> Looks fine to me.  Will apply later.
> >>>>>>
> >>>>>> Thanks for your quick work.
> >>>>>>
> >>>>>> Ryusuke Konishi
> >>>>>>     
> >>>>>>         
> >>>>>>             
> >>>>> I enhanced your change so that min_clean_segments and
> >>>>> max_clean_segments can be specified with a ratio (%) or an absolute
> >>>>> value (MB, GB, and so on) of capacity.
> >>>>>
> >>>>> The change is available on the head of util's git repo.
> >>>>>
> >>>>> Now, my question is how we should set the default value of these
> >>>>> parameters.  During test, I got disk full several times, and I feel
> >>>>> min_free_segments = 100 is a bit tight.
> >>>>>
> >>>>> Of course this depends on the usage of each, but I think that the
> >>>>> default values are desirable to have some generality (when possible).
> >>>>>
> >>>>> The following setting is my current idea for this.  How does it look?
> >>>>>
> >>>>>  min_clean_segments      10%
> >>>>>  max_clean_segments      20%
> >>>>>  clean_check_interval    10
> >>>>>
> >>>>> I also feel GC speed should be accelerated than now while the
> >>>>> filesystem is close to disk full.  One simple method is adding
> >>>>> optional nsegments_per_clean and cleaning_interval parameters for <
> >>>>> min_clean_segments.  Or, some sort of adaptive acceleration should be
> >>>>> applied.
> >>>>>
> >>>>> I'm planning to make the next util release after this settles down.
> >>>>>
> >>>>> Any idea?
> >>>>>
> >>>>> Thanks,
> >>>>> 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

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

[admin-/LHdS3kC8BfYtjvyW6yDsg]

Hi,

At the moment of submitting my cleaner patches, I didn't know if you would
use the new behavior as default behavior or the old one therefore in
documentation I had described that min_clean_segments = 0 would mean
normal cleaner behavior. However as the new behavior is now used as
default behavior, normal behavior might be confusing.

I am thinking about doing the following changes in documentation:

in nilfs_cleanerd.conf change

# Minium number of clean segments
# 0  = normal cleaner behaviour
# >0 = start cleaning if less segments are available
min_clean_segments      10%

to

# Minimum number of clean segments
#   0 = continuous cleaning
# > 0 = pause cleaning until less segments are available
min_clean_segments      10%

I just saw that I had a typo in the word minimum.

in nilfs_cleanerd.conf.8 change

.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.

to

.B min_clean_segments
Specify the minimum number of clean segments. A value of 0 means
continuous cleaning. A value greater than 0 means pause cleaning
until less than min_clean_segments are available.

What do you think about it ?

If you want to change, will you do changes or should I send you a patch ?

Thanks in advance,
Bye,
David Arendt

> Hi,
> On Tue, 06 Apr 2010 19:41:33 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>> Hi,
>>
>> here the updated patch
>>
>> Thanks in advance
>> Bye,
>> David Arendt
>
> Quick work ;)
> Looks fine to me.
>
> I'll apply it after some tests.
>
> Thanks.
> Ryusuke Konishi
>
>
>> On 04/06/10 18:06, Ryusuke Konishi wrote:
>> > Hi,
>> > On Mon, 05 Apr 2010 15:35:34 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org>
>> wrote:
>> >
>> >> Hi,
>> >>
>> >> here is the patch
>> >>
>> >> Thanks in advance
>> >> Bye,
>> >> David Arendt
>> >>
>> > Thanks.
>> >
>> > Looks functionally fine. Just a matter of coding style:
>> >
>> > The following part of cleaner loop looks bloated.  I think it's a good
>> > opportunity to divide it from the loop.
>> >
>> > Also, the length of their lines looks too long.  It's preferable if
>> > they're naturally wrapped within 80-columns.  This is not a must
>> > especially for userland tools, however kernel developers often prefer
>> > this conventional coding rule.  Separating the routine would make this
>> > easy.
>> >
>> >                 if (cleanerd->c_config.cf_min_clean_segments > 0) {
>> > ---> from
>> >                         if (cleanerd->c_running) {
>> >                                 if (sustat.ss_ncleansegs >
>> cleanerd->c_config.c\
>> > f_max_clean_segments + r_segments) {
>> >                                         nilfs_cleanerd_clean_check_pause(cleane\
>> > rd, &timeout);
>> >                                         goto sleep;
>> >                                 }
>> >                         }
>> >                         else {
>> >                                 if (sustat.ss_ncleansegs <
>> cleanerd->c_config.c\
>> > f_min_clean_segments + r_segments)
>> >                                         nilfs_cleanerd_clean_check_resume(clean\
>> > erd);
>> >                                 else
>> >                                         goto sleep;
>> >                         }
>> >
>> >                         if (sustat.ss_ncleansegs <
>> cleanerd->c_config.cf_min_cl\
>> > ean_segments + r_segments) {
>> >                                 cleanerd->c_ncleansegs =
>> cleanerd->c_config.cf_\
>> > mc_nsegments_per_clean;
>> >                                 cleanerd->c_cleaning_interval =
>> cleanerd->c_con\
>> > fig.cf_mc_cleaning_interval;
>> >                         }
>> >                         else {
>> >                                 cleanerd->c_ncleansegs =
>> cleanerd->c_config.cf_\
>> > nsegments_per_clean;
>> >                                 cleanerd->c_cleaning_interval =
>> cleanerd->c_con\
>> > fig.cf_cleaning_interval;
>> >                         }
>> > <---- to
>> >                 }
>> >
>> > With regards,
>> > Ryusuke Konishi
>> >
>> >
>> >
>> >> On 04/05/10 13:34, Ryusuke Konishi wrote:
>> >>
>> >>> Hi,
>> >>> On Mon, 05 Apr 2010 09:50:11 +0200, David Arendt <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org>
>> wrote:
>> >>>
>> >>>
>> >>>> Hi,
>> >>>>
>> >>>> Actually I run with min_clean_segments at 250 and found that to be
>> a
>> >>>> good value. However for example for a 2 gbyte usb key, this value
>> would
>> >>>> not work at all, therefor I find it a good idea to set the default
>> at
>> >>>> 10% as it would be more general for any device size as lots of
>> people
>> >>>> simply try the defaults without changing configuration files.
>> >>>>
>> >>>>
>> >>> Ok, let's start with the 10% min_clean_segments for the default
>> >>> values.
>> >>>
>> >>>
>> >>>
>> >>>> I really like your idea with having a second set of
>> >>>> nsegments_per_clean and cleaning_interval_parameters for <
>> >>>> min_clean_segments. I am wondering if adaptive optimization will be
>> >>>> good, as I think different people will expect different behavior.
>> >>>> Someones might prefer the system using 100% io usage for cleaning
>> >>>> and the disk not getting full. Other ones might prefer the disk
>> >>>> getting full and using less io usage.
>> >>>>
>> >>>>
>> >>> Well, that's true.  Netbook users would prefer suspending the
>> cleaner
>> >>> wherever possible to avoid their SSD worn out. Meanwhile, NAS users
>> >>> would expect it to operate safely to avoid trouble.
>> >>>
>> >>> For the present, let's try to handle the disk full issue by adding a
>> >>> few parameters effectively.
>> >>>
>> >>>
>> >>>
>> >>>> Therefor I think it would add a lot of parameters to the
>> >>>> configuration file for giving people the ability to tune it
>> >>>> correctly, and this would possibly complicate the configuration to
>> >>>> much.
>> >>>>
>> >>>>
>> >>>
>> >>>
>> >>>> If you decide for the second set of nsegments_per_clean and
>> >>>> cleaning_interval_parameters, please tell me if I should implement
>> it or
>> >>>> if you will implement it, not that we are working on the same
>> >>>> functionality at the same time.
>> >>>>
>> >>>>
>> >>> I'll do the change of default values.  And will give over to you for
>> >>> this extension ;)
>> >>>
>> >>>
>> >>>
>> >>>> I think good names might be
>> >>>>
>> >>>> mc_nsegments_per_clean and
>> >>>> mc_cleaning_interval
>> >>>>
>> >>>> as this would be compatible with the current indentation in
>> >>>> nilfs_cleanerd.conf.
>> >>>>
>> >>>>
>> >>> All right.
>> >>>
>> >>>
>> >>>
>> >>>> What would you take as default values ? As you always told that it
>> would
>> >>>> be preferable to reduce cleaning_interval instead of increasing
>> >>>> nsegments_per_clean
>> >>>>
>> >>>>
>> >>> Yes, adjusting mc_cleaning_interval should come before increasing
>> >>> mc_nsegments_per_clean.  My image is, for example, as follows:
>> >>>
>> >>>   mc_cleaning_interval  1 or 2
>> >>>   mc_nsegments_per_clean  2 ~ 4 (as needed)
>> >>>
>> >>>
>> >>>
>> >>>> would you set cleaning interval to 0 in this case
>> >>>> causing permanent cleaning and leave nsegements_per_clean at or
>> which
>> >>>> values would you choose ?
>> >>>>
>> >>>>
>> >>> The permanent cleaning may spoil responsiveness of the system.  I
>> >>> don't know.  Seems that we need some test.
>> >>>
>> >>> Thanks,
>> >>> Ryusuke Konishi
>> >>>
>> >>>
>> >>>
>> >>>> On 04/05/10 05:02, Ryusuke Konishi wrote:
>> >>>>
>> >>>>
>> >>>>> Hi!
>> >>>>> On Mon, 29 Mar 2010 16:39:02 +0900 (JST), Ryusuke Konishi
>> <ryusuke-sG5X7nlA6pw@public.gmane.org> wrote:
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>>> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt
>> <admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org> wrote:
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>>> Hi,
>> >>>>>>>
>> >>>>>>> here the changes
>> >>>>>>>
>> >>>>>>> Thank in advance,
>> >>>>>>> David Arendt
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>> Looks fine to me.  Will apply later.
>> >>>>>>
>> >>>>>> Thanks for your quick work.
>> >>>>>>
>> >>>>>> Ryusuke Konishi
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>> I enhanced your change so that min_clean_segments and
>> >>>>> max_clean_segments can be specified with a ratio (%) or an
>> absolute
>> >>>>> value (MB, GB, and so on) of capacity.
>> >>>>>
>> >>>>> The change is available on the head of util's git repo.
>> >>>>>
>> >>>>> Now, my question is how we should set the default value of these
>> >>>>> parameters.  During test, I got disk full several times, and I
>> feel
>> >>>>> min_free_segments = 100 is a bit tight.
>> >>>>>
>> >>>>> Of course this depends on the usage of each, but I think that the
>> >>>>> default values are desirable to have some generality (when
>> possible).
>> >>>>>
>> >>>>> The following setting is my current idea for this.  How does it
>> look?
>> >>>>>
>> >>>>>  min_clean_segments      10%
>> >>>>>  max_clean_segments      20%
>> >>>>>  clean_check_interval    10
>> >>>>>
>> >>>>> I also feel GC speed should be accelerated than now while the
>> >>>>> filesystem is close to disk full.  One simple method is adding
>> >>>>> optional nsegments_per_clean and cleaning_interval parameters for
>> <
>> >>>>> min_clean_segments.  Or, some sort of adaptive acceleration should
>> be
>> >>>>> applied.
>> >>>>>
>> >>>>> I'm planning to make the next util release after this settles
>> down.
>> >>>>>
>> >>>>> Any idea?
>> >>>>>
>> >>>>> Thanks,
>> >>>>> 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

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

[Ryusuke Konishi]

Hi,
On Wed, 7 Apr 2010 12:39:39 +0200, admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org wrote:
> Hi,
> 
> At the moment of submitting my cleaner patches, I didn't know if you would
> use the new behavior as default behavior or the old one therefore in
> documentation I had described that min_clean_segments = 0 would mean
> normal cleaner behavior. However as the new behavior is now used as
> default behavior, normal behavior might be confusing.

That's right.
 
> I am thinking about doing the following changes in documentation:
> 
> in nilfs_cleanerd.conf change
> 
> # Minium number of clean segments
> # 0  = normal cleaner behaviour
> # >0 = start cleaning if less segments are available
> min_clean_segments      10%
> 
> to
> 
> # Minimum number of clean segments
> #   0 = continuous cleaning
> # > 0 = pause cleaning until less segments are available
> min_clean_segments      10%
> 
> I just saw that I had a typo in the word minimum.
> 
> in nilfs_cleanerd.conf.8 change
> 
> .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.
> 
> to
> 
> .B min_clean_segments
> Specify the minimum number of clean segments. A value of 0 means
> continuous cleaning. A value greater than 0 means pause cleaning
> until less than min_clean_segments are available.
> 
> What do you think about it ?
> 
> If you want to change, will you do changes or should I send you a patch ?

Well, that change would be better.
Send me the patch, please?

Thanks,
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

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

[admin-/LHdS3kC8BfYtjvyW6yDsg]

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

Hi,

here the patch

Thanks in advance
Bye,
David Arendt

> Hi,
> On Wed, 7 Apr 2010 12:39:39 +0200, admin-/LHdS3kC8BfYtjvyW6yDsg@public.gmane.org wrote:
>> Hi,
>>
>> At the moment of submitting my cleaner patches, I didn't know if you
>> would
>> use the new behavior as default behavior or the old one therefore in
>> documentation I had described that min_clean_segments = 0 would mean
>> normal cleaner behavior. However as the new behavior is now used as
>> default behavior, normal behavior might be confusing.
>
> That's right.
>
>> I am thinking about doing the following changes in documentation:
>>
>> in nilfs_cleanerd.conf change
>>
>> # Minium number of clean segments
>> # 0  = normal cleaner behaviour
>> # >0 = start cleaning if less segments are available
>> min_clean_segments      10%
>>
>> to
>>
>> # Minimum number of clean segments
>> #   0 = continuous cleaning
>> # > 0 = pause cleaning until less segments are available
>> min_clean_segments      10%
>>
>> I just saw that I had a typo in the word minimum.
>>
>> in nilfs_cleanerd.conf.8 change
>>
>> .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.
>>
>> to
>>
>> .B min_clean_segments
>> Specify the minimum number of clean segments. A value of 0 means
>> continuous cleaning. A value greater than 0 means pause cleaning
>> until less than min_clean_segments are available.
>>
>> What do you think about it ?
>>
>> If you want to change, will you do changes or should I send you a patch
>> ?
>
> Well, that change would be better.
> Send me the patch, please?
>
> Thanks,
> Ryusuke Konishi
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: documentation.patch --]
[-- Type: text/x-diff; name="documentation.patch", Size: 1376 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-04-08 06:27:04.652095525 +0200
+++ nilfs2-utils/man/nilfs_cleanerd.conf.5	2010-04-08 12:33:39.940330908 +0200
@@ -27,8 +27,8 @@
 .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.
+continuous cleaning. A value greater than 0 means pause cleaning until
+less than min_clean_segments are available.
 .TP
 .B max_clean_segments
 Specify the maximum number of clean segments. If min_clean_segments is
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-04-08 06:27:04.653095645 +0200
+++ nilfs2-utils/sbin/cleanerd/nilfs_cleanerd.conf	2010-04-08 12:39:13.178765756 +0200
@@ -11,9 +11,9 @@
 # Protection period in second.
 protection_period	3600
 
-# Minium number of clean segments 
-# 0  = normal cleaner behaviour
-# >0 = start cleaning if less segments are available
+# Minimum number of clean segments
+#   0 = continuous cleaning
+# > 0 = pause cleaning until less segments are available
 min_clean_segments	10%
 
 # Maximum number of clean segments