<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Oct 4, 2017 at 7:35 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Ahh, i see what you’re saying.  Dest *becomes* Source for a short time.<br><br>The main thing I’m concerned about with this new implementation is that it increases the complexity quite a bit, and in doing so makes the semantics harder to reason about.  How many additional weird codepaths in the kernel is this tickling now?  So there could be more (or at least different) race conditions.<br></blockquote><div><br></div><div>That was a concern for me as well, but it is somewhat mitigated by the fact that it is essentially a reimplementation of ReplaceFile, which is already part of the Windows API, so I'd expect the kernel code paths to be relatively well exercised. From MSDN: "An application can call ReplaceFile instead of calling separate functions to save the data to a new file, rename the original file using a temporary name, rename the new file to have the same name as the original file, and delete the original file." i.e. what my code is doing.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Would it be possible for the ThinLTO processes to map a shared memory region and synchronize on it instead of relying on flaky file system semantics?<br></blockquote><div><br></div><div>Not easily. The cache needs to be persisted between linker invocations, so I think there would need to be something on the file system. I suppose that you could imagine mmap'ing a large sparse file into every process and synchronizing on that, but there is also a mechanism for pruning old cache entries in order to save disk space (which may run concurrently with another process that is adding entries to the cache), so even if we did have an mmap'ed file, we'd need to arrange for the pruner to release pages to free storage, and to do so without races.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">As long as you’re relying on the filesystem i don’t think any amount of complexity will save you from every race condition.<br><br>I’m not *completely* opposed here, but I’m a little concerned about the additional complexity <br></blockquote><div><br></div><div>My view is that any alternatives to the file system would come with its own complexity (which my gut says would be higher than using the file system directly) and in the end would have to interact with the file system at some level in order to persist data.</div><div><br></div><div>Peter</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"><div class="gmail_quote"><div dir="ltr">On Wed, Oct 4, 2017 at 7:23 PM Peter Collingbourne <<a href="mailto:peter@pcc.me.uk" target="_blank">peter@pcc.me.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Oct 4, 2017 at 7:01 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">But the Source file is just a temporary file right that’s not (yet) part of the cache right?  So why would another process be accessing the same temporary file instead of creating a different one?<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>My point is that Source receives the name of Dest during the call to SetFileInformationByHandle. So after SetFileInformationByHandle returns there is a file with the name Dest that was opened (as Source) without sharing, so concurrent accesses to Dest fail until the handle is closed.<br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Is the Source file already part of the cache for some reason?<br></blockquote><div> </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>No.</div><div><br></div><div>Peter</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Btw, I have an (absolutely terrible) idea how to solve this “for real”, but I’ll save it until I understand the problem better <br><div class="gmail-m_-4916188517609236880m_-6570315679393216155HOEnZb"><div class="gmail-m_-4916188517609236880m_-6570315679393216155h5"><div class="gmail_quote"><div dir="ltr">On Wed, Oct 4, 2017 at 6:38 PM Peter Collingbourne via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">pcc added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D38570#889021" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D38570#889021</a>, @zturner wrote:<br>
<br>
> So if I understand the problem correctly, you don't have any issue with `ReplaceFile`'s behavior on the destination, only the source.  So, for example, you call `ReplaceFile`, it returns, and then some other process tries to open the //source// file and fails.<br>
<br>
<br>
Not quite. From the ThinLTO perspective I don't care about the behaviour on the source file before it is renamed, because all source files are temporary files whose names are unknown to other processes. The problem is with the behaviour on the source file after it has been renamed to the destination file. ThinLTO does this for each new cache file that it creates:<br>
<br>
  rename("thinlto-cache/<wbr>temporary file name", "thinlto-cache/llvmcache-<wbr>ABC123") // ABC123 is a unique hash that is computed from the linker's input files and command line options. There is a many-to-one mapping from hashes to object files (modulo collisions, but that's unlikely enough not to matter).<br>
<br>
In parallel, another process will be doing this in order to try and open an existing cache file:<br>
<br>
  open("thinlto-cache/llvmcache-<wbr>ABC123") // This is expected to either succeed (cache hit) or return "no such file or directory" (cache miss).<br>
<br>
Where this breaks is when `rename` is implemented in terms of `ReplaceFile`. In pseudocode, here's what I think is going on in the important part of `ReplaceFile`:<br>
<br>
  void ReplaceFile(char *Src, char *Dst) {<br>
    HANDLE h = CreateFile(Src, no sharing); // (1) Src cannot be opened<br>
    SetFileInformationByHandle(h, Rename, Dst); // (2) Now Dst cannot be opened<br>
    CloseFile(h); // (3) Now Dst can be opened<br>
  }<br>
<br>
So if the `open` happens between 2 and 3, it will get a permission denied error, which is not what the caller of `open` is expecting.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D38570" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D38570</a><br>
<br>
<br>
<br>
</blockquote></div>
</div></div></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><br><br clear="all"><div><br></div>-- <br><div class="gmail-m_-4916188517609236880m_-6570315679393216155gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div></blockquote></div>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div>