[LibOS] fs_lock: Use a separate lock for all operations

[pwmarcz]

Description of the changes

Instead of using shim_dentry.lock for data stores in individual dentries, we use a global local (g_fs_lock_lock) for all fs_lock
related operations. This is in preparation for removing shim_dentry.lock.

Context: #279.

I realize that using a lock for guarding a field on another object (shim_dentry) doesn't look nice. However, I tried the alternative (taking the global lock for dentries, g_dcache_lock) and it breaks because of the spaghetti in filesystem code[1]. While I could probably work around this specific issue, I think it's better to keep this module separate (i.e. using its own lock) so that it's easier to reason about.

[1] If you want to know the gory details, it was shim_pipe: because it uses extra file descriptor, the callback for open actually "closes" one of these artificial file descriptors, triggering fs_lock while we're already holding g_dcache_lock. Because we don't expect to be holding g_dcache_lock at this stage, we try to lock it for the second time. IMO shim_pipe breaks some implicit assumptions here, but there might be more places like this.

How to test this PR?

CI should be enough.

---

This change is 

[dimakuv]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @pwmarcz)

---

LibOS/shim/src/fs/shim_fs_lock.c, line 15 at r1 (raw file):

/*
 * Global lock for the whole subsystem. Protects access to `g_fs_lock_list`, and also to dentry
 * fields (`fs_lock and `maybe_has_fs_locks`).

Missing closing backtick in fs_lock

---

LibOS/shim/src/fs/shim_fs_lock.c, line 628 at r1 (raw file):

    *out_dents = dents;
    return 0;
}

I guess removing this two-phase deletion of fs locks is a huge benefit of this PR. I'm pretty sure the performance overhead of a single global lock is negligible in real-world cases.

[pwmarcz]

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)

---

LibOS/shim/src/fs/shim_fs_lock.c, line 15 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Missing closing backtick in fs_lock

Fixed.

---

LibOS/shim/src/fs/shim_fs_lock.c, line 628 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I guess removing this two-phase deletion of fs locks is a huge benefit of this PR. I'm pretty sure the performance overhead of a single global lock is negligible in real-world cases.

Yes, I'm happy about it. But also about untangling this module from the rest of the system, it's more independent now.

I think a global lock could have an impact on performance if an application takes many locks for various ranges of a single file (since lock operations are O(n) in the number of file ranges). But I'm not sure how likely that is, and anyway, to fix it we'd also need to rewrite the lock list to something else.

[dimakuv]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

[dimakuv]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved