Take g_dcache_lock in dentry-related functions

[pwmarcz]

Description of the changes

This commit makes holding g_dcache_lock mandatory when calling path_lookupat, dentry_open and dentry callbacks (shim_d_ops). The callbacks are now also better documented.

This is in preparation for removing the per-object dentry lock (shim_dentry.lock), and later, switching to inodes. It should also make the filesystem functions less prone to race conditions.

Context: #279 (inodes migration). Should also help with gramineproject/graphene#944 (but since I touch only some of the filesystem code, I won't claim it's fixed yet).

How to test this PR?

I spent some time making sure the callbacks are called according to documentation; I would be grateful for a double-check.

---

This change is 

[dimakuv]

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 17 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.h, line 220 at r1 (raw file):

    /* detach internal data from dentry */
    int (*dput)(struct shim_dentry* dent);

How is this unused? Don't we need to free resources of dent?

---

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

     * \brief Create and open a new regular file
     *
     * \param hdl a newly created handle

You're missing description of dir param.

---

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

    int (*follow_link)(struct shim_dentry* dent, char** out_target);
    /* set up symlink name to a dentry */
    int (*set_link)(struct shim_dentry* dent, const char* link);

This was never used?

---

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

     * \param dent dentry, valid and negative
     * \param flags open flags, including access mode (O_RDONLY / O_WRONLY / O_RDWR)
     * \param perm permissions of the new file

No such param

---

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

    /* change the mode or owner of a file; the caller has to update dentry */
    int (*chmod)(struct shim_dentry* dent, mode_t perm);
    int (*chown)(struct shim_dentry* dent, int uid, int gid);

We don't emulate this? I thought we have (dummy) emulation of chown() syscall.

---

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

     * dentry (as in `lookup`).
     */
    int (*mkdir)(struct shim_dentry* dent, mode_t perm);

I'm confused here. What is dent -- the directory-to-be-created or the parent directory? If the former, then the caller is supposed to prepare the dentry for consumption by mkdir(), right? But you don't describe how it should be prepared. If the latter, then how do we know the dir name?

The previous function signature was intuitively understandable to me. The new signature I don't get without more comments.

UPDATE: Actually, dir was unused even in the previous code. So dent is a directory-to-be-created. Could you make it a bit more explicit in the comment?

---

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

     * \param dent dentry, valid and non-negative, must have a parent
     *
     * Unlinks a file under `dent`. Note that there might be handles for that file; if possible,

I don't like the word under. Maybe represented by? Or corresponding to?

---

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

     * \brief Get file status
     *
     * \param dentry, valid and non-negative

You should add \param buf

---

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

     * \brief Get file status
     *
     * \param dentry, valid and non-negative

\param dentry, -> \param dent dentry,

---

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

     *
     * \param dent dentry, valid and non-negative, describing a symlink
     * \param[out] out_target on success, contains link target

Ideally, we should reshuffle and rename all functions' arguments here to have out_ prefix.

Here, you don't really need [out] specifier -- the arg name already tells us that.

---

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

     * Changes the permissions for a file.
     *
     * The caller should hold `g_dcache_lock`. On success, the caller should update `dent->perm`.

The caller should update? Isn't it what this function is supposed to do?

---

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

     * \param new target dentry, valid, can be negative or non-negative
     *
     * Renames a file under `old` to the path described by `new`. Updates the fields of `new` (same

I don't like the word under. Maybe described by?

---

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

     * \brief List all files in the directory
     *
     * \param dentry the dentry, must be valid, non-negative and describing a directory

dentry -> dent

---

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

    }
    hdl->flags = flags;
    hdl->acc_mode = ACC_MODE(flags & O_ACCMODE);

Is this just cleanup? What happens e.g. for PSEUDO_STR dentries? How will they have these fields assigned? Or they don't need these fields at all?

---

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

static int chroot_open(struct shim_handle* hdl, struct shim_dentry* dent, int flags) {
    int ret;

Why don't you have assert(locked(&g_dcache_lock));?

Is this because you still have dent->lock here, and you don't want to have deadlocks/weirdness by introducing another lock?

Then your commit description and comments in shim_d_ops are incorrect. Please either add some FIXME comments here, or make it explicit in the commit description/shim_d_ops that we are in transition state.

(Same applies to all dentry callbacks.)

---

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

            goto out;
    } else {
        dent->state |= DENTRY_PERSIST;

Was there any sense in this flag?

---

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

    if (dir)
        put_dentry(dir);
    return ret;

Can we re-write this function with gotos? This is unreadable...

[pwmarcz]

Reviewable status: 15 of 20 files reviewed, 17 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.h, line 220 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

How is this unused? Don't we need to free resources of dent?

Yes, I think the intention was to free dent->data (which currently leaks), but this callback wasn't called.

I tried to fix it before, but at the time ran into problems because some filesystems had really weird ownership patterns. Now, having rewritten much of that code, I should be able to add the dput callback. But my plan is to remove dent->data and replace it with inode->data, so don't want to have to fix it twice.

(There's a similar issue with migrating dentry data, I also want to fix it for inodes).

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You're missing description of dir param.

Right. I actually forgot to remove it: the callbacks don't use it, and there's dent->parent for that. Fixed.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This was never used?

I think so. It would be used for the symlink() syscall but we never supported it. We only have symlinks in pseudo-FSes like /proc.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No such param

I forgot to rename mode below. Fixed.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We don't emulate this? I thought we have (dummy) emulation of chown() syscall.

We do, but it doesn't even call a callback (and I think that's fine).

---

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

I'm confused here. What is dent -- the directory-to-be-created or the parent directory? If the former, then the caller is supposed to prepare the dentry for consumption by mkdir(), right? But you don't describe how it should be prepared.

The former. I don't describe how, because this is filesystem-specific: for some filesystems, it might involve updating some stuff in data or handling the inode (at least for now); for others, dent might require no extra preparation.

All these functions (lookup, creat, mkdir) follow the same pattern: they receive a dentry, and create a new file in place pointed to by that dentry. I tried to make that more clear in the description.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't like the word under. Maybe represented by? Or corresponding to?

Changed to described by throughout this PR.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You should add \param buf

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

\param dentry, -> \param dent dentry,

Done.

---

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

Ideally, we should reshuffle and rename all functions' arguments here to have out_ prefix.

If you mean dent, I disagree. The dent parameter is both read and written to, and it's usually the main object (in OOP way). Also, the rule (perhaps implicit) that I've been following is to only treat output pointers this way:

/* these functions act on existing objects: */
int lookup(struct shim_dentry* dent);
int open(struct shim_dentry* dent, ...);
ssize_t read(..., void* buf, size_t size);

/* these functions retrieve or create new objects: */
int path_lookupat(..., struct shim_dentry** out_dent);
int follow_link(..., char** out_target);

Here, you don't really need [out] specifier -- the arg name already tells us that.

OK, removed.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The caller should update? Isn't it what this function is supposed to do?

No. The idea is that these callbacks do only filesystem-specific processing, e.g. chmod will call DkStreamAttributesSetByHandle. Any updates that we always do (like updating perm, or marking the dentry as valid) are done in the general FS layer: this way, there's less duplication and less opportunities to get them wrong.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't like the word under. Maybe described by?

Changed to represented by.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

dentry -> dent

Done.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is this just cleanup? What happens e.g. for PSEUDO_STR dentries? How will they have these fields assigned? Or they don't need these fields at all?

I moved this up to assoc_handle_with_dentry, since this update should be done always.

---

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

Why don't you have assert(locked(&g_dcache_lock));?

Because I'm not accessing any fields that, by current rules, require this lock. I could add such assert, but the code here doesn't need it.

We really only require g_dcache_lock for dentry structure, i.e. children/siblings and possibly state. It's still pretty broken but I'm fixing it step by step. This commit does not change the locking rules except for calling the callbacks under g_dcache_lock.

Is this because you still have dent->lock here, and you don't want to have deadlocks/weirdness by introducing another lock?

The assert wouldn't introduce any new deadlock or behavior change, right?

The dent->lock is here, because (by current rules) it protects dent->inode.

Then your commit description and comments in shim_d_ops are incorrect.

Which part is incorrect? I claim to call callbacks with g_dcache_lock, not to change other rules for using this lock.

Please either add some FIXME comments here, or make it explicit in the commit description/shim_d_ops that we are in transition state.

I added a comment to the top of shim_dentry about the current state.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Was there any sense in this flag?

I think the idea was to prevent deleting such "persisted" dentries from cache, because they have important state that would be gone. But right now, we don't make such distinction.

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we re-write this function with gotos? This is unreadable...

Done. It looks like I also overlooked the stat() call above. The part above looks like it deserves its own helper function.

[dimakuv]

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

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Yes, I think the intention was to free dent->data (which currently leaks), but this callback wasn't called.

I tried to fix it before, but at the time ran into problems because some filesystems had really weird ownership patterns. Now, having rewritten much of that code, I should be able to add the dput callback. But my plan is to remove dent->data and replace it with inode->data, so don't want to have to fix it twice.

(There's a similar issue with migrating dentry data, I also want to fix it for inodes).

OK, thanks, makes sense.

---

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

If you mean dent, I disagree. The dent parameter is both read and written to, and it's usually the main object (in OOP way). Also, the rule (perhaps implicit) that I've been following is to only treat output pointers this way

I agree with you. I didn't mean dent here. I meant e.g. creat(hdl) above, but I now see that all these objects are already created by the caller (so all these dent callbacks manipulate the objects in-place). So it looks like there are no other func arguments that require the out_ prefix.

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

No. The idea is that these callbacks do only filesystem-specific processing, e.g. chmod will call DkStreamAttributesSetByHandle. Any updates that we always do (like updating perm, or marking the dentry as valid) are done in the general FS layer: this way, there's less duplication and less opportunities to get them wrong.

Ok, got it now.

---

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Why don't you have assert(locked(&g_dcache_lock));?

Because I'm not accessing any fields that, by current rules, require this lock. I could add such assert, but the code here doesn't need it.

We really only require g_dcache_lock for dentry structure, i.e. children/siblings and possibly state. It's still pretty broken but I'm fixing it step by step. This commit does not change the locking rules except for calling the callbacks under g_dcache_lock.

Is this because you still have dent->lock here, and you don't want to have deadlocks/weirdness by introducing another lock?

The assert wouldn't introduce any new deadlock or behavior change, right?

The dent->lock is here, because (by current rules) it protects dent->inode.

Then your commit description and comments in shim_d_ops are incorrect.

Which part is incorrect? I claim to call callbacks with g_dcache_lock, not to change other rules for using this lock.

Please either add some FIXME comments here, or make it explicit in the commit description/shim_d_ops that we are in transition state.

I added a comment to the top of shim_dentry about the current state.

OK, understood now.

---

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

It looks like I also overlooked the stat() call above.

What do you mean by this? d_ops->stat() call? But it looks the same to me, the old version and the new version.

---

LibOS/shim/src/sys/shim_stat.c, line 266 at r2 (raw file):

    }
    if (!*pathname && !(flags & AT_EMPTY_PATH))
        return -ENOENT;

But you already have this check two lines below.

[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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

It looks like I also overlooked the stat() call above.

What do you mean by this? d_ops->stat() call? But it looks the same to me, the old version and the new version.

I meant the d_ops->stat() call in the "empty path" branch (if (!*pathname))). It was called without holding the lock.

(I wonder if there's some static analysis tool that would help us here... I heard Linux has something but I haven't looked at it).

---

LibOS/shim/src/sys/shim_stat.c, line 266 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

But you already have this check two lines below.

Oops! Yes, that's a leftover from when I hoped to fit this in single fuction. Removed.

[dimakuv]

Reviewed 1 of 1 files at r3, 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 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved