On 2014-02-06 15:37, Andreas Rohner wrote:
> On 2014-02-06 02:16, Ryusuke Konishi wrote:
>> On Wed, 5 Feb 2014 03:16:39 +0100, Andreas Rohner wrote:
>>> This patch adds a flag, that enables the GC to skip the timeout in
>>> certain situations. For example if the cleaning of some segments
>>> was deferred to a later time, then no real progress has been
>>> made. Apart from reading a few summary blocks, no data was read or
>>> written to disk. In this situation it makes sense to skip the
>>> normal timeout once and immediately try to clean the next set of
>>> segments.
>>>
>>> Unfortunately it is not possible, to directly continue the cleaning
>>> loop, because this would lead to an unresponsive GC process.
>>> Therefore the timeout is simply set to 0 seconds.
>>>
>>> Signed-off-by: Andreas Rohner <andreas.rohner@xxxxxxx>
>>> ---
>>> sbin/cleanerd/cleanerd.c | 27 ++++++++++++++++-----------
>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
>>> index 1374e38..e1bd558 100644
>>> --- a/sbin/cleanerd/cleanerd.c
>>> +++ b/sbin/cleanerd/cleanerd.c
>>> @@ -167,6 +167,7 @@ struct nilfs_cleanerd {
>>> int running;
>>> int fallback;
>>> int retry_cleaning;
>>> + int no_timeout;
>>
>> Corresponding comment is missing for this one, too.
>>
>>> int shutdown;
>>> long ncleansegs;
>>> struct timeval cleaning_interval;
>>> @@ -894,7 +895,7 @@ static int nilfs_cleanerd_recalc_interval(struct nilfs_cleanerd *cleanerd,
>>> interval = nilfs_cleanerd_cleaning_interval(cleanerd);
>>> /* timercmp() does not work for '>=' or '<='. */
>>> /* curr >= target */
>>> - if (!timercmp(&curr, &cleanerd->target, <)) {
>>> + if (!timercmp(&curr, &cleanerd->target, <) || cleanerd->no_timeout) {
>>> cleanerd->timeout.tv_sec = 0;
>>> cleanerd->timeout.tv_usec = 0;
>>> timeradd(&curr, interval, &cleanerd->target);
>>> @@ -1395,6 +1396,7 @@ static int nilfs_cleanerd_clean_segments(struct nilfs_cleanerd *cleanerd,
>>> "number: %m");
>>> goto out;
>>> }
>>> + cleanerd->no_timeout = 0;
>>
>> This one is needed for the else case of nilfs_cleanerd_clean_loop() ?
>>
>> if (ns > 0) {
>> ret = nilfs_cleanerd_clean_segments(
>> cleanerd, segnums, ns, sustat.ss_prot_seq,
>> prottime, &ndone);
>> if (ret < 0)
>> return -1;
>> } else {
>> cleanerd->retry_cleaning = 0;
>> + cleanerd->no_timeout = 0;
>> }
>
> It is important to make sure that no_timeout is set to 1 for only one
> iteration of nilfs_cleanerd_clean_loop. Otherwise it would permanently
> disable the timeout. But you are right, this is probably not a good
> place to do it. In version 5 I moved it.
Sorry I misunderstood you there. Yes it is definitely needed in the else
case of nilfs_cleanerd_clean_loop. In version 5 I moved it to the top of
the loop altogether.
Regards,
Andreas Rohner
--
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
