From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202410 header.b=E3pi0ALa; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id B421D5A004E for ; Tue, 29 Oct 2024 05:25:31 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1730175927; bh=T/709/5mLDruEnoQG51FrZ0fdj1BhluIumOPVRAYA14=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=E3pi0ALasGXsLaaE5ZCgu/+Zd8tn5kiHQflmY5s7ZzJ8jQznAL83gVxz8lDvx7cSb t7+OW5Yv0fFem2HDs4ug9bW6nIxNf1gUX57XhCO25WST5u9k1xvJ9eYkigwzRr7iPB 8GpPWpUkew0wbfqjQ9VvQcXRBzFRACfXQnmMm3kOLR9RtcgsjTJhWXj+Mef8WoqkFp SGf2YDB3HV/rwcp1RW1HqvknLdWbI+ovVGxrqL5mvGAgxuo2cDX8B7R7qJkadJA4MP U7VPvbrU+V9noQZFRtBWGq7puIv8JmK2QAPNJbPQbz+II1IjTr+DUE0C/AotWelzLj XifGHKIB3Z6yQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Xcxxz66wxz4x8S; Tue, 29 Oct 2024 15:25:27 +1100 (AEDT) Date: Tue, 29 Oct 2024 15:20:56 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 5/9] log: Don't use O_APPEND at all Message-ID: References: <20241028100044.939714-1-sbrivio@redhat.com> <20241028100044.939714-6-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="V0uBEMURAD1p7azh" Content-Disposition: inline In-Reply-To: <20241028100044.939714-6-sbrivio@redhat.com> Message-ID-Hash: IOJJ2I7WV7UIQLEINPDXPUYHT5VMOTZU X-Message-ID-Hash: IOJJ2I7WV7UIQLEINPDXPUYHT5VMOTZU X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --V0uBEMURAD1p7azh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > Signed-off-by: Stefano Brivio > --- > log.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) >=20 > 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 >=3D 3.15, extent-based ext4 or XFS, glibc >=3D 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 timesp= ec *now) > #endif > logfile_rotate_move(fd, now); > =20 > - if (fcntl(fd, F_SETFL, O_RDWR | O_APPEND)) > - return -errno; > - > return 0; > } > =20 > @@ -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"); > =20 > - log_file =3D open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEX= EC, > + log_file =3D open(path, O_CREAT | O_TRUNC | O_RDWR | O_CLOEXEC, > S_IRUSR | S_IWUSR); > if (log_file =3D=3D -1) > die_perror("Couldn't open log file %s", path); --=20 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 --V0uBEMURAD1p7azh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmcgYqcACgkQzQJF27ox 2GeeyhAAiHTwPisFobCDJonxela03NCK6b6yWXJI6Vw56rHe9NS0M3IFa3S65m93 9zpoZEQfdl486FVuwnlfwEvkFpbrEGsSK336BF6gZtN64xrXlAJbE3NU8JJCVpT4 rCoVFSjdk49YpPfY6cYqCzg1xm/KuEAwktkJLyu55gsFZB3+OSy4i+yImmUtJaJm rzZ6w4sO6epj+7r+B8CXhH311iXWx7MuFeWJuXiz7XC0TDSMjq8JJGTCaNETZEBs Uk5fqR2sOKp1zNnRD2rw9ED7Vo4ifuijuUjaaCZYwFOdvqbvxVOUmoj2cfyM+Hr4 JCfn7TFv9jwL5viZ6VrkNlvlGlZmsbIjza+fGw1xlRdZvBwrW1hugaT1bwKC9gt8 ACNnMbYGSktla1cJZ/Q5V8uFM3mq0TYb+Nk2Nsh0TqaD2nV8/+oZ44yAETwLpRAK Ynca4TQlPYHEduyjE+l0CeLguEmCgOsJz+ZN5S6eYRa34N/GoOltz4uOqEsunh/R i7XC3Uldd6sGiH78iYNMfv7pXhZsQzmFCgOnMpnQxix4hEHzmJKXghG1OG46TDn1 6//B1wrSyeNfRJSt0QADhw0l0bkjp/Ikod5eHAeK7cE0dBx4LzrTPUVbabOCKlGI UF5f6AdNxXNCLTU8okRtFgtTi88RDcH3zr57JiBx36+9S6zigOQ= =gDIj -----END PGP SIGNATURE----- --V0uBEMURAD1p7azh--