[gdal-dev] Race condition between forked processes with opened Tiff dataset on Linux

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[gdal-dev] Race condition between forked processes with opened Tiff dataset on Linux

Jiri Drbalek
Hello.

If a Linux process with opened Tiff dataset is forked, it is not possible to read from the dataset concurrently in these forked processes, because file offsets and other attributes of the opened Tiff file are shared between those processes.

One solution would be to serialize calls to GDAL, but this obviously completely destroy multiprocessing.

Another solution would be to open the dataset per each process, but this is also not desirable. An opened Tiff allocates memory for list of tile or strip offsets and sizes. These metadata can take hundreds of megabytes for large Tiff files, not to mention opening more of them. Therefore forking saves a lot of memory as these metadata are shared with parent process.

I've made a patch which optionally close the underlying Tiff file once a dataset is opened. One can then fork safely, underlying file is lazily opened again in each subprocess.

What do you think about this problem and proposed solutions? Is there some more elegant solution?

Here are two variants of the patch:

Thank you for any help.

Jiri


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Race condition between forked processes with opened Tiff dataset on Linux

Even Rouault-2

Jiri,

 

I'm not sure to which extent we want to support the fork() situation, as there are perhaps situations where that wouldn't work for some reason, but as something experimental why not.

 

Regarding the way file closing is done in VSIUnixStdioHandle, I think that instead of relying on ferror(), fclose() should be just called and fp nullified, and you would test that in Seek(), Read(), Write() etc to know if you must reopen the file

 

Another option would be to put that reopening logic at the driver level, but your option is probably better if we wanted to extend that to other drivers.

 

Actually I'm thinking to yet another option where things would work even without an explicit config option. VSIUnixStdioHandle could keep the getpid() active at opening time, and check before each access if it has changed, in which case it would open a fresh handle (assuming the getpid() cost is neglectable. not completely sure of that for drivers that do 1 byte per 1 byte read where the buffering logic of file streams avoids doing one system call each time... Looking at the manpage of getpid(), there used to be a time where glibc did a buffering of the getpid() result, but this was eventually removed because bug prone, so now a glibc getpid() translates into a system call)

 

In any case, we would definitely need an explicit test in autotest/cpp for that new capability

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Race condition between forked processes with opened Tiff dataset on Linux

Andrew C Aitchison-2
In reply to this post by Jiri Drbalek

On Thu, 14 Dec 2017, Jiri Drbalek wrote:

> Hello.
>
> If a Linux process with opened Tiff dataset is forked, it is not possible
> to read from the dataset concurrently in these forked processes, because
> file offsets and other attributes of the opened Tiff file are shared
> between those processes.

> I've made a patch which optionally close the underlying Tiff file once a
> dataset is opened. One can then fork safely, underlying file is lazily
> opened again in each subprocess.
>
> What do you think about this problem and proposed solutions? Is there some
> more elegant solution?

Can I assume that we are talking about opening for read and not for write ?

For writing, I was taught that multi-process programs should do all file
writing in a dedicated thread.

--
Andrew C. Aitchison Cambridge, UK
  [hidden email]
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Race condition between forked processes with opened Tiff dataset on Linux

Jiri Drbalek
> Can I assume that we are talking about opening for read and not for write ?

Yes, it's only for reading.

I've concluded eventually that I will try to use memory mapping.

Here is a post about memory mapping between Even and me I forgot to
resend to the mailing list:

> Dear Even,
>
> Thank you for your helpful answer.
>
> I forgot to mention that when I was testing the fork() situation with
> libtiff alone, it was working fine when the tiff file was memory
> mapped. Unfortunately, for some reason, GDAL doesn't support memory
> mapping of compressed tiffs. Libtiff can read them, at least those
> with deflate compression I've tested. What is the reason for that
> restriction?

mmap is platform specific, so there was a need for a more general
mechanism. And at the time the GeoTIFF driver was created, 32 bit
processes were still common but large files already existed, so even
on Linux, mmap wasn't really always usable. Another potential issue is
that the OS might not behave appropriately if you mmap() a file larger
than the available RAM and read it entirely. At least that was my
experience with some older kernels where the OS wouldn't unload cached
pages aggressively enough, making it irresponsive due to heavy cache
swapping.

Another option at the /vsi level I looked yesterday was the use of
pread() that takes both a file offset and size to read, and makes it
possible to use the same underlying file descriptor from multiple
threads/processes. But the caveat is that this translates directly as
a system call, bypassing file stream buffering.

> Should I try to enable mmaping of compressed tiffs?

That could be a solution worth investigating. Perhaps restricted to
64bit posix platforms, and with a configuration option, such as the
existing GTIFF_USE_MMAP that you'll see if you look at
frmts/gtiff/tifvsi.cpp. The name is a bit misleading since that's
currently only available for a pseudo-mmap emulation for /vsimem/
files, that was added per https://trac.osgeo.org/gdal/changeset/39555.
As you may wonder why this was done, the aim was to be able to test
the code paths in libtiff that are mmap() specific, when GDAL is
tortured by oss-fuzz.


Even

2017-12-16 8:53 GMT+01:00, Andrew C Aitchison <[hidden email]>:

>
> On Thu, 14 Dec 2017, Jiri Drbalek wrote:
>
>> Hello.
>>
>> If a Linux process with opened Tiff dataset is forked, it is not possible
>> to read from the dataset concurrently in these forked processes, because
>> file offsets and other attributes of the opened Tiff file are shared
>> between those processes.
>
>> I've made a patch which optionally close the underlying Tiff file once a
>> dataset is opened. One can then fork safely, underlying file is lazily
>> opened again in each subprocess.
>>
>> What do you think about this problem and proposed solutions? Is there
>> some
>> more elegant solution?
>
> Can I assume that we are talking about opening for read and not for write ?
>
> For writing, I was taught that multi-process programs should do all file
> writing in a dedicated thread.
>
> --
> Andrew C. Aitchison Cambridge, UK
>   [hidden email]
>
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Race condition between forked processes with opened Tiff dataset on Linux

Jiri Drbalek
In reply to this post by Even Rouault-2
Even,

that was really helpful. Finally, I solved it without any need to modify GDAL.

I do the mmap() myself in my application. Then I create VSILFILE out of the mmapped block by VSIFileFromMemBuffer().

I can then GDALOpen() the /vsimem/ file and fork freely.

The GTIFF_USE_MMAP is a cherry on the cake.

I still have to evaluate performance impact, but it seems promising so far.

Thanks for the advice.

Jiri




2017-12-15 11:29 GMT+01:00 Even Rouault <[hidden email]>:

On jeudi 14 décembre 2017 23:31:48 CET Jiri Drbalek wrote:

> Dear Even,

>

> Thank you for your helpful answer.

>

> I forgot to mention that when I was testing the fork() situation with

> libtiff alone, it was working fine when the tiff file was memory

> mapped. Unfortunately, for some reason, GDAL doesn't support memory

> mapping of compressed tiffs. Libtiff can read them, at least those

> with deflate compression I've tested. What is the reason for that

> restriction?

 

mmap is platform specific, so there was a need for a more general mechanism. And at the time the GeoTIFF driver was created, 32 bit processes were still common but large files already existed, so even on Linux, mmap wasn't really always usable. Another potential issue is that the OS might not behave appropriately if you mmap() a file larger than the available RAM and read it entirely. At least that was my experience with some older kernels where the OS wouldn't unload cached pages aggressively enough, making it irresponsive due to heavy cache swapping.

 

Another option at the /vsi level I looked yesterday was the use of pread() that takes both a file offset and size to read, and makes it possible to use the same underlying file descriptor from multiple threads/processes. But the caveat is that this translates directly as a system call, bypassing file stream buffering.

 

> Should I try to enable mmaping of compressed tiffs?

 

That could be a solution worth investigating. Perhaps restricted to 64bit posix platforms, and with a configuration option, such as the existing GTIFF_USE_MMAP that you'll see if you look at frmts/gtiff/tifvsi.cpp. The name is a bit misleading since that's currently only available for a pseudo-mmap emulation for /vsimem/ files, that was added per https://trac.osgeo.org/gdal/changeset/39555. As you may wonder why this was done, the aim was to be able to test the code paths in libtiff that are mmap() specific, when GDAL is tortured by oss-fuzz.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com



_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev