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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]

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 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.

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).  But that at least is occasional,
unlike each log write.

Maybe it's not worth having O_APPEND, but I don't think the reasoning
above makes any real sense.

> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  log.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/log.c b/log.c
> index 6932885..dd25862 100644
> --- a/log.c
> +++ b/log.c
> @@ -204,9 +204,6 @@ out:
>   */
>  static int logfile_rotate(int fd, const struct timespec *now)
>  {
> -	if (fcntl(fd, F_SETFL, O_RDWR /* Drop O_APPEND: explicit lseek() */))
> -		return -errno;
> -
>  #ifdef FALLOC_FL_COLLAPSE_RANGE
>  	/* Only for Linux >= 3.15, extent-based ext4 or XFS, glibc >= 2.18 */
>  	if (!fallocate(fd, FALLOC_FL_COLLAPSE_RANGE, 0, log_cut_size))
> @@ -215,9 +212,6 @@ static int logfile_rotate(int fd, const struct timespec *now)
>  #endif
>  		logfile_rotate_move(fd, now);
>  
> -	if (fcntl(fd, F_SETFL, O_RDWR | O_APPEND))
> -		return -errno;
> -
>  	return 0;
>  }
>  
> @@ -416,7 +410,7 @@ void logfile_init(const char *name, const char *path, size_t size)
>  	if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0)
>  		die_perror("Failed to read own /proc/self/exe link");
>  
> -	log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC,
> +	log_file = open(path, O_CREAT | O_TRUNC | O_RDWR | O_CLOEXEC,
>  			S_IRUSR | S_IWUSR);
>  	if (log_file == -1)
>  		die_perror("Couldn't open log file %s", path);

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]