[LibOS] Move fully to inodes #428

pwmarcz · 2022-02-24T14:38:46Z

Description of the changes

This change removes the state dentry field, and uses the inode field directly: a dentry is either positive (with inode) or negative (without inode). Dentry fields describing the file (type, perm) are also gone.

As a consequence, negative lookups are no longer cached: a negative dentry only means that a file is not known to exist.

How to test this PR?

Jenkins should be enough.

What's next

Cleanup: using inode as a parameter should simplify the implementation of individual filesystems.

This change is

dimakuv

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 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 238 at r1 (raw file):

     * \brief Unlink a file
     *
     * \param dent dentry, full, must have a parent

full? Why not just postive?

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

     * possible, they should still work.
     *
     * The caller should hold `g_dcache_lock`. On success, the caller detach the inode from the

the caller should detach

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

     *
     * The caller should hold `g_dcache_lock`. On success, the caller should update
     * `dent->inode->perm`.

Is this one of the TODOs for the follow-up PR? That chmod() will use inode instead of this?

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

        buf_puts(&buf, "M");
    } else if (!dent->parent) {
        buf_puts(&buf, "*");

Why did you remove these two?

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

         * directly. */
        mount_point = g_dentry_root;
        get_dentry(g_dentry_root);

How did it work before? Why did you have to add this new case?

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

    if (cur_dent != *dent) {
        get_dentry(cur_dent);
        put_dentry(*dent);

Any reason you swapped these two places?

pwmarcz

Reviewable status: 16 of 18 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 and @pwmarcz)

a discussion (no related file):
This PR conflicts with #425 (marking dentries as deleted) and #420 (Doxygen formatting). It looks like both will be merged sooner, so I will be rebasing this PR afterwards.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

full? Why not just postive?

My mistake, fixed (this is a leftover from an earlier version of this patch, where I considered "empty/full" instead of "negative/positive").

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

the caller should detach

Fixed, thanks.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is this one of the TODOs for the follow-up PR? That chmod() will use inode instead of this?

Possibly. I was thinking also about lookup, which right now has to create an inode and attach it to a dentry, and it could just create an inode.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why did you remove these two?

I removed it, because I thought it duplicates the information I added to root dentries recently (e.g. bin/ (chroot "file:/bin")): you already see where the root dentry is, and the one above it is the mountpoint.

But looking at the output again, I think it's still helpful to have the mountpoint marked with "M": mountpoints are special because they're shadowed by the mounted filesystem.

Example:

[P1:T1:helloworld] [ synth   2] 040555 dr-x      usr/
[P1:T1:helloworld] [ synth   2] 040555 dr-x        lib/
[P1:T1:helloworld] [ synth   2] 040555 dr-x M        x86_64-linux-gnu/
[P1:T1:helloworld] [chroot   1] 040755 drwx            x86_64-linux-gnu/ (chroot "file:/usr//lib/x86_64-linux-gnu")
[P1:T1:helloworld] [ synth   2] 040555 dr-x M    bin/
[P1:T1:helloworld] [chroot   1] 040755 drwx        bin/ (chroot "file:/bin")
[P1:T1:helloworld] [ synth   2] 040555 dr-x      mnt/
[P1:T1:helloworld] [ synth   2] 040555 dr-x M      tmpfs/
[P1:T1:helloworld] [ tmpfs   1] 040700 drwx          tmpfs/ (tmpfs "")

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

How did it work before? Why did you have to add this new case?

This is a consequence of adding the rule that "if a file exists, it must have an inode". All filesystems follow this rule, but for g_dentry_root we just marked the dentry with DENTRY_VALID flag so that it can be looked up. After all, nobody will make any operations on this dentry, because it's going to be shadowed by the mounted filesystem.

After this refactoring, we can't get away with that anymore (and there's not even a flag to set anymore). I could create a fake inode for g_dentry_root, but I want inodes to belong to a mounted filesystem (inode->mount != NULL), and here there isn't any. So the easiest way forward is to say that this file does not exist.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Any reason you swapped these two places?

In the previous version, I was tempted to leave out the if (cur_dent != *dent) condition in order to simplify this function. But if put_dentry is first, that actually can break: if cur_dent == *dent, then we can end up deallocating the dentry.

So, even though it doesn't matter as long as the condition is in place, I slightly prefer the current order.

pwmarcz

Reviewable status: 16 of 18 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 and @pwmarcz)

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Possibly. I was thinking also about lookup, which right now has to create an inode and attach it to a dentry, and it could just create an inode.

Oh, but if you mean "the caller should update dent->inode->perm", this is actually done in this PR.

dimakuv

Reviewed 2 of 2 files at r2, all commit messages.
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)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This PR conflicts with #425 (marking dentries as deleted) and #420 (Doxygen formatting). It looks like both will be merged sooner, so I will be rebasing this PR afterwards.

#425 will be merged very soon, so yeah.

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

Oh, but if you mean "the caller should update dent->inode->perm", this is actually done in this PR.

No, your first reply was the one I was asking about :) Resolving.

pwmarcz

Reviewable status: 16 of 18 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @pwmarcz)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

#425 will be merged very soon, so yeah.

Rebased after #425.

dimakuv

Reviewed 2 of 2 files at r3, 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)

-- commits, line 15 at r3:
What is this? The fixup commit belongs to the wrong commit...

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 @pwmarcz)

-- commits, line 15 at r3:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is this? The fixup commit belongs to the wrong commit...

My bad. I think I used @^ instead of @ for fixup. Will fix during next rebase (after #420 is merged).

mkow

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

This change removes the `state` dentry field, and uses the `inode` field directly: a dentry is either positive (with inode) or negative (without inode). Dentry fields describing the file (`type`, `perm`) are also gone. As a consequence, negative lookups are no longer cached: a negative dentry only means that a file is not known to exist. Signed-off-by: Paweł Marczewski <[email protected]>

pwmarcz

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Rebased after #425.

And rebased after #420.

-- commits, line 15 at r3:

Previously, pwmarcz (Paweł Marczewski) wrote…

My bad. I think I used @^ instead of @ for fixup. Will fix during next rebase (after #420 is merged).

Done.

dimakuv

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

mkow

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

pwmarcz mentioned this pull request Feb 24, 2022

Inodes #279

Closed

dimakuv suggested changes Feb 25, 2022

View reviewed changes

pwmarcz commented Feb 25, 2022

View reviewed changes

dimakuv previously approved these changes Feb 28, 2022

View reviewed changes

pwmarcz dismissed dimakuv’s stale review via ed50e97 February 28, 2022 17:41

pwmarcz force-pushed the pawel/inode-2 branch from 7472523 to ed50e97 Compare February 28, 2022 17:41

pwmarcz commented Feb 28, 2022

View reviewed changes

dimakuv suggested changes Mar 1, 2022

View reviewed changes

pwmarcz commented Mar 1, 2022

View reviewed changes

mkow previously approved these changes Mar 1, 2022

View reviewed changes

pwmarcz dismissed mkow’s stale review via 29f1275 March 3, 2022 14:24

pwmarcz force-pushed the pawel/inode-2 branch from 1c9299c to 29f1275 Compare March 3, 2022 14:24

pwmarcz commented Mar 3, 2022

View reviewed changes

dimakuv approved these changes Mar 3, 2022

View reviewed changes

mkow approved these changes Mar 3, 2022

View reviewed changes

pwmarcz merged commit 29f1275 into master Mar 3, 2022

pwmarcz deleted the pawel/inode-2 branch March 3, 2022 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LibOS] Move fully to inodes #428

[LibOS] Move fully to inodes #428

pwmarcz commented Feb 24, 2022 •

edited by mkow

Loading

dimakuv left a comment

pwmarcz left a comment

pwmarcz left a comment

dimakuv left a comment

pwmarcz left a comment

dimakuv left a comment

pwmarcz left a comment

mkow left a comment

pwmarcz left a comment

dimakuv left a comment

mkow left a comment

[LibOS] Move fully to inodes #428

[LibOS] Move fully to inodes #428

Conversation

pwmarcz commented Feb 24, 2022 • edited by mkow Loading

Description of the changes

How to test this PR?

What's next

dimakuv left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

pwmarcz commented Feb 24, 2022 •

edited by mkow

Loading