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

Fri Mar 11 16:05:32 PST 2022

bnbarham added a comment.

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.

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121423