On Wed, 30 Oct 2024 13:33:43 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Tue, Oct 29, 2024 at 11:23:29AM +0100, Stefano Brivio wrote:...but if it kind of works for multiple writers, we shouldn't prevent that usage, right? On the other hand, I don't think we should try to make that usage all nice and supported because we would need a prefix, which, in case of a single writer, just adds noise and size. And I don't think we want to detect if there are multiple writers... So, all in all, I would choose to spend no effort and leave like it is, until somebody comes up with a use case in one direction or the other. -- StefanoOn Tue, 29 Oct 2024 20:32:40 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Ok, that makes sense. Except that maybe there is a reason to use O_APPEND (the multiple writer thing), even if it's not the one you thought of initially. [snip]On Tue, Oct 29, 2024 at 09:48:50AM +0100, Stefano Brivio wrote:I initially opened it with O_APPEND because I _thought_ I would set the offset to a possibly inconsistent value around the rotation. Then I dropped O_APPEND around the rotation, forgetting about the initial reason why I added it at all. So it makes no sense to have 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:I don't entirely follow. I see why you disable O_APPEND across the rotation, but I'm not clear on why it's opened with O_APPEND in the first place, if it's not for the typical logging reason.That's fair. I wonder if it might make sense to flock() the logfile, to (somewhat) enforce that only one process uses it at a time.Ah, sure. But I think that supporting multiple writers would need more work anyway (at least adding a prefix as you mentioned).> 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.I mean that in the case that there are multiple writers, the rotation breaks that "no data loss, and probably readable-ish" property of O_APPEND.