SIONlib issueshttps://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues2019-12-02T16:03:43+01:00https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/186_sion_filedesc is probably a god object2019-12-02T16:03:43+01:00Benedikt Steinbuschb.steinbusch@fz-juelich.de_sion_filedesc is probably a god object... or rather, it would be one, if C was an object oriented language. It contains everything and at the same time encapsulates almost nothing. It represents all of these:
- a single physical file with a single logical file opened,
- a s...... or rather, it would be one, if C was an object oriented language. It contains everything and at the same time encapsulates almost nothing. It represents all of these:
- a single physical file with a single logical file opened,
- a single physical file with multiple (or all) of its logical files opened,
- multiple physical files with mulitple (or all) of their logical files opened.
To make matters worse, at any given time, only a single logical file in a single physical file out of these collections is in focus. When switching focus (which commonly happens when seeking, but also during open and close), the meta-data of the newly selected file (be it logical and/or physical) is copied to special fields of `_sion_filedesc` and the meta-data of the no longer in focus file is written back to the fields it was previously copied from. There are however no functions that do this. These copies are done ad-hoc, every time, see, e.g., [sion_seek](https://trac.version.fz-juelich.de/SIONlib/browser/trunk/src/lib/sion_internal_seek.c?rev=2345#L57) or [sion_generic_paropen_mapped](https://trac.version.fz-juelich.de/SIONlib/browser/trunk/src/parlib/sion_generic_mapped.c?rev=2345#L474).
Separating the different aspects of `_sion_filedesc` into multiple types and encapsulating copy operations (or not copying in the first place) would help cut down on redundant code and clarify responsibilities.https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/185Error handling by deadlock2019-12-02T16:03:43+01:00Benedikt Steinbuschb.steinbusch@fz-juelich.deError handling by deadlockWhen return codes of fallible functions are not ignored (see #181) they are often handled by immediately performing clean up (or not, see #184) and returning early. This is better than ignoring the errors, but has serious drawbacks in th...When return codes of fallible functions are not ignored (see #181) they are often handled by immediately performing clean up (or not, see #184) and returning early. This is better than ignoring the errors, but has serious drawbacks in the parallel parts of SIONlib. It is not necessarily the case that all errors will always manifest globally (i.e. on all processes). This leads to some (but not all) processes returning early from functions that later try to perform collective operations leading to a deadlock. Considering that in the context of HPC, users are often billed for compute time by the wall clock, this is a particularly poor error handling strategy.
Better strategies would be to make error handling collective (if any process encounters an error, all return early) or abort the program (harsh, but probably saves a lot of wasted compute time).https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/188sion_flush does not flush buffers2021-08-25T15:20:29+02:00Benedikt Steinbuschb.steinbusch@fz-juelich.desion_flush does not flush buffers`sion_flush()` does not flush the I/O buffers as one would expect from its name but rather updates the internal blocksize array which is being removed as part of SIONlib 2.0. Should `sion_flush()` be re-dedicated to flushing buffers or s...`sion_flush()` does not flush the I/O buffers as one would expect from its name but rather updates the internal blocksize array which is being removed as part of SIONlib 2.0. Should `sion_flush()` be re-dedicated to flushing buffers or should it be removed now that it is a no-op?https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/163Incorrect intent declarations in Fortran bindings2019-12-02T16:03:43+01:00Benedikt Steinbuschb.steinbusch@fz-juelich.deIncorrect intent declarations in Fortran bindingsAnke Kreuzer recently reported a segmentation fault originating in a call to `fsion_paropen_mpi`.
> Hallo Benedikt,
>
> also der Code rund um das FSION_PAROPEN_MPI sieht so aus aus:
>
> fsblksize = -1
> chunksize = INT(16+nt*(96+...Anke Kreuzer recently reported a segmentation fault originating in a call to `fsion_paropen_mpi`.
> Hallo Benedikt,
>
> also der Code rund um das FSION_PAROPEN_MPI sieht so aus aus:
>
> fsblksize = -1
> chunksize = INT(16+nt*(96+120*ndl),KIND=8)
>
> IF(myid==0) THEN
> WRITE(*,*) " Before create_Com"
> ENDIF
> color = crank/24
> key = modulo(crank,24);
> ! WRITE(*,*) "My id: ",myid , "color: ", color, "key: ", key
>
> CALL MPI_COMM_SPLIT(MPI_COMM_WORLD, color, myid,
> & lcomm, ierr)
> IF(myid==0) THEN
> WRITE(*,*) " After create_Com, before SION_PAROPEN_MPI"
> ENDIF
>
> So, bis hier hin laeuft das Programm durch und dann kommt:
>
> CALL FSION_PAROPEN_MPI('NAM_CPs',
> & 'bw',-1,icomm0,lcomm,
> & chunksize, fsblksize, myid, newfname, sid)
>
> IF(myid==0) THEN
> WRITE(*,*) " After paropen"
> ENDIF
>
> Und diese letzte Ausgabe wird nie ausgegeben, da vorher der Segmentation
> fault passiert.
The problem is that SIONlib tries to return the number of files it actually opened (based on the number of distinct local communicators) in the `nfiles` argument. However, the special value `-1` is specified as an integer literal and as the dummy argument `nfiles` is declared with `intent(in)` is probably placed in a read-only part of memory.
A quick inspection reveals that the `intent` declarations on the Fortran subroutine and the argument const-ness on the C function that is called underneath differ in several places.
```
subroutine fsion_paropen_mpi(fname,file_mode,nfiles,fgcomm,flcomm,chunksizes,fsblksize,&
& globalrank,newfname,sid)
implicit none
character(len=*), intent(in) :: fname
character(len=*), intent(in) :: file_mode
integer, intent(in) :: nfiles
integer, intent(in) :: fgcomm
integer, intent(inout) :: flcomm
integer*8, intent(inout) :: chunksizes
integer*4, intent(inout) :: fsblksize
integer, intent(in) :: globalrank
character(len=*), intent(out) :: newfname
integer, intent(out) :: sid
call fsion_paropen_mpi_c(fname,file_mode,nfiles,fgcomm,flcomm,chunksizes,fsblksize,&
& globalrank,newfname,sid)
end subroutine fsion_paropen_mpi
```
```
void fsion_paropen_mpi_c(char *fname,
char *file_mode,
int *numFiles,
MPI_Fint * fgComm,
MPI_Fint * flComm,
sion_int64 *chunksize,
sion_int32 *fsblksize,
int *globalrank,
char *newfname,
int *sid,
int fname_len,
int file_mode_len,
int newfname_len);
```https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/184Audit open and close functions for memory leaks2019-12-02T16:03:43+01:00Benedikt Steinbuschb.steinbusch@fz-juelich.deAudit open and close functions for memory leaksAddress sanitizer indicates that some open and close functions leak memory on the error paths (e.g. when called with incorrect parameters, as some tests do). A more thorough audit should be performed.Address sanitizer indicates that some open and close functions leak memory on the error paths (e.g. when called with incorrect parameters, as some tests do). A more thorough audit should be performed.https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/183Apply hints when doing collective I/O2021-08-25T15:20:43+02:00Benedikt Steinbuschb.steinbusch@fz-juelich.deApply hints when doing collective I/OBefore resolving #159, hints were applied by calling `sion_ensure_free_space` on every task. This is currently not done anymore. One possible way to re-enable this would be to extend the protocol of the iterators in `sion_internal_positi...Before resolving #159, hints were applied by calling `sion_ensure_free_space` on every task. This is currently not done anymore. One possible way to re-enable this would be to extend the protocol of the iterators in `sion_internal_positioning.{c|h}` to return an `enum` rather than `bool` and use that to indicate a jump to a new block. (Or keep the boolean and return a true tagged union `enum ??? { Remainder { length: i64 }, Jump { position: i64, length: i64 } }`.) Then on every jump to a different block either the sender or the collector can take action (apply hints).https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/182_sion_file_set_position_ansi and _sion_file_get_position_ansi use POSIX funct...2021-08-25T15:20:29+02:00Benedikt Steinbuschb.steinbusch@fz-juelich.de_sion_file_set_position_ansi and _sion_file_get_position_ansi use POSIX functions on some platformsE.g., on Linux, said functions reach around the C standard `FILE*` via `fileno()` and modify/inspect the position of the POSIX file descriptor directly via `lseek()`. This leads to confusing results by making the buffering that C standar...E.g., on Linux, said functions reach around the C standard `FILE*` via `fileno()` and modify/inspect the position of the POSIX file descriptor directly via `lseek()`. This leads to confusing results by making the buffering that C standard I/O performs observable, e.g. in a sequence `read -> set_position -> read` the second `read` can return bytes that were read by the first `read` and still remain in the I/O buffer.
This behavior is not consistent with POSIX (where no buffering is performed) or the C standard library (where `fseek` transparently flushes buffer as necessary) or even with itself (on mac OS (Darwin) `_sion_file_set_position_ansi` is implemented via `fseek`).
To paper over the inconsistency most (but not all) uses of `_sion_file_set_position()` and `_sion_file_get_position()` in SIONlib are prefixed with an explicit `_sion_file_flush()`.
Changing the implementation of the functions in question to use `fseek` and `ftello` like on mac OS does not break anything. In fact, after doing so, it is possible to remove `_sion_file_flush` from all sequences `_sion_file_flush -> _sion_file_{get|set}_position` without breaking any tests.
== Question
Is there a reason for `_sion_file_{get|set}_position_ansi` being implemented the way they are or can they be changed to be more consistent?https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/136Use more recent fortran for f90 interface2019-12-02T16:03:43+01:00Kay ThustUse more recent fortran for f90 interfaceE.g. integer*8 is f77 style, use selected_int_kind insteadE.g. integer*8 is f77 style, use selected_int_kind insteadhttps://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/134Add convenience functions to access filedesc members2021-08-25T15:20:29+02:00Kay ThustAdd convenience functions to access filedesc membersE.g. sion_get_chunksize
Alternatively fix sion_get_locations.E.g. sion_get_chunksize
Alternatively fix sion_get_locations.2.0.0https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/133Clean up debugging information2019-12-02T16:03:57+01:00Kay ThustClean up debugging information- `DFUNCTION` should be used consistently
- "Enter" debug messages should be the first
- "Exit" should be the last, if possible also for cases where the function does not exit successful- `DFUNCTION` should be used consistently
- "Enter" debug messages should be the first
- "Exit" should be the last, if possible also for cases where the function does not exit successfulhttps://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/129Add sion_paropen_mapped_omp2019-04-12T14:18:44+02:00Kay ThustAdd sion_paropen_mapped_ompmapped omp is still missingmapped omp is still missingWolfgang FringsWolfgang Fringshttps://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/115Add platform library for adapting SIONlib's behavior to the platform it runs on2019-04-12T14:16:26+02:00Kay ThustAdd platform library for adapting SIONlib's behavior to the platform it runs onFormally known as hardware libFormally known as hardware libWolfgang FringsWolfgang Fringshttps://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/193Hybrid API too simple for heterogeneous systems(?)2021-02-12T08:03:35+01:00Benedikt Steinbuschb.steinbusch@fz-juelich.deHybrid API too simple for heterogeneous systems(?)The hybrid API currently flattens the 2D identifiers `(mpi_rank, omp_thread_num)` into a 1D space as follows:
```
sionlib_rank = mpi_rank * num_threads + omp_thread_num
```
This assumes that there is a single `num_threads` that is vali...The hybrid API currently flattens the 2D identifiers `(mpi_rank, omp_thread_num)` into a 1D space as follows:
```
sionlib_rank = mpi_rank * num_threads + omp_thread_num
```
This assumes that there is a single `num_threads` that is valid across all MPI processes. On heterogeneous systems (or in heterogeneous applications, think MPMD) this is not necessarily the case.
As a first step, this should be mentioned in the documentation. In the future, this limitation should be lifted by allowing arbitrary numbers of threads on each process.https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/110Check debug levels and usage of DFUNCTION2019-12-02T16:03:57+01:00Kay ThustCheck debug levels and usage of DFUNCTIONhttps://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/154SIONlib 2.0 API changes2021-08-25T15:20:29+02:00Benedikt Steinbuschb.steinbusch@fz-juelich.deSIONlib 2.0 API changes# `open` functions
## Current API
Some parts of the current API combine many functions into one. This makes it easy to write down advanced operations concisely, but arguably makes it harder than necessary for the user to get started do...# `open` functions
## Current API
Some parts of the current API combine many functions into one. This makes it easy to write down advanced operations concisely, but arguably makes it harder than necessary for the user to get started doing easy things at first. Take for example the following function
```
int sion_paropen_mpi(
const char *fname,
const char *file_mode,
int *numFiles,
MPI_Comm gComm,
const MPI_Comm *lComm,
sion_int64 *chunksize,
sion_int32 *fsblksize,
int *globalrank,
FILE **fileptr,
char **newfname
)
```
For each of these 10 function arguments, the user has to find out:
* the argument `intent`:
* some are `in` parameters,
* some are `out` parameters,
* some are both,
* some are marked incorrectly;
* whether he has to supply a value or not:
* some of the arguments are mandatory,
* some are optional with different special values to signal the absence of a value (e.g. `NULL` vs `-1`);
* does it interact with other arguments:
* some arguments have intricate relationships to one another, where they can only be used in certain combinations (e.g. `numFiles` and `lComm`).
## Proposed Changes
In the branch `DEV-BS-API2` I propose some structural changes to the API that I hope simplify the getting started scenario by deferring some of these questions to a later point during the development of a project that wants to make use of SIONlib. To that end, I have introduced a type `sion_mpi_options` (see https://trac.version.fz-juelich.de/SIONlib/browser/branches/DEV-BS-API2/src/parlib/sion_mpi.h#L94) that encapsulates the argument list of `sion_paropen_mpi`. It is initialized using a function that takes all arguments that are strictly necessary to open a file (except for rank, which I recently found out is an unnecessary `out` argument):
```
sion_mpi_options sion_mpi_options_new(
const char *filename,
sion_open_mode mode,
MPI_Comm communicator,
int rank // not strictly necessary, see ticket #153
); // ignoring rank, that is three parameters to fopen's two
```
All other members are initialised to sensible default values, i.e. `fsblksize` is detected from the file system, `chunksize` is chosen accordingly (a "correct" chunksize is no longer necessary with the `hide-chunks` changes) and the `file_number` is set to 1...
Setter functions are used to override default values, e.g. `sion_mpi_options_set_chunksize` or `sion_mpi_options_set_multifile_number`.
Once the user has modified the options object to his satisfaction, it is used as the sole argument of `sion_mpi_paropen_from_options`. This design is inspired by the Builder pattern (https://en.wikipedia.org/wiki/Builder_pattern)
Along with these changes to the `open` function, an exhaustive list of getter functions for file properties needs to be offered, to query things like `newfname` in cases where these are of interest to the user.
### An Alternative
Alternatively, the responsibilities could be split slightly differently, encapsulating only the optional arguments in the `sion_mpi_options` type. The open function would then look like this:
```
int sion_paropen_mpi_with_options(
const char *filename,
sion_open_mode mode,
MPI_Comm communicator,
const sion_mpi_options *options
);
```
And the user can pass in `NULL` for `options` if he is fine with the defaults and thus does not have to come into contact with the `sion_mpi_options` type and associated functions until he needs them. There could also be an even simpler function to do just this:
```
int sion_paropen_mpi(const char *filename, sion_open_mode mode, MPI_Comm communicator) {
sion_paropen_mpi_with_options(filename, mode, communicator, NULL);
}
```
## Pro
All in all, these changes help answer or defer the questions mentioned earlier:
* **Argument intent:** When opening a file, all arguments have intent `in`. Dynamic properties of open files can later be queried by the user through getter functions (in case he is interested).
* **Optional arguments:** Mandatory arguments have to be provided. Optional arguments are hidden inside the `sion_mpi_options` type. Instead of having to find out what special value signals the absence of an argument, the user can simply ignore them.
* **Argument interactions:** The setter functions of the `sion_mpi_options` type can be used to enforce constraints between arguments, see `sion_mpi_options_set_multifile_{number|communicator}`.
## Contra
Although these changes can make it easier to get into working with SIONlib at first by hiding some of the complexity, it does in turn hurt the visibility of some advanced mechanisms. E.g., in the current API, the `file_number` argument explicitly appears in the function signature of `sion_mpi_paropen` alerting the user to the presence of this feature, whereas in the proposed API, a user can successfully open a file and use it without learning about the `multifile` mechanism and as a consequence miss potential performance improvements.
## Open Questions
* Can this mechanism replace all arguments of `open`, especially those that are currently encoded in the mode string?
* Does this mechanism work for the other `open` flavors (serial, OpenMP, hybrid)?
* ...?
# `seek` and file position in general
## Current API
Currently, a location inside a SIONlib container is identified by a triple of (`rank`, `blocknr`, `position_in_block`).
```
int sion_seek(
int sid,
int rank,
int blocknr,
sion_int64 position_in_block
)
```
Once the continuous write work has been completed, the model of a logical file in a SIONlib container can be changed from a sequence of chunks each with individual fill amount to a sequence of `n-1` chunks containing `chunksize` bytes and a last chunk containing the remaining `size(logical file) - (n - 1) * chunksize` bytes. In this picture the block numbers can be de-emphasized in favour of absolute offsets into the logical file (which are straightforward to calculate) or relative offsets from the current file pointer position. This would make SIONlib logical files and their API more similar to the naive task-local files they are trying to replace.
## Proposed Changes
The names and documentation of the function arguments could be changed, renaming the `blocknr` argument to `whence` as found on conventional `seek` functions and more emphasis would be placed on the special values `SION_ABSOLUTE_POS` and `SION_END_POS` as well as another special value `SION_SEEK_RELATIVE` that allows searching relative to the current position, but across block boundaries. The `position_in_block` argument would be renamed to `position`.
### A step further
Alternatively, to further de-emphasize the concept of blocks, the option to seek by blocks could be removed from `sion_seek` altogether and be moved to a different function e.g. `sion_seek_block`. This simplifies `sion_seek` for use cases where block boundaries do not matter. Furthermore, this enables a more versatile family of seek by block operations, such as relative seek by block (e.g. seek to the beginning of the next block, after writing a header of metadata at the beginning of a file).
### Yet further
The interface of `sion_seek` could further be simplified by removing the `rank` parameter (which, with the changes above, would make the interface as good as identical to a conventional `seek` function). This functionality could be moved to a separate function, e.g. `sion_seek_logical_file`. This simplifies common use cases where opening a SIONlib container means opening a single logical file inside of it, e.g. the (non-mapped) `sion_paropen...` functions or `sion_open_rank` where seeking by rank is not possible anyways.
## Open Questions
- Which alternative should we decide on?2.0.0https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/145Split up Fortran interface libraries2019-12-02T16:03:42+01:00Benedikt Steinbuschb.steinbusch@fz-juelich.deSplit up Fortran interface librariesCurrently, the OpenMP part of the Fortran interface is included in the serial library, while the hybrid part is included in the MPI library.
Split up the Fortran interface into eight libraries: `(F77, F90) x (serial, omp, mpi, ompi)`Currently, the OpenMP part of the Fortran interface is included in the serial library, while the hybrid part is included in the MPI library.
Split up the Fortran interface into eight libraries: `(F77, F90) x (serial, omp, mpi, ompi)`https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/141Raising an error in _sion_paropen_generic_one_file lets buddy checkpointing hang2019-12-02T16:03:42+01:00Kay ThustRaising an error in _sion_paropen_generic_one_file lets buddy checkpointing hangRaising an error, e.g. line 159 in [src/parlib/sion_generic_internal.c@2030](src/parlib/sion_generic_internal.c@2030) causes tests for buddy checkpointing to hang.Raising an error, e.g. line 159 in [src/parlib/sion_generic_internal.c@2030](src/parlib/sion_generic_internal.c@2030) causes tests for buddy checkpointing to hang.https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/140Add checks for malloc(0)2019-12-02T16:03:42+01:00Kay ThustAdd checks for malloc(0)Check all circumstances where `malloc(0)` could occur and handle them properly since it is implementation dependent.
Usually this should not be an issue, since zero sized allocs should be prevented by other logic, e.g. size of global co...Check all circumstances where `malloc(0)` could occur and handle them properly since it is implementation dependent.
Usually this should not be an issue, since zero sized allocs should be prevented by other logic, e.g. size of global communicator > 0.https://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/82build python interface2019-12-02T16:03:56+01:00Kay Thustbuild python interfacesionconfig needs additional information: pythonpath, pythonversionsionconfig needs additional information: pythonpath, pythonversionhttps://gitlab.jsc.fz-juelich.de/cstao-public/SIONlib/SIONlib/-/issues/80Error codes for unsuccessful read / write calls2021-08-25T15:20:43+02:00Kay ThustError codes for unsuccessful read / write callsreported by Bert Wesarg
> Das Problem ist wohl eher, das wir gerne zw. 'erfolgreichem lesen, auch wenn es zu wenig war' und 'fehler beim lesen' untscheiden wollen. Mit der ASNI API kann man das gut mit ferror() unterscheiden. Aber bei s...reported by Bert Wesarg
> Das Problem ist wohl eher, das wir gerne zw. 'erfolgreichem lesen, auch wenn es zu wenig war' und 'fehler beim lesen' untscheiden wollen. Mit der ASNI API kann man das gut mit ferror() unterscheiden. Aber bei sion_fread_key() sehe ich da jetzt nicht eine moeglichkeit.
Und später
> Auch bei ANSI kann man anhand des Rükgabewertes nicht entscheiden, ob es ein error oder ein end-of-file ist. Ob nun die Anzahl der Bytes oder der Elemente zurück gegeben werden, spielt da keine Rolle. Ich sehe also gerade nicht, wie diese Information vorher verfügbar hätte sein könnte. POSIX auf der anderen Seite gibt einem -1 wieder, in Falle eines Fehlers.2.0.0