Hi,
On Sat, 27 Mar 2010 21:00:52 +0100, David Arendt <ad...@prnet.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, ad...@prnet.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 majord...@vger.kernel.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 majord...@vger.kernel.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
