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@xxxxxxxx>
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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
