romio Type_create_darray (mirroring previous Type_create_darray fix)

[markalle]

The romio code for Type_create_darray had the same bug I fixed a while back in the main MPI lib's Type_create_darray. Basically the cyclic() function is the main function that's considered right, and block() is an optimization that left out a necessary piece of code to resize the datatype it created.

For what it's worth I took the conservative approach and made block() match cyclic(). I like the code a bit better in the main MPI library (ompi_datatype_create_darray.c) that uses opal_datatype_resize() where the implementation of cyclic() matches the current MPI standard's description. The romio code uses LB/UB and Type_struct, and I'm presuming that matched the older MPI standard's description of Type_create_darray.

Anyway I didn't change to the new style, I just made block() match cyclic() in the romio code.

I don't have a complete testcase handy, but can make one if needed. Basically it involves creating a new datatype with Type_create_darray (mixing BLOCK and CYCLIC) and then

    MPI_File_set_view(fd, 0, MPI_INT, newtype, "native", MPI_INFO_NULL);
    MPI_File_write(fd, sbuf, count, MPI_INT, MPI_STATUS_IGNORE);

and then you can read from the file or hexdump it etc and the data will be written in the wrong locations.

[markalle]

This is the previous fix for the Type_create_darray in the main MPI lib:
#3672

[markalle]

bot:retest

[markalle]

bot:retest

[hppritcha]

bot:ompi:retest

[hppritcha]

bot:mellanox:retest

[hppritcha]

bot:ompi:retest

[markalle]

And here's a testcase that can be used to see the original bug and that this fixes it:
https://gist.github.com/markalle/940de93d64fd779e304ee124855b8e6a
mpicc -o x darray_bug_romio.c
mpirun -mca io romio321 -np 1 ./x

[edgargabriel]

I just downloaded the branch, compiled the code, but the testcase that you point to is still not passing for me. I double checked that the modification that is shown in this patch is in fact in the code (which is correct).

gabriel@salmon:~/tmp> mpicc -o darray_bug_romio darray_bug_romio.c
gabriel@salmon:~/tmp> mpirun --mca io romio321 -np 1 ./darray_bug_romio
[pt2pt] what locations in rbuf appear to have data:
data at disp 0 (sz 8)
data at disp 16 (sz 8)
data at disp 64 (sz 8)
data at disp 80 (sz 8)
data at disp 96 (sz 8)
data at disp 112 (sz 8)
data at disp 160 (sz 8)
data at disp 176 (sz 8)
[romio] what locations in rbuf appear to have data:
[romio] data not found in expected locations -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1

Not sure how to proceed on this.

[gpaulsen]

@markalle This PR is really old. Is it still needed on master? Can we just close this?
I'm adding WorkInProgress label until we can verify if this PR is still desirable.

[awlauria]

@markalle do we still want this? Status?

[lanl-ompi]

Can one of the admins verify this patch?

[jsquyres]

Hey @markalle Is this PR still needed?

[markalle]

I repushed. Over in Spectrum MPI I had made several related fixes toward syncing up the Count and Flatten functions, but that's been a loosing battle that keeps re-breaking. So in this version I'm taking the simpler fix where Count is only used to allocate the arrays that are filled by Flatten, and even then I put an extra safety valve to resize those arrays if needed (I don't think that code ever gets activated though). Anyway the final flat->count is then reset based on the *curr_index which I think is the best book-keeping variable for keeping track of the count since it was used every time data is written into those arrays

[rhc54]

As I stated on the user mailing list, this PR should not be committed. It modifies an imported software package without a corresponding upstream fix. This leaves the OMPI community responsible for maintaining ROMIO - a position we cannot absorb.

This PR should instead be filed against ROMIO itself, and then monitored for acceptance so we can update OMPI once ROMIO is updated.

[gpaulsen]

Mark has submitted a ROMIO PR upstream here: pmodels/mpich#4943
we still need to go through their contributor process, so it may take some time.

[markalle]

So what about OMPI 4.0.x? That presumably won't get a romio update, so once 4943 goes into mpich I should plan to to merge that into 4.0.x?

[gpaulsen]

Hopefully once the fix gets into romio, they will spin a bugfix release on whatever branch v4.0.x's ROMIO is based on and then we can pull that into v4.0.x But I'm not ceratin of the details or the branch mappings between projects.

[markalle]

I'll close this one. The overall status of getting things into MPICH and getting them back from mpich into our branches is far from done, but this one is at least fixed in MPICH master so it can be closed here. And the job of making sure we have a merge from MPICH master of important fixes back to our various branches is ongoing.