Improve module file reading and writing #758

tskeith · 2019-09-24T00:25:39Z

Use flock() to lock .mod files during reading and writing to prevent
writing while a read is in progress and reading or writing when a write
is in progress. To achieve this, change module file writing to use C
stdio so that the file descriptor is readily available. errno seems
to be set more reliably as well.

Change module file reading to only read the file once; we used to
read it to verify the checksum and then again to parse it.
Instead, change Parsing so that we can get the file contents
after Prescan() and use that to verify the checksum. Also, it has
a mechanism for searching directories for files, so make use of that
instead of duplicating that functionality in ModFileReader.
This requires some changes to how errors are returned so they can
be reported in the right place. The "isModuleFile" flag is passed in
to SourceFile so that file locking can be done only for module files.

sscalpone · 2019-09-24T03:16:01Z

I'm not too clear on the reason for the locking. Isn't a mod file written to a tmp file & then, if the tmp mod file is different from the original, the tmp is renamed? If that's the case, then any process that has the old file open will still be accessing the old file even after the old file is removed.

jeanPerier

Looks good. I have the same question as Steve regarding when such file conflicts can happen. It looks OK to me as additional safety though since it seems to have a small code footprint.

lib/semantics/mod-file.cc

tskeith · 2019-09-24T14:33:48Z

I'm not too clear on the reason for the locking. Isn't a mod file written to a tmp file & then, if the tmp mod file is different from the original, the tmp is renamed? If that's the case, then any process that has the old file open will still be accessing the old file even after the old file is removed.

No, the mod file is written directly to its destination, which is why locking is necessary but a temp file is not.

What you're suggesting is:

If the file exists, read it and compare to what will be written
Write to a temp
Rename temp to correct name (rename() from stdio seems to be the only way to do this)

That does seem simpler -- I'll try it that way and see.

lib/parser/source.cc

lib/semantics/mod-file.cc

tskeith · 2019-09-24T23:24:56Z

I've pushed a new version that uses a temp file instead of file locking. I'm interested in opinions on which is better.

klausler

I like the use of a (presumably) atomic rename more than locking.

lib/parser/parsing.h

lib/semantics/mod-file.cc

psteinfeld

I don't understand much of the code, but what I understand looks good to me.

Fix problems with writing a mod file while another compilation is reading or writing. Write to a temp and then rename it: - compute the new contents of the .mod file - if it already exists, check if it is already correct - if not, write new contents to a temp file - rename the temp to the final destination `mkstemps()` seems to be the best way to create the temp file. It returns a file descriptor, so change the rest of the mod file writing to use POSIX open/read/write/close. This seems to set errno more reliably too. There is some extra work around creating the temp to make it have the same directory and suffix as the final file (so that if one gets left behind by a crash, "rm *.mod" still cleans it up). `mkstemps()` creates file with 0600 permissions so try to change it to what it would have been if we just wrote the file. Change module file reading to only read the file once; we used to read it to verify the checksum and then again to parse it. Instead, change `Parsing` so that we can get the file contents after `Prescan()` and use that to verify the checksum. Also, it has a mechanism for searching directories for files, so make use of that instead of duplicating that functionality in `ModFileReader`. This requires some changes to how errors are returned so they can be reported in the right place.

The tests run by `test_any.sh` don't redirect their .mod files to a different directory so they occasionally fail when the same file is accessed by different tests at the same time. With locking of module files implemented, this problem reproduces much more reliably Work around it by changing the module names to be distinct. Also remove some comments left over when copied from `test_symbols.sh` tests.

Fix problems with writing a mod file while another compilation is reading or writing. Write to a temp and then rename it: - compute the new contents of the .mod file - if it already exists, check if it is already correct - if not, write new contents to a temp file - rename the temp to the final destination `mkstemps()` seems to be the best way to create the temp file. It returns a file descriptor, so change the rest of the mod file writing to use POSIX open/read/write/close. This seems to set errno more reliably too. There is some extra work around creating the temp to make it have the same directory and suffix as the final file (so that if one gets left behind by a crash, "rm *.mod" still cleans it up). `mkstemps()` creates file with 0600 permissions so try to change it to what it would have been if we just wrote the file. Change module file reading to only read the file once; we used to read it to verify the checksum and then again to parse it. Instead, change `Parsing` so that we can get the file contents after `Prescan()` and use that to verify the checksum. Also, it has a mechanism for searching directories for files, so make use of that instead of duplicating that functionality in `ModFileReader`. This requires some changes to how errors are returned so they can be reported in the right place. Original-commit: flang-compiler/f18@d0d5497 Reviewed-on: flang-compiler/f18#758 Tree-same-pre-rewrite: false

The tests run by `test_any.sh` don't redirect their .mod files to a different directory so they occasionally fail when the same file is accessed by different tests at the same time. With locking of module files implemented, this problem reproduces much more reliably Work around it by changing the module names to be distinct. Also remove some comments left over when copied from `test_symbols.sh` tests. Original-commit: flang-compiler/f18@f7b5c5f Reviewed-on: flang-compiler/f18#758

…/tsk-modfiles Improve module file reading and writing Original-commit: flang-compiler/f18@b5f48df Reviewed-on: flang-compiler/f18#758

Fix problems with writing a mod file while another compilation is reading or writing. Write to a temp and then rename it: - compute the new contents of the .mod file - if it already exists, check if it is already correct - if not, write new contents to a temp file - rename the temp to the final destination `mkstemps()` seems to be the best way to create the temp file. It returns a file descriptor, so change the rest of the mod file writing to use POSIX open/read/write/close. This seems to set errno more reliably too. There is some extra work around creating the temp to make it have the same directory and suffix as the final file (so that if one gets left behind by a crash, "rm *.mod" still cleans it up). `mkstemps()` creates file with 0600 permissions so try to change it to what it would have been if we just wrote the file. Change module file reading to only read the file once; we used to read it to verify the checksum and then again to parse it. Instead, change `Parsing` so that we can get the file contents after `Prescan()` and use that to verify the checksum. Also, it has a mechanism for searching directories for files, so make use of that instead of duplicating that functionality in `ModFileReader`. This requires some changes to how errors are returned so they can be reported in the right place. Original-commit: flang-compiler/f18@d0d5497 Reviewed-on: flang-compiler/f18#758 Tree-same-pre-rewrite: false

The tests run by `test_any.sh` don't redirect their .mod files to a different directory so they occasionally fail when the same file is accessed by different tests at the same time. With locking of module files implemented, this problem reproduces much more reliably Work around it by changing the module names to be distinct. Also remove some comments left over when copied from `test_symbols.sh` tests. Original-commit: flang-compiler/f18@f7b5c5f Reviewed-on: flang-compiler/f18#758

tskeith requested review from jeanPerier, vjayathirtha-nv, klausler and psteinfeld September 24, 2019 00:25

jeanPerier approved these changes Sep 24, 2019

View reviewed changes

lib/semantics/mod-file.cc Outdated Show resolved Hide resolved

lib/semantics/mod-file.cc Show resolved Hide resolved

klausler reviewed Sep 24, 2019

View reviewed changes

lib/parser/source.cc Outdated Show resolved Hide resolved

klausler reviewed Sep 24, 2019

View reviewed changes