[PATCH] D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D121423#3376377 <https://reviews.llvm.org/D121423#3376377>, @bnbarham wrote:

> In D121423#3376347 <https://reviews.llvm.org/D121423#3376347>, @dexonsmith wrote:
>
>> I've thought of another concern: the "recursively call CWD" behaviour can result in CWD being called multiple times on a filesystem instance.
>
> Ouch. I didn't consider this case. There's certainly no tests for it and I'd be surprised if anyone actually does that.
>
> But in saying that, I think:
>
>>> To alleviate the "getting stuck" maybe we could find the first setCWD that succeeds, getCWD, and then re-run setCWD on all the rest to that CWD. Thoughts?
>
> Does actually fix that issue right? Ie. the problem is passing a relative path to each FS. But if we avoid that by using the absolute path of the first FS that succeeds then we're all good.

I think you're right. If the first success is used to convert to absolute, it should be okay. I think worth adding tests for since I do think this sort of thing can happen... TBH, all of the cases I can think of are about old RedirectingFS behaviour, so maybe not, but at least worth figuring out if it works with the changes here and keeping it working to prevent future confusion.

>> Stepping back, I'm not sure it's sound to be calling `cd` on the underlying FS. I think this is what led to `RedirectingFS` maintaining its own CWD and using absolute paths for access.
>
> I would have to check the order of commits here. But I believe `OverlayFS` stopped being used for `-ivfsoverlay` before CWD was added, so `RedirectingFS` necessarily had to pass the absolute path down because there was no other option. That path leads to insanity though (IMO) and the current implementation isn't even correct - given multiple overlays, if the first fails then we *always* end up returning the original path.
>
> What's the alternative to setting the CWD on the underlying FS here? Just removing `OverlayFS`? I did attempt to fix the bug I just mentioned directly, but it makes `RedirectingFS` even *more* of a mess and I was struggling quite a bit to reason about everything. I really don't think that's the direction we want to go.
>
> My vote is on:
>
> 1. Use the CWD of the first successful FS
> 2. Set CWD on all FS to the one returned by the first successful FS
>
> Though I believe this will mean the tests are still broken as this really only fixes the relative case, I haven't had a chance to figure out what's going on there yet.

Redirecting FS is definitely hard to reason about right now; still integrating what gets simplified with your change. I had completely missed that, because you're removing the fallthrough behaviour, it no longer needs to query the underlying FS with a relative path... ever. This is awesome!

----

A few remaining thoughts:

- The first successful FS will often be a RedirectingFS. But the BaseFS seems most likely to be a filesystem that is well-equipped to canonicalize the CWD. Since we're calling this on every FS, should we run the `cd` loop bottom-up instead of top-down (`reverse(overlays())`)?

- Should we make PhysicalFileSystem better at canonicalizing the CWD? (maybe off-topic for this patch)
  - On Windows, it could just run `remove_dots(/*remove_dot_dot=/true)`. This is trivial.
  - Else, if there are `..`s in a new relative path CWD, it can `getRealPath()` and:
    - if it matches what `remove_dot_dot` would give, use that
    - else, try `getRealPath()` on the `remove_dot_dot=true` path, and if that's the same as the origin `getRealPath()`, use the remove-dot-dot path
    - else, give up and use the current logic
  - Downside is that `getRealPath()` can be slow. But will this matter in practice for `setCWD()` calls?

- The client's view of the OverlayFS *should* be a single (virtual) filesystem. But without top-level CWD modelling, we can't actually provide that. Consider:

  overlay:
    base:
      /file
    memory:
      /dir/
  
  operations:
    cd /dir
    cat ../file

This *should* give the content from `/file` in `base`. But even with our proposed changes it's not going to work correctly. I think one way to be correct would be:

- Find the "first" FS (like CWD?) where `getRealPath(dirname(relativepath))` does not fail
- Then combine that real-path with `filename(relativepath)`, and go through the overlays with that path

... but this would probably be too slow. Not sure if you have ideas on how to fix it; and I don't think it's made worse by this patch (off topic?). Maybe worth adding a FIXME or description somewhere documenting that it doesn't work.

(Regardless, I think your plan should work.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121423/new/

https://reviews.llvm.org/D121423