Re: [PATCH v3 5/9] log: Don't use O_APPEND at all

On Tue, 29 Oct 2024 15:20:56 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Mon, Oct 28, 2024 at 11:00:40AM +0100, Stefano
Brivio wrote:
  We open the log file with O_APPEND, but switch it
off before seeking,
 and turn it back on afterwards.
 
 We never seek when O_APPEND is on, so we don't actually need it, as
 its only function is to override the offset for writes so that they
 are always performed at the end regardless of the current offset
 (which is at the end anyway, for us).   
 
 Sorry, this sounded fishy to me on the call, but I figured I was just
 missing something.  But looking at this the reasoning doesn't make
 sense to me.
 
 We don't seek with O_APPEND, but we do write(), which is exactly where
 it matters. AIUI the point of O_APPEND is that if you have multiple
 processes writing to the same file, they won't clobber each others
 writes because of a stale file pointer. 
That's not the reason why I originally added it though: it was there
because I thought I would lseek() to do the rotation and possibly end
up with the cursor somewhere before the end. Then restart writing, and
the write would happen in the middle of the file:

$ cat append.c
#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <fcntl.h>

int main(int argc, char **argv)
{
	int flags = O_CREAT | O_TRUNC | O_WRONLY | ((argc == 3) ? O_APPEND : 0);
	int fd = open(argv[1], flags, S_IRUSR | S_IWUSR);
	char buf[BUFSIZ];

	memset(buf, 'a', BUFSIZ);
	write(fd, buf, 10);
	lseek(fd, 1, SEEK_SET);
	memset(buf, 'b', BUFSIZ);
	write(fd, buf, 10);
	write(fd, (char *){ "\n" }, 1);

	return 0;
}
$ gcc -o append{,.c}
$ ./append test append
$ cat test
aaaaaaaaaabbbbbbbbbb
$ ./append test
$ cat test
abbbbbbbbbb

...
  That's usually not
 _necessary_ for us as such, but it's perhaps valuable since it reduces
 the likelihood of data loss if somehow you do get two instances
 logging to the same file. 
The result will be completely unreadable anyway, so I don't think it
matters for us.

...
  Of course the rotation process *can* clobber things
(which is exactly
 why I was always a bit sceptical of this "in place" rotation, not that
 we really have other options). 
Why would it clobber things? logfile_rotate_fallocate() and
logfile_rotate_move() take care of cutting cleanly at a line boundary,
and tests check that.

-- 
Stefano