Hi,
On Mon, 05 Apr 2010 15:35:34 +0200, David Arendt <admin@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxx> wrote:
> >>>
> >>>
> >>>> On Mon, 29 Mar 2010 06:35:27 +0200, David Arendt <admin@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
