Skip to content

lib: fix reading GRASS vrts with multiple threads#7201

Open
metzm wants to merge 4 commits intoOSGeo:mainfrom
metzm:vrt_openmp
Open

lib: fix reading GRASS vrts with multiple threads#7201
metzm wants to merge 4 commits intoOSGeo:mainfrom
metzm:vrt_openmp

Conversation

@metzm
Copy link
Copy Markdown
Contributor

@metzm metzm commented Mar 20, 2026

Description

Parallelised reading of the real raster maps constituting a GRASS virtual raster causes IO read errors and segmentation faults. This PR enforces reading of the different rasters in only one thread.

Someone with better knowledge of OpenMP directives than me should have a look at this PR. At least for me this PR fixes the read errors.

How to reproduce

  1. create tiles with r.tile
  2. build a GRASS vrt from these tiles with r.buildvrt
  3. run r.univar on this vrt with nprocs > 1

This also fixes #6616

@metzm metzm added this to the 8.6.0 milestone Mar 20, 2026
@metzm metzm added bug Something isn't working raster Related to raster data processing C Related code is in C libraries backport to 8.4 PR needs to be backported to release branch 8.4 backport to 8.5 PR needs to be backported to release branch 8.5 labels Mar 20, 2026
Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I think this is the direction we should go for these functions at this point. I'm assuming it works although I did not test this.

There is still a very slight risk of a race accessing the fileinfo to get vrt, so I suggest modifying the code to account for that. We already know that raster open/close is the problem, so I'm suggesting changes aligned with that.

The rule I'm using is that everything accessing fileinfo is in danger. For standard rasters, we avoid the issue by opening outside of parallel code and having per-thread file descriptors. This is my reason adding one more critical block.

What this does not fix is a combination of vrt rasters and standard rasters being read at the same time, but that's outside of scope of this PR. (My rule would apply to it, so I'm mentioning it for context.)

Additionally, I suggest naming the critical block (raster_vrt_read). This will allow other unrelated parallel code with a critical block to run in parallel (r.univar has one).

metzm and others added 2 commits March 21, 2026 09:57
Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
@metzm
Copy link
Copy Markdown
Contributor Author

metzm commented Mar 21, 2026

Thanks for your suggestion to also protect R__.fileinfo. Applied with slight modifications

@metzm metzm requested a review from wenzeslaus March 23, 2026 20:53
Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I think this is all we can do here.

I actually meant to replace the whole second comment with a implementation perspective comment (replacing the encountered bug perspective), but even this can be merged as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport to 8.4 PR needs to be backported to release branch 8.4 backport to 8.5 PR needs to be backported to release branch 8.5 bug Something isn't working C Related code is in C libraries raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] r.patch fails if one input is a GRASS vrt and nprocs > 1

2 participants