[LibOS] Implement POSIX locks

[pwmarcz]

Description of the changes

This implements POSIX locks (fcntl) in client-server IPC architecture. This is a bit hacky and has some drawbacks (locking will always use IPC if you're not in main process, files are identifed by hacks), but I think it should be good enough.

(The sync engine is simply not ready for this: it would require some more features, but first I think it needs to be simplified, I think I've already blown all available complexity budget for this).

See shim_fs_lock.h for an introduction.

Closes #437.

How to test this PR?

• I initially added an example for SQLite, but together with @mkow we decided it's probably not worth keeping. Too much maintenance burden, and there are other tests anyway. So I removed it from this PR.
• There is a regression test (fcntl_lock). It tests some cases but LTP turns out to be much more thorough. Still, it might be a good sanity check, and it includes some cases we care about (it makes a difference to us whether the waiting process is the main one).
• I added a log_debug() that displays locked ranges after each lock operation.
• I was able to reenable many LTP tests. The tests for this feature turn out to be really good! After my test started passing, they still found the following:
  • Various correctness tests for overlapping ranges triggered bugs (typos) in my code
  • I learned that it's enough to close a FD (even duplicated) to drop locks, not close the whole file handle
  • I learned that I need to drop all locks for a process immediately after it exits (i.e. before wait() returns), and not as an asynchronous action

---

This change is 

[dimakuv]

Reviewed 16 of 17 files at r1, 6 of 6 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 37 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

a discussion (no related file):

I initially added an example for SQLite, but together with @mkow we decided it's probably not worth keeping. Too much maintenance burden, and there are other tests anyway. So I removed it from this PR.

Could you submit it as a separate PR, so that we don't lose this work?

Generally, I believe that SQLite is a very good candidate to be included in our Examples and CI. It at least shows that SQL database engines can work on Graphene (currently we only have noSQL Redis in cache-only mode).

---

---

LibOS/shim/include/shim_fs_lock.h, line 70 at r1 (raw file):

 *
 * If `pl->type` is `F_UNLCK`, the function will remove any locks held by the given PID for the
 * given range. Removing a locks never waits.

Typo: a lock

---

LibOS/shim/include/shim_fs_lock.h, line 97 at r1 (raw file):

int posix_lock_clear_pid(IDTYPE pid);

/* Version of `posix_lock_set` called from IPC callback. */

Could you add comments on params? For example, path must be an absolute path or can be relative? Must it be normalized? And what does postponed mean?

UPDATES:

• path is indeed an absolute path
• I don't like postponed and proposed to remove it in other comments

---

LibOS/shim/include/shim_fs_lock.h, line 101 at r1 (raw file):

                            unsigned long seq, bool* postponed);

/* Version of `posix_lock_get` called from IPC callback. */

Could you add comments on params? This function is kinda obvious, so feel free to ignore.

---

LibOS/shim/include/shim_ipc.h, line 287 at r1 (raw file):

int ipc_posix_lock_set(const char* path, struct posix_lock* pl, bool wait);
int ipc_posix_lock_set_send_response(IDTYPE vmid, unsigned long seq, int result);

I am confused by this function. Why don't we have a similar one ipc_posix_lock_get_send_response?

UPDATE: I proposed to remove it (make it static) in other comments

---

LibOS/shim/src/bookkeep/shim_handle.c, line 320 at r1 (raw file):

    if (handle && handle->dentry) {
        /* Clear POSIX locks for a file. We are required to do that every time a FD is closed
         * closed, even if the process holds other handles for that file, or duplicated FDs for the

Typo: you have closed closed

---

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

        }

        new_dent->fs_lock = NULL;

Could you add a comment that POSIX locks are not inherited over forks?

---

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

#include "shim_lock.h"

/* Describes a pending request for a POSIX lock, either local on remote. After processing the request,

on -> or

---

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

     * processing the request, IPC response will be sent. */
    IDTYPE vmid;
    unsigned int seq;

How do you distinguish between a local and a remote request? Could you add a comment on this?

UPDATE: Theoretically it can be both. But in reality, event!=NULL and vmid!=0 denote that local/remote event should be fired.

---

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

    LISTP_TYPE(posix_lock_request) posix_lock_requests;

    LIST_TYPE(fs_lock) list;

This field is for traversing g_fs_lock_list, right? Could you add a comment about this?

---

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

static void fs_lock_gc(struct fs_lock* fs_lock) {
    assert(locked(&g_fs_lock_lock));
    if (g_log_level >= LOG_LEVEL_DEBUG)

Isn't it too much info? I would prefer trace level.

---

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

 * ranges overlap, and at least one of them is a write lock.
 */
static struct posix_lock* posix_lock_find(struct fs_lock *fs_lock, struct posix_lock* pl) {

Wrong asterisk alignment in the first argument (also in a couple other functions in this file, please fix all)

---

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

 * ranges overlap, and at least one of them is a write lock.
 */
static struct posix_lock* posix_lock_find(struct fs_lock *fs_lock, struct posix_lock* pl) {

Can we rename this func to posix_lock_find_conflict?

---

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

/*
 * Process pending requests after modifying the list of locks.

Could you add a comment that at this point, we probably removed some conflicting locks, so we should notify the waiters that they can try to grab the lock again.

---

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

        *out_req = req;
    } else {
        *out_req = NULL;

You already set it below, looks like it is not needed on this line

---

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

int posix_lock_set(struct shim_dentry* dent, struct posix_lock* pl, bool wait) {
    int ret;
    if(g_process_ipc_ids.leader_vmid) {

if (g_... -- add a space

---

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

    if(g_process_ipc_ids.leader_vmid) {
        /* In the IPC version, we use `dent->maybe_has_locks` to short-circuit unlocking files that
         * we never locked. This is to prevent unnecessary IPC calls on on handle. */

Typo: on on

---

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

    struct posix_lock_request* req = NULL;

    int ret = path_lookupat(g_dentry_root, path, LOOKUP_NO_FOLLOW, &dent);

Why don't we follow symlinks here?

---

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

    struct shim_dentry* dent = NULL;
    int ret = path_lookupat(g_dentry_root, path, LOOKUP_NO_FOLLOW, &dent);

ditto

---

LibOS/shim/src/ipc/shim_ipc_fs_lock.c, line 117 at r3 (raw file):

        return 0;
    }
    return ipc_posix_lock_set_send_response(src, seq, ret);

Is this postponed hack really needed? Why not return ret in this function instead of sending the response, and invoke posix_lock_process_requests() from within posix_lock_set_from_ipc() always?

I understand that this is slightly less efficient, but we probably don't care about efficiency at this point. And I find this postponed logic quite contrived.

This way you'll be also able to make ipc_posix_lock_set_send_response() static.

---

LibOS/shim/src/sys/shim_exit.c, line 98 at r1 (raw file):

    }

    /* Clear POSIX locks before we notify parent: after a successful `wait`, our locks should be

What does it mean in your comment: "after a successful wait"?

---

LibOS/shim/src/sys/shim_fcntl.c, line 2 at r1 (raw file):

/* SPDX-License-Identifier: LGPL-3.0-or-later */
/* Copyright (C) 2014 Stony Brook University */

You should add your copyright here.

---

LibOS/shim/src/sys/shim_fcntl.c, line 5 at r1 (raw file):

/*
 * Implementation of system call "fcntl".

Could you list here the supported commands?

---

LibOS/shim/src/sys/shim_fcntl.c, line 17 at r1 (raw file):

#include "shim_handle.h"
#include "shim_internal.h"
#include "shim_process.h"

Please sort

---

LibOS/shim/src/sys/shim_fcntl.c, line 45 at r1 (raw file):

/*
 * Convert user-mode `struct flock` into our `struct posix_lock`. This mostly means converting the
 * position parameters (l_whence, l_start, l_len) to an absolute inclusve range [start .. end]. See

inclusive

---

LibOS/shim/src/sys/shim_fcntl.c, line 97 at r1 (raw file):

    uint64_t start, end;
    if (fl->l_len > 0) {
        /* l_len < 0: the range is [origin .. origin + len - 1] */

Typo: should be l_len > 0

---

LibOS/shim/src/sys/shim_fcntl.c, line 134 at r1 (raw file):

         *   equal to arg and make it be a copy of fd.  This is different from
         *   dup2(2), which uses exactly the descriptor specified.
         *   On success, the new descriptor is returned.

Could you re-wrap these comments to 100 chars per line? And slightly fix the texts where appropriate.

---

LibOS/shim/src/sys/shim_fcntl.c, line 215 at r1 (raw file):

         *   or file-region locks).  The third argument, lock, is a pointer to
         *   a structure that has at least the following fields (in unspecified
         *   order).

The part about ...that has at least... is useless here, you can just say struct flock.

---

LibOS/shim/src/sys/shim_fcntl.c, line 222 at r1 (raw file):

         *   l_whence, l_start, and l_len fields of lock.  If a conflicting lock
         *   is held by another process, this call returns -1 and sets errno to
         *   EACCES or EAGAIN.

The part about this call returns -1 is wrong, please fix.

---

LibOS/shim/src/sys/shim_fcntl.c, line 229 at r1 (raw file):

         *   waiting, then the call is interrupted and (after the signal handler
         *   has returned) returns immediately (with return value -1 and errno
         *   set to EINTR; see signal(7)).

The part about interrupts is wrong, please make it clear that currently we don't return EINTR.

---

LibOS/shim/src/sys/shim_fcntl.c, line 247 at r1 (raw file):

            if (fl->l_type == F_RDLCK && !(hdl->acc_mode & MAY_READ))
                return -EINVAL;

Aren't you supposed to break? Right now you forget to release the handle.

---

LibOS/shim/src/sys/shim_fcntl.c, line 250 at r1 (raw file):

            if (fl->l_type == F_WRLCK && !(hdl->acc_mode & MAY_WRITE))
                return -EINVAL;

ditto

---

LibOS/shim/src/sys/shim_fcntl.c, line 280 at r1 (raw file):

            if (!hdl->dentry)
                return -EINVAL;

ditto

---

LibOS/shim/src/sys/shim_fcntl.c, line 288 at r1 (raw file):

            if (pl.type == F_UNLCK)
                return -EINVAL;

ditto

---

LibOS/shim/test/regression/fcntl_lock.c, line 104 at r1 (raw file):

static void unlock(long int start, long int len) {
    if (!try_lock(F_SETLK, F_UNLCK, SEEK_SET, start, len))
        errx(1, "untry_lock failed");

untry_lock?

---

LibOS/shim/test/regression/fcntl_lock.c, line 194 at r1 (raw file):

    } while (ret == -1 && errno == EINTR);
    if (ret == -1)
        err(1, "write");

read

---

LibOS/shim/test/regression/test_libos.py, line 587 at r1 (raw file):

    def test_110_fcntl_lock(self):
        stdout, _ = self.run_binary(['fcntl_lock'])
        self.assertIn('TEST OK', stdout)

You should also remove the created file unconditionally at the end of this test.

[pwmarcz]

Reviewable status: 8 of 17 files reviewed, 37 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I initially added an example for SQLite, but together with @mkow we decided it's probably not worth keeping. Too much maintenance burden, and there are other tests anyway. So I removed it from this PR.

Could you submit it as a separate PR, so that we don't lose this work?

Generally, I believe that SQLite is a very good candidate to be included in our Examples and CI. It at least shows that SQL database engines can work on Graphene (currently we only have noSQL Redis in cache-only mode).

OK, I'll create a PR with this branch as target (since it doesn't work on master yet).

---

---

LibOS/shim/include/shim_fs_lock.h, line 70 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Typo: a lock

Fixed.

---

LibOS/shim/include/shim_fs_lock.h, line 97 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you add comments on params? For example, path must be an absolute path or can be relative? Must it be normalized? And what does postponed mean?

UPDATES:

• path is indeed an absolute path
• I don't like postponed and proposed to remove it in other comments

I agree, these deserve more explanation, especially the part about a response. I added a full doc comment.

(I thought about using the abs_path name throughout the code, but it adds too much noise. But the comment now mentions it's supposed to be absolute).

---

LibOS/shim/include/shim_fs_lock.h, line 101 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you add comments on params? This function is kinda obvious, so feel free to ignore.

Done.

---

LibOS/shim/include/shim_ipc.h, line 287 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I am confused by this function. Why don't we have a similar one ipc_posix_lock_get_send_response?

UPDATE: I proposed to remove it (make it static) in other comments

Because the response for posix_lock_get is sent immediately, and the one for posix_lock_set has to sometimes be sent later. I replied in detail in the other comment.

---

LibOS/shim/src/bookkeep/shim_handle.c, line 320 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Typo: you have closed closed

Fixed.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you add a comment that POSIX locks are not inherited over forks?

It's not even about POSIX locks: currently fs_lock is used only by process leader. I added a comment.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

on -> or

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

How do you distinguish between a local and a remote request? Could you add a comment on this?

UPDATE: Theoretically it can be both. But in reality, event!=NULL and vmid!=0 denote that local/remote event should be fired.

I reworded this comment, and made the code a bit clearer: we always set all the fields, and check that exactly one of the cases is true.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This field is for traversing g_fs_lock_list, right? Could you add a comment about this?

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Isn't it too much info? I would prefer trace level.

Makes sense, I imagine a program making heavy use of locks will output a lot of these (and we can always grep for them). Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Wrong asterisk alignment in the first argument (also in a couple other functions in this file, please fix all)

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we rename this func to posix_lock_find_conflict?

That's a good name. Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you add a comment that at this point, we probably removed some conflicting locks, so we should notify the waiters that they can try to grab the lock again.

Added a comment (actually we add the lock already and notify the waiter that we succeeded).

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You already set it below, looks like it is not needed on this line

My bad. Fixed.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

if (g_... -- add a space

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Typo: on on

Changed to on a.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why don't we follow symlinks here?

If the original dent was not a symlink, it should not matter. If it was a symlink, we want to recover the same symlink, and LOOKUP_FOLLOW would recover its target instead.

(Currently, dent should never be a symlink, because Graphene does not allow you to get a handle to a symlink. But LOOKUP_NO_FOLLOW is IMO the right default: there is no reason for us to want to traverse a symlink).

---

LibOS/shim/src/ipc/shim_ipc_fs_lock.c, line 117 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is this postponed hack really needed? Why not return ret in this function instead of sending the response, and invoke posix_lock_process_requests() from within posix_lock_set_from_ipc() always?

I understand that this is slightly less efficient, but we probably don't care about efficiency at this point. And I find this postponed logic quite contrived.

This way you'll be also able to make ipc_posix_lock_set_send_response() static.

Are you proposing to always add a request and then invoke posix_lock_process_requests()? I don't want to do that: I think it would obscure the fact that the wait == false case is guaranteed to return immediately (even if technically that can be guaranteed). I'm sure the current version can be improved, but I like that it clearly distunguishes the "wait" and "return immediately" cases.

I agree with moving the invocation of ipc_posix_lock_send_response() to the main module, so that both these are handled in that file. That indeed removes the need for postponed. Done.

However, I disagree with making ipc_posix_lock_set_send_response() static. I think it belongs in this file - I want to keep actual logic and IPC glue code separate.

---

LibOS/shim/src/sys/shim_exit.c, line 98 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What does it mean in your comment: "after a successful wait"?

Reworded to say: "after a successful wait() by parent, our locks should already be gone". This is one of the issues found by LTP: the parent process got notified about a child process exit, but there was a short window before the locks of that process were removed (so F_SETLK sometimes failed for parent).

---

LibOS/shim/src/sys/shim_fcntl.c, line 2 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You should add your copyright here.

Done.

---

LibOS/shim/src/sys/shim_fcntl.c, line 5 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you list here the supported commands?

Done.

---

LibOS/shim/src/sys/shim_fcntl.c, line 17 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please sort

Done.

---

LibOS/shim/src/sys/shim_fcntl.c, line 45 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

inclusive

Fixed.

---

LibOS/shim/src/sys/shim_fcntl.c, line 97 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Typo: should be l_len > 0

Fixed (and I'd rather say len > 0 etc. in comments, for readability).

---

LibOS/shim/src/sys/shim_fcntl.c, line 134 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you re-wrap these comments to 100 chars per line? And slightly fix the texts where appropriate.

They seem to be copied more or less verbatim from man fcntl, but the description in man page usually has some more paragraphs of context. They also seem to be outdated: for example, the versions of Linux man pages that I can find already correct some long argument types to int.

So, I'd rather not maintain these descriptions independently here. For any kind of serious review or modification I'd expect the developer to look at the man page anyway.

How about we just keep the argument types here, and whatever Graphene does differently?

/* See `man fcntl` for the expected semantics of these calls. */

/* F_DUPFD (int) */
case F_DUPFD: ...

/* F_SETLK, F_SETLKW (struct flock*): see `shim_fs_lock.h` for caveats */
case F_SETLK:
case F_SETLKW: ...

/* F_SETOWN (int): a dummy implementation for now */
case F_SETOWN:
    ret = 0;
    break;

---

LibOS/shim/src/sys/shim_fcntl.c, line 247 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Aren't you supposed to break? Right now you forget to release the handle.

My mistake, we never want to return in this switch. Fixed.

---

LibOS/shim/test/regression/fcntl_lock.c, line 104 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

untry_lock?

Ouch, a search-and-replace accident. Fixed.

---

LibOS/shim/test/regression/fcntl_lock.c, line 194 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

read

Fixed.

---

LibOS/shim/test/regression/test_libos.py, line 587 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You should also remove the created file unconditionally at the end of this test.

Done.

[pwmarcz]

Reviewable status: 8 of 17 files reviewed, 37 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

OK, I'll create a PR with this branch as target (since it doesn't work on master yet).

PR: #2493

---

[dimakuv]

Reviewed 9 of 9 files at r4.
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), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

LibOS/shim/include/shim_fs_lock.h, line 120 at r4 (raw file):

 *
 * \param path absolute path for a file
 * \param pl parameters of new lock (type cannot be `F_UNLCK`

You forgot closing )

---

LibOS/shim/src/ipc/shim_ipc_fs_lock.c, line 117 at r3 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Are you proposing to always add a request and then invoke posix_lock_process_requests()? I don't want to do that: I think it would obscure the fact that the wait == false case is guaranteed to return immediately (even if technically that can be guaranteed). I'm sure the current version can be improved, but I like that it clearly distunguishes the "wait" and "return immediately" cases.

I agree with moving the invocation of ipc_posix_lock_send_response() to the main module, so that both these are handled in that file. That indeed removes the need for postponed. Done.

However, I disagree with making ipc_posix_lock_set_send_response() static. I think it belongs in this file - I want to keep actual logic and IPC glue code separate.

OK, I like the new version.

---

LibOS/shim/src/sys/shim_exit.c, line 98 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Reworded to say: "after a successful wait() by parent, our locks should already be gone". This is one of the issues found by LTP: the parent process got notified about a child process exit, but there was a short window before the locks of that process were removed (so F_SETLK sometimes failed for parent).

OK, got it. Pretty cool.

---

LibOS/shim/src/sys/shim_fcntl.c, line 134 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

They seem to be copied more or less verbatim from man fcntl, but the description in man page usually has some more paragraphs of context. They also seem to be outdated: for example, the versions of Linux man pages that I can find already correct some long argument types to int.

So, I'd rather not maintain these descriptions independently here. For any kind of serious review or modification I'd expect the developer to look at the man page anyway.

How about we just keep the argument types here, and whatever Graphene does differently?

/* See `man fcntl` for the expected semantics of these calls. */

/* F_DUPFD (int) */
case F_DUPFD: ...

/* F_SETLK, F_SETLKW (struct flock*): see `shim_fs_lock.h` for caveats */
case F_SETLK:
case F_SETLKW: ...

/* F_SETOWN (int): a dummy implementation for now */
case F_SETOWN:
    ret = 0;
    break;

Yeah, removing is also a good option (well, actually a better one :)). Please proceed with your proposed change.

[pwmarcz]

Reviewable status: 14 of 17 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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

LibOS/shim/include/shim_fs_lock.h, line 120 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You forgot closing )

Fixed.

---

LibOS/shim/src/sys/shim_fcntl.c, line 134 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yeah, removing is also a good option (well, actually a better one :)). Please proceed with your proposed change.

Done.

---

LibOS/shim/src/sys/shim_fcntl.c, line 253 at r5 (raw file):

                if (pl2.end == FS_LOCK_EOF) {
                    /* range until EOF is encoded as len == 0 */
                    fl->l_len = 0;

Apparently this was not checked by LTP. I added a test to regression/fcntl_lock.c.

[mkow]

FYI: I'm submitting a partial review for now and will finish the rest tomorrow.

Reviewed 8 of 17 files at r1, 1 of 6 files at r2, 1 of 1 files at r3, 5 of 9 files at r4, 1 of 3 files at r5, 1 of 1 files at r6.
Reviewable status: 16 of 17 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)

---

LibOS/shim/include/shim_fs.h, line 157 at r1 (raw file):

    void* data;

    /* File lock information, stored in the main process. See shim_fs_lock.c. */

stored in the main process -> stored only in the main process (added "only")

---

LibOS/shim/include/shim_fs.h, line 118 at r3 (raw file):

#define DENTRY_MAX_CHILDREN 1000000

struct fs_lock_info;

Shouldn't we include shim_fs_lock.h instead?

---

LibOS/shim/include/shim_fs.h, line 162 at r3 (raw file):

    /* True if the file might have locks placed by current process. Used in processes other than
     * main process, to prevent unnecessary IPC calls on handle close. See shim_fs_lock.c. */
    bool maybe_has_locks;

locks -> fs_locks (this is confusing, because it may mean Graphene-level locks for concurrent access to this struct)

---

LibOS/shim/include/shim_fs_lock.h, line 7 at r3 (raw file):

Currently POSIX locks are implemented.

I'd add "only", sounds a bit weird otherwise.

---

LibOS/shim/include/shim_fs_lock.h, line 104 at r3 (raw file):

int posix_lock_get_from_ipc(const char* path, struct posix_lock* pl, struct posix_lock* out_pl);

#endif /* SHIM_FS_LOCK_H */

This doesn't match

---

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

DEFINE_LISTP(fs_lock);
DEFINE_LIST(fs_lock);
struct fs_lock {

Why isn't this in the header?

---

LibOS/shim/test/ltp/ltp.cfg, line 372 at r1 (raw file):

timeout = 60

# no deadlock detection

please add "in POSIX locks"

---

LibOS/shim/test/regression/fcntl_lock.c, line 107 at r1 (raw file):

}

static void lock_ok(int type, long int start, long int len) {

Why is this named lock_ok but the above one is just unlock?

---

LibOS/shim/test/regression/fcntl_lock.c, line 132 at r1 (raw file):

 */
static void test_ranges() {
    printf("test ranges...\n");

test -> testing (ditto below)
or just reword it somehow to make more sense grammatically/in this context :)

---

LibOS/shim/test/regression/fcntl_lock.c, line 349 at r1 (raw file):

    if (unlink(TEST_FILE) < 0)
        err(1, "unlink");

I'd leave this to keep this test self-contained, the removal in Python should be just a fallback.

---

LibOS/shim/test/regression/fcntl_lock.c, line 111 at r6 (raw file):

    if (cmd == F_GETLK) {
        return fl.l_type == F_UNLCK;

No check for ret in this case?

[dimakuv]

Reviewed 1 of 3 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

LibOS/shim/src/sys/shim_fcntl.c, line 253 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Apparently this was not checked by LTP. I added a test to regression/fcntl_lock.c.

Oops, good catch!

[pwmarcz]

Reviewable status: 11 of 17 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

LibOS/shim/include/shim_fs.h, line 157 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

stored in the main process -> stored only in the main process (added "only")

Done.

---

LibOS/shim/include/shim_fs.h, line 118 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't we include shim_fs_lock.h instead?

This is a private structure of shim_fs_lock.c anyway, so shim_fs_lock.h does not declare it.

I could declare it in shim_fs_lock.h, and include that here. Do you think that's better? I didn't want that, because then nobody would be forced to include shim_fs_lock.h directly, which I think is good for clarity.

---

LibOS/shim/include/shim_fs.h, line 162 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

locks -> fs_locks (this is confusing, because it may mean Graphene-level locks for concurrent access to this struct)

Done.

---

LibOS/shim/include/shim_fs_lock.h, line 7 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Currently POSIX locks are implemented.

I'd add "only", sounds a bit weird otherwise.

Agreed. Done.

---

LibOS/shim/include/shim_fs_lock.h, line 104 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This doesn't match

Fixed.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Why isn't this in the header?

Because it's a private structure, and only this file needs to know the details. See also futex or vma subsystems.

---

LibOS/shim/test/ltp/ltp.cfg, line 372 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

please add "in POSIX locks"

Done ("for POSIX locks" sounded better for me).

---

LibOS/shim/test/regression/fcntl_lock.c, line 107 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is this named lock_ok but the above one is just unlock?

Yes, it makes sense to rename it just to lock. Done.

---

LibOS/shim/test/regression/fcntl_lock.c, line 132 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

test -> testing (ditto below)
or just reword it somehow to make more sense grammatically/in this context :)

Changed all to "testing".

---

LibOS/shim/test/regression/fcntl_lock.c, line 349 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

    if (unlink(TEST_FILE) < 0)
        err(1, "unlink");

I'd leave this to keep this test self-contained, the removal in Python should be just a fallback.

OK. I restored the unlink here, and changed the Python code to "remove if exists".

---

LibOS/shim/test/regression/fcntl_lock.c, line 111 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

No check for ret in this case?

I added a check above, we really permit ret other than 0 in one specific case.

[dimakuv]

Reviewed 6 of 6 files at r7.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)

[mkow]

Reviewed 1 of 6 files at r2, 3 of 9 files at r4, 1 of 3 files at r5, 6 of 6 files at r7.
Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

LibOS/shim/include/shim_fs.h, line 118 at r3 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This is a private structure of shim_fs_lock.c anyway, so shim_fs_lock.h does not declare it.

I could declare it in shim_fs_lock.h, and include that here. Do you think that's better? I didn't want that, because then nobody would be forced to include shim_fs_lock.h directly, which I think is good for clarity.

My main concern was this was the reason to use a pointer instead of directly embedding fs_lock_info into the dentry, but seems that's not the case. Anyways, I think we should rethink our conventions w.r.t. headers and what goes where...

---

LibOS/shim/include/shim_ipc.h, line 268 at r6 (raw file):

    int type;
    uint64_t start;
    uint64_t end;

Please comment whether this is inclusive or exclusive

---

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

    PAL_HANDLE event;
    int* result;

What's the lifetime/ownership of this? Why is it a pointer in the first place? I didn't read the implementation yet, but looks really weird and unconventional, so I think it deserves a comment :)

---

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

sorted by PID and then by start position

Am I right, that this sorting is actually only for pretty-printing purposes?

Update: this is also used to be able to merge locks, right? If so, could you comment about this here? Overall sorted lists are rarely useful (because they are O(n^2) anyways), so it's a bit puzzling when you read it.

---

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

including access to dentry `fs_lock`

That sounds weird (using a global lock to lock fields in some structures?), doesn't dentry already have its own lock?

---

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

    if (!req)
        return -ENOMEM;
    req->pl = *pl;

What's the condition on whether other fields are in initialized state? It's quite implicit right now, it deserves a comment at the structure at least. The current comment sounds like some of the fields should be initialized depending on whether this request is an IPC one, but here you don't initialize anything except .pl.

---

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

    /* Target range: we will be changing it when merging existing locks. */
    uint64_t start = pl->start, end = pl->end;

I'd prefer to have this on separate lines with aligned =.

---

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

            continue;
        }
        if (pl->pid < cur->pid) {

I think it would read easier if you swapped sides, so that it complements the condition above.

---

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

        }

        if (pl->type == cur->type) {

ditto (this time because we're iterating with cur, so it seems more natural to have it on the left)

---

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

start <= cur->start

Could you swap sides? Would be easier to notice that this is complement to the conditions above.

---

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

                 */
                assert(start <= cur->start && end < cur->end);
                assert (end < FS_LOCK_EOF);

no space after assert

---

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

            }
        }
    }

wow, that loop was tough

---

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

bool wait

This name seems a bit misleading, because this function is not actually waiting if it's true, it just inserts a wait request into a list and returns.

---

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

        goto out;
    if (req) {
        assert(wait);

Why this always has to be the case here?

---

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

    if (req) {
        assert(wait);

ditto

---

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

    int ret;
    if (g_process_ipc_ids.leader_vmid) {
        /* In the IPC version, we use `dent->maybe_has_fs_locks` to short-circuit unlocking files that

please rewrap

---

LibOS/shim/src/sys/shim_fcntl.c, line 70 at r7 (raw file):

    uint64_t origin;
    switch (fl->l_whence) {

Couldn't we just call fs->fs_ops->seek() and let it do the calculations?

---

LibOS/shim/test/ltp/ltp.cfg, line 372 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Done ("for POSIX locks" sounded better for me).

Right, sounds better this way.

[pwmarcz]

Reviewable status: 16 of 17 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @pwmarcz)

---

LibOS/shim/include/shim_ipc.h, line 268 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please comment whether this is inclusive or exclusive

If I describe start and end, then probably I should also describe type, but I'd rather not duplicate all the comments from struct posix_lock. How about I say "see struct posix_lock in shim_fs_lock.h" instead?

I could also embed struct posix_lock here... that's a bit ugly because it contains the list node, though.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

What's the lifetime/ownership of this? Why is it a pointer in the first place? I didn't read the implementation yet, but looks really weird and unconventional, so I think it deserves a comment :)

Added a comment.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

sorted by PID and then by start position

Am I right, that this sorting is actually only for pretty-printing purposes?

Update: this is also used to be able to merge locks, right? If so, could you comment about this here? Overall sorted lists are rarely useful (because they are O(n^2) anyways), so it's a bit puzzling when you read it.

I added a comment.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

including access to dentry `fs_lock`

That sounds weird (using a global lock to lock fields in some structures?), doesn't dentry already have its own lock?

I could use dent->lock, but I need the global lock for the global list anyway, at least when adding/removing/traversing it.

And actually I can't think of a way to traverse the list and process all the locks in posix_lock_clear_pid, any ideas?

---

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

Previously, mkow (Michał Kowalczyk) wrote…

What's the condition on whether other fields are in initialized state? It's quite implicit right now, it deserves a comment at the structure at least. The current comment sounds like some of the fields should be initialized depending on whether this request is an IPC one, but here you don't initialize anything except .pl.

I thought it would be clearer if we require the user to initialize all the fields. I grouped them in a struct { ... } notify to make that obvious.

(I also considered using a generic callback, arg pair, but it introduced another layer of indirection and did not look good.)

---

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

Previously, mkow (Michał Kowalczyk) wrote…

I'd prefer to have this on separate lines with aligned =.

Done.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

I think it would read easier if you swapped sides, so that it complements the condition above.

It was clearer for me to keep all comparisons "left to right", so that the variables read in the order they was encountered. I won't insist, but do you think I should drop that convention in this whole function, or in some of the checks only (and which ones)?

---

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

Previously, mkow (Michał Kowalczyk) wrote…

ditto (this time because we're iterating with cur, so it seems more natural to have it on the left)

Yeah, here there's no reason to keep it the opposite way. Done.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

start <= cur->start

Could you swap sides? Would be easier to notice that this is complement to the conditions above.

See above discussion about "left to right".

---

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

Previously, mkow (Michał Kowalczyk) wrote…

no space after assert

Fixed.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

wow, that loop was tough

I hope the ASCII art helped!

---

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

Previously, mkow (Michał Kowalczyk) wrote…

bool wait

This name seems a bit misleading, because this function is not actually waiting if it's true, it just inserts a wait request into a list and returns.

There are multiple functions using this bool wait, and while some of them schedule a request until later instead of waiting, I'd still argue they describe the same concept. So I'm not sure if I should rename just some of these parameters...

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Why this always has to be the case here?

Because posix_lock_add_request is allowed to add a request only if wait is true.

I'm not sure how to make that clearer. One idea:

if (wait) {
    ret = posix_lock_set_or_add_request(dent, pl, &req);
} else {
    ret = posix_lock_set_or_give_up(dent, pl);
}

But that introduces too much duplication (between those two functions) to be worth it, IMO.

Another one:

ret = posix_lock_set_or_add_request(dent, pl, wait ? &req : NULL);

But I think that's abusing the out_req parameter.

So for now, I added a short comment to posix_lock_set_or_add_request.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

please rewrap

Done.

---

LibOS/shim/src/sys/shim_fcntl.c, line 70 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Couldn't we just call fs->fs_ops->seek() and let it do the calculations?

I thought so too, but seek() will change the file position. So I'm only allowed to call it with (0, SEEK_CUR) to find the current position.

[mkow]

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)

---

LibOS/shim/include/shim_ipc.h, line 268 at r6 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

If I describe start and end, then probably I should also describe type, but I'd rather not duplicate all the comments from struct posix_lock. How about I say "see struct posix_lock in shim_fs_lock.h" instead?

I could also embed struct posix_lock here... that's a bit ugly because it contains the list node, though.

"see struct posix_lock in shim_fs_lock.h" sounds good

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Added a comment.

Much better now, thanks!

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

I could use dent->lock, but I need the global lock for the global list anyway, at least when adding/removing/traversing it.

And actually I can't think of a way to traverse the list and process all the locks in posix_lock_clear_pid, any ideas?

Is there anything wrong with keeping this lock constrained only to the global list and dentry locks to their own fields? When traversing the global list you'd need to take the list lock and then try to lock all the entries when inspecting them.
Be careful about the lock hierarchy though (right now it's global lock -> dentry lock, right?).

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

It was clearer for me to keep all comparisons "left to right", so that the variables read in the order they was encountered. I won't insist, but do you think I should drop that convention in this whole function, or in some of the checks only (and which ones)?

For me in code like this I need to follow which cases are handled where, and this is much easier if I have if (a < b) and if (a >= b) instead of if (b <= a).

Let's ask @dimakuv and @boryspoplawski for their preferences.

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

I hope the ASCII art helped!

Yup, without it I'd probably just block with "please explain what's happening here" :)

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Because posix_lock_add_request is allowed to add a request only if wait is true.

I'm not sure how to make that clearer. One idea:

if (wait) {
    ret = posix_lock_set_or_add_request(dent, pl, &req);
} else {
    ret = posix_lock_set_or_give_up(dent, pl);
}

But that introduces too much duplication (between those two functions) to be worth it, IMO.

Another one:

ret = posix_lock_set_or_add_request(dent, pl, wait ? &req : NULL);

But I think that's abusing the out_req parameter.

So for now, I added a short comment to posix_lock_set_or_add_request.

Ok, maybe then just add "posix_lock_add_request is allowed to add a request only if wait is true" as a comment?

---

LibOS/shim/src/sys/shim_fcntl.c, line 70 at r7 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I thought so too, but seek() will change the file position. So I'm only allowed to call it with (0, SEEK_CUR) to find the current position.

Could you say this in a comment in the code? I guess we won't be the last wondering about this :)

[boryspoplawski]

Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @pwmarcz)

---

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

Previously, mkow (Michał Kowalczyk) wrote…

For me in code like this I need to follow which cases are handled where, and this is much easier if I have if (a < b) and if (a >= b) instead of if (b <= a).

Let's ask @dimakuv and @boryspoplawski for their preferences.

I prefer the current version. The pids in this list form a monotonic sequence / range (I guess so at least, didn't read the code other than these 2 ifs) so for me it's more natural to always use </<= as comparison.

[dimakuv]

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)

---

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

Previously, boryspoplawski (Borys Popławski) wrote…

I prefer the current version. The pids in this list form a monotonic sequence / range (I guess so at least, didn't read the code other than these 2 ifs) so for me it's more natural to always use </<= as comparison.

Sounds like we're doing bike shedding here :) I have no global preference. In this particular case, there is no mental effort for me to see that "we are searching for lock with higher PID, at which point we exit the loop; we memorize the lock with lower PID; if the lock has the same PID, then we go into the split-merge lock ranges logic".

I mean, these are five lines of code close to each other and doing continue/break. If these IF clauses would expand to 50 LoC each, then I would prefer Michal's suggestion (clauses complement each other). But here it's too tiny to bother.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Yup, without it I'd probably just block with "please explain what's happening here" :)

This ASCII art really helped! I took a note of this way of representation, will use it also.

---

LibOS/shim/src/sys/shim_fcntl.c, line 70 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you say this in a comment in the code? I guess we won't be the last wondering about this :)

+1 to add the comment about the usage of seek(0, SEEK_CUR) in this code.

[mkow]

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Sounds like we're doing bike shedding here :) I have no global preference. In this particular case, there is no mental effort for me to see that "we are searching for lock with higher PID, at which point we exit the loop; we memorize the lock with lower PID; if the lock has the same PID, then we go into the split-merge lock ranges logic".

I mean, these are five lines of code close to each other and doing continue/break. If these IF clauses would expand to 50 LoC each, then I would prefer Michal's suggestion (clauses complement each other). But here it's too tiny to bother.

Ok, unblocking then.

@dimakuv: The problem is that this is a part of a function which was really hard to check thoroughly for correctness, partially because of the amount of conditions (I had to check for all cases which could be missed in those IFs).

[pwmarcz]

Reviewable status: 12 of 17 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

LibOS/shim/include/shim_ipc.h, line 268 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

"see struct posix_lock in shim_fs_lock.h" sounds good

Done.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Is there anything wrong with keeping this lock constrained only to the global list and dentry locks to their own fields? When traversing the global list you'd need to take the list lock and then try to lock all the entries when inspecting them.
Be careful about the lock hierarchy though (right now it's global lock -> dentry lock, right?).

Unfortunately, it's the opposite. We first take dent->lock, and usually that's enough; but sometimes we need to take the global lock to add to the list.

See my current solution. If you are not impressed by my Three Star Programming, I'm open to suggestions :)

---

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

Previously, mkow (Michał Kowalczyk) wrote…

Ok, maybe then just add "posix_lock_add_request is allowed to add a request only if wait is true" as a comment?

Done.

---

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

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Added a comment here too.

---

LibOS/shim/src/sys/shim_fcntl.c, line 70 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1 to add the comment about the usage of seek(0, SEEK_CUR) in this code.

Done.

[pwmarcz]

Reviewable status: 12 of 17 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

LibOS/shim/test/ltp/ltp.cfg, line 372 at r9 (raw file):

timeout = 60

# depends on POSIX locks returning EINTR after signal, which we don't support

Failed on Jenkins... by my reading of the code, the test actually depends on this EINTR even when it passes.

[dimakuv]

Reviewed 4 of 5 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)

[mkow]

Reviewed 4 of 5 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

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

Three Star Programming

Elite!

[mkow]

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[mkow]

Reviewed 4 of 4 files at r11.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

[dimakuv]

Reviewed 4 of 4 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved