Bug 510 - Multiple processes writing to zone stats file overwrite each other's data
Multiple processes writing to zone stats file overwrite each other's data
Status: RESOLVED FIXED
Product: NSD
Classification: Unclassified
Component: NSD Code
3.2.x
i386 Linux
: P5 normal
Assigned To: NSD team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-21 20:35 CEST by Simon Arlott
Modified: 2014-01-08 13:48 CET (History)
1 user (show)

See Also:


Attachments
Patch to fix this issue (advisory file locking does not work on NFS) (1.13 KB, patch)
2013-07-21 20:42 CEST, Simon Arlott
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Arlott 2013-07-21 20:35:49 CEST
With "server-count: 8" , 2 CPU cores and zone stats enabled, the multiple processes are competing with each other to write to the zone stats file resulting in overwritten output:

XSTATS example.com. 1374420446 RR=0 RNXD=0 RFwdR=0 RDupR=0 RFail=0 RFErr=0 RErr=0 RAXFR=0 RLame=0 ROpts=0 SSysQ=0 SAns=0 SFwdQ=0 SDupQ=0 SErr=0 RQ=0 RIQ=0 RFwdQ=0 RDupQ=0 RTCP=0 SFwdR=0 SFail=0 SFErr=0 SNaAns=0 SNXD=0 RUQ=0 RURQ=0 RUXFR=0 RUUpd=NSTATS example.in-addr.arpa. 1374420446 SOA=1

fwrite() is not atomic, and fopen() in append mode will not result in the correct position for appending when there are concurrent writes.
Comment 1 Simon Arlott 2013-07-21 20:42:55 CEST
Created attachment 229 [details]
Patch to fix this issue (advisory file locking does not work on NFS)

Locking the stats file will be a problem for servers with a large number of zones or processes.

Ideally the main process needs to co-ordinate running stats for one server process at a time.
Comment 2 Matthijs Mekking 2013-12-04 11:36:39 CET
Hi, 

You are right, this needs a fix and file locking is not the most elegant way to do this. We can do one of the following:

1. Accept your patch, despite this is will not scale well with a large number of zones. 

2. Let the main process coordinate which child process is allowed running stats, but this does also not scale well, since it creates a lot of ipc overhead.

3. Replace fopen() and fwrite() with open() and write() (per process running stats there will be one write to the file). Although write() is atomic, open() with O_APPEND may still lead to corrupted files on NFS file systems in this case (because NFS does not support appending to a file).

4. Write to a different file per process to avoid the race condition. 

I am tempted to implement solution 3 or 4. What are your thoughts about these?
Comment 3 Matthijs Mekking 2013-12-04 11:49:00 CET
A fifth proposal by Jaap is the following:

5. Log stats to syslog instead of files and let syslogd coordinate things.
Comment 4 Simon Arlott 2013-12-04 13:32:05 CET
(In reply to comment #2)
> 1. Accept your patch, despite this is will not scale well with a large
> number of zones. 

I'm using this patch. Anyone with a large number of zones will already have a problem of busy processes every stats interval without the locking.

> 2. Let the main process coordinate which child process is allowed running
> stats, but this does also not scale well, since it creates a lot of ipc
> overhead.

This is a good solution, but see solution 6 for another option.

> 3. Replace fopen() and fwrite() with open() and write() (per process running
> stats there will be one write to the file). Although write() is atomic,
> open() with O_APPEND may still lead to corrupted files on NFS file systems
> in this case (because NFS does not support appending to a file).

write() is not guaranteed to be atomic.

> 4. Write to a different file per process to avoid the race condition. 

This would create a lot more files so it doesn't really help anyone with a large number of zones.

(In reply to comment #3)
> A fifth proposal by Jaap is the following:
> 
> 5. Log stats to syslog instead of files and let syslogd coordinate things.

I'd still use the file option.


6. Lock the file but don't write the stats at the sime time, if you have 8 processes and the stats interval is 1 hour, write stats for 1 process every 450 seconds.
Comment 5 Matthijs Mekking 2013-12-04 15:33:18 CET
(In reply to comment #4)
> 6. Lock the file but don't write the stats at the sime time, if you have 8
> processes and the stats interval is 1 hour, write stats for 1 process every
> 450 seconds.

Hm, I don't like that. The zone stats are written at the same time the bind8 stats are written to syslog and I don't want to spread out the stats: They should be grouped together.
Comment 6 Matthijs Mekking 2013-12-04 15:44:46 CET
(In reply to comment #4)
> > 4. Write to a different file per process to avoid the race condition. 
> 
> This would create a lot more files so it doesn't really help anyone with a
> large number of zones.

Note this is one file per process, not per zone. So in your case, this would create 8 files.
Comment 7 Matthijs Mekking 2014-01-07 13:33:17 CET
If no further objections, I am going to implement option 4
Comment 8 Matthijs Mekking 2014-01-08 13:48:53 CET
r4114