Reduce unnecessary rename operations in prerotateSingleLog() by edycm · Pull Request #450 · logrotate/logrotate

edycm · 2022-06-11T10:32:08Z

config file:

[root@localhost logrotate]# cat logrotate.conf 
/root/logrotate_test/log/test.log
{
	su root root
	missingok
	size 100M
	rotate 3
	start 10
	copytruncate
	#dateext
	#dateformat _%Y%m%d_%s
}

before fixing:

[root@localhost logrotate]# ./logrotate logrotate.conf -fv
reading config file logrotate.conf
Reading state from file: /var/lib/logrotate.status
Allocating hash table for state file, size 64 entries
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state

Handling 1 logs

rotating pattern: /root/logrotate_test/log/test.log
 forced from command line (3 rotations)
empty log files are rotated, old logs are removed
considering log /root/logrotate_test/log/test.log
  Now: 2022-06-11 10:43
  Last rotated at 2022-06-11 10:40
  log needs rotating
rotating log /root/logrotate_test/log/test.log, log->rotateCount is 3
dateext suffix '-20220611'
glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
renaming /root/logrotate_test/log/test.log.12 to /root/logrotate_test/log/test.log.13 (rotatecount 3, logstart 10, i 12), 
old log /root/logrotate_test/log/test.log.12 does not exist
renaming /root/logrotate_test/log/test.log.11 to /root/logrotate_test/log/test.log.12 (rotatecount 3, logstart 10, i 11), 
old log /root/logrotate_test/log/test.log.11 does not exist
renaming /root/logrotate_test/log/test.log.10 to /root/logrotate_test/log/test.log.11 (rotatecount 3, logstart 10, i 10), 
renaming /root/logrotate_test/log/test.log.9 to /root/logrotate_test/log/test.log.10 (rotatecount 3, logstart 10, i 9), 
old log /root/logrotate_test/log/test.log.9 does not exist
renaming /root/logrotate_test/log/test.log.8 to /root/logrotate_test/log/test.log.9 (rotatecount 3, logstart 10, i 8), 
old log /root/logrotate_test/log/test.log.8 does not exist
renaming /root/logrotate_test/log/test.log.7 to /root/logrotate_test/log/test.log.8 (rotatecount 3, logstart 10, i 7), 
old log /root/logrotate_test/log/test.log.7 does not exist
renaming /root/logrotate_test/log/test.log.6 to /root/logrotate_test/log/test.log.7 (rotatecount 3, logstart 10, i 6), 
old log /root/logrotate_test/log/test.log.6 does not exist
renaming /root/logrotate_test/log/test.log.5 to /root/logrotate_test/log/test.log.6 (rotatecount 3, logstart 10, i 5), 
old log /root/logrotate_test/log/test.log.5 does not exist
renaming /root/logrotate_test/log/test.log.4 to /root/logrotate_test/log/test.log.5 (rotatecount 3, logstart 10, i 4), 
old log /root/logrotate_test/log/test.log.4 does not exist
renaming /root/logrotate_test/log/test.log.3 to /root/logrotate_test/log/test.log.4 (rotatecount 3, logstart 10, i 3), 
old log /root/logrotate_test/log/test.log.3 does not exist
renaming /root/logrotate_test/log/test.log.2 to /root/logrotate_test/log/test.log.3 (rotatecount 3, logstart 10, i 2), 
old log /root/logrotate_test/log/test.log.2 does not exist
renaming /root/logrotate_test/log/test.log.1 to /root/logrotate_test/log/test.log.2 (rotatecount 3, logstart 10, i 1), 
old log /root/logrotate_test/log/test.log.1 does not exist
renaming /root/logrotate_test/log/test.log.0 to /root/logrotate_test/log/test.log.1 (rotatecount 3, logstart 10, i 0), 
old log /root/logrotate_test/log/test.log.0 does not exist
log /root/logrotate_test/log/test.log.13 doesn't exist -- won't try to dispose of it
copying /root/logrotate_test/log/test.log to /root/logrotate_test/log/test.log.10
truncating /root/logrotate_test/log/test.log

after fixing:

[root@localhost logrotate]# ./logrotate logrotate.conf -fv
reading config file logrotate.conf
warning: state file /var/lib/logrotate.status is world-readable and thus can be locked from other unprivileged users. Skipping lock acquisition...
Reading state from file: /var/lib/logrotate.status
Allocating hash table for state file, size 64 entries
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state

Handling 1 logs

rotating pattern: /root/logrotate_test/log/test.log
 forced from command line (3 rotations)
empty log files are rotated, old logs are removed
considering log /root/logrotate_test/log/test.log
  Now: 2022-06-11 11:12
  Last rotated at 2022-06-11 10:43
  log needs rotating
rotating log /root/logrotate_test/log/test.log, log->rotateCount is 3
dateext suffix '-20220611'
glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
renaming /root/logrotate_test/log/test.log.12 to /root/logrotate_test/log/test.log.13 (rotatecount 3, logstart 10, i 12), 
old log /root/logrotate_test/log/test.log.12 does not exist
renaming /root/logrotate_test/log/test.log.11 to /root/logrotate_test/log/test.log.12 (rotatecount 3, logstart 10, i 11), 
renaming /root/logrotate_test/log/test.log.10 to /root/logrotate_test/log/test.log.11 (rotatecount 3, logstart 10, i 10), 
log /root/logrotate_test/log/test.log.13 doesn't exist -- won't try to dispose of it
copying /root/logrotate_test/log/test.log to /root/logrotate_test/log/test.log.10
truncating /root/logrotate_test/log/test.log

kdudka · 2022-06-13T06:35:55Z

Thanks for the patch! The existing code copes better with the case where start N is introduced after a few rotations without it but it is probably not much needed in practice. As I understand it the start directive was introduced mainly to be able to count logs from zero rather than one, which is the default.

kdudka · 2022-06-13T07:12:27Z

Actually the patch changes logrotate's behavior even when the start directive is not used.

@cgzones, should we rather use i >= logStart - 1 to preserve compatibility or is it fine to optimize the most common case, too?

cgzones · 2022-06-13T13:49:25Z

We should probably define how logrotate should behave if the start directive gets changed and there are preexisting rotated files.

Currently if test.log and test.log.0 exist and rotation is done with config

/tmp/logtest/test.log {
        rotate 3
        start 1
}

it results in

reading config file config
Creating stub state file: state
Reading state from file: state
Allocating hash table for state file, size 64 entries

Handling 1 logs

rotating pattern: /tmp/logtest/test.log  forced from command line (3 rotations)
empty log files are rotated, old logs are removed
considering log /tmp/logtest/test.log
Creating new state
  Now: 2022-06-13 15:43
  Last rotated at 2022-06-13 15:00
  log needs rotating
rotating log /tmp/logtest/test.log, log->rotateCount is 3
dateext suffix '-20220613'
glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
set default create context to xuser_u:object_r:user_tmp_t:s0
renaming /tmp/logtest/test.log.3 to /tmp/logtest/test.log.4 (rotatecount 3, logstart 1, i 3), 
old log /tmp/logtest/test.log.3 does not exist
renaming /tmp/logtest/test.log.2 to /tmp/logtest/test.log.3 (rotatecount 3, logstart 1, i 2), 
old log /tmp/logtest/test.log.2 does not exist
renaming /tmp/logtest/test.log.1 to /tmp/logtest/test.log.2 (rotatecount 3, logstart 1, i 1), 
old log /tmp/logtest/test.log.1 does not exist
renaming /tmp/logtest/test.log.0 to /tmp/logtest/test.log.1 (rotatecount 3, logstart 1, i 0), 
log /tmp/logtest/test.log.4 doesn't exist -- won't try to dispose of it
set default create context to xuser_u:object_r:user_tmp_t:s0
renaming /tmp/logtest/test.log to /tmp/logtest/test.log.1
set default create context to xuser_u:object_r:user_tmp_t:s0

and afterwards only test.log.1 exists (with the content of test.log) and the rotated test.log.0 got lost.

If we don't care about preexisting files then this optimization (although probably not noticeable in practice) seems OK.

kdudka · 2022-06-14T10:12:24Z

Right. My proposal is not really an improvement. Not touching test.log.0, unless start 0 is used, seems better. As for caring about preexisting files, logrotate does not care much about them when we change the rotate count either. So the optimization seems fine after all.

Reduce unnecessary rename operations in prerotateSingleLog()

f295547

kdudka closed this in dfb3034 Jun 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce unnecessary rename operations in prerotateSingleLog()#450

Reduce unnecessary rename operations in prerotateSingleLog()#450
edycm wants to merge 1 commit intologrotate:masterfrom
edycm:master

edycm commented Jun 11, 2022

Uh oh!

kdudka commented Jun 13, 2022 •

edited

Loading

Uh oh!

kdudka commented Jun 13, 2022

Uh oh!

cgzones commented Jun 13, 2022

Uh oh!

kdudka commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

edycm commented Jun 11, 2022

Uh oh!

kdudka commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdudka commented Jun 13, 2022

Uh oh!

cgzones commented Jun 13, 2022

Uh oh!

kdudka commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kdudka commented Jun 13, 2022 •

edited

Loading