[LibOS] Rewrite path lookup functions #2333

pwmarcz · 2021-04-26T12:19:57Z

This is the next step in FS rewrite, after #2324 (the first commit should disappear after merging #2324).

See also the issue for dentry cleanup (#2321).

Initially, I wanted to reverse the "follow symlinks" semantics (LOOKUP_FOLLOW -> LOOKUP_NO_FOLLOW). I decided against that after seeing Linux and BSD have a flag like that, but found that FreeBSD has a convention to also use NOFOLLOW (defined as zero) to make the code clearer.

Apart from that (and general cleanup), the most important change is LOOKUP_CREATE and not returning -ENOENT when the file can be created.

Next step: rewrite mount_fs and make mount semantics more sane (as mentioned in #2324).

This change is

pwmarcz

Reviewable status: 0 of 17 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)

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

#define DUMP_FLAG(flag, s, empty) buf_puts(&buf, (dent->state & flag) ? s : empty)

static void dump_dentry(struct shim_dentry* dent, unsigned int level) {

For now, I'm manually adding dump_dentry(NULL); in libos_clean_and_exit(). Any idea how best I can enable it without having to make local modifications to the code? I would stick it at log_trace(), but it's probably not useful if you're not working on the filesystem specifically.

dimakuv

Reviewed 17 of 17 files at r1.
Reviewable status: all files reviewed, 25 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)

a discussion (no related file):

Instead, the code now treats such cases as a success and create a

Typo in commit message: create -> creates.

a discussion (no related file):

This also adds a LOOKUP_NO_FOLLOW value to make the the call sites

One more typo in commit message: the the -> the

a discussion (no related file):
Awesome PR!

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

};

/* TODO: This actually does not get migrated after a fork, we just copy `g_process.root` */

Is g_process.root equal to g_dentry_root? In this case, you're right, we should simply use the migration mechanism instead of copying.

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

#define LOOKUP_DIRECTORY 002
#define LOOKUP_CONTINUE  004  // No longer needed
#define LOOKUP_PARENT    010  // Not sure we need this

I just love these old comments! :)

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

 * -ENOENT).
 */
int check_permissions(struct shim_dentry* dent, mode_t mask);

Shouldn't you start this function name with _ because it assumes a lock?

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

 * LOOKUP_NO_FOLLOW as a pseudo-flag for readability.
 *
 * This is modeled after Linux and and BSD codebases, which define a positive FOLLOW flag, and a

typo: and and -> and

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

 *
 * On success, returns 0, and puts the found dentry in `*new`. The reference count of the dentry
 * will be increased by one.

increased by one == set to one? The wording is confusing -- we just created the dentry and we set the refcount, there is nothing to increase.

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

 *
 * On failure, returns a negative error code, and sets `*new` to NULL. (Note that in case of
 * LOOKUP_CREATE, not all file-not-found errors are considered a failure).

What does this mean? Could you give an example? By file-not-found errors, do you mean -ENOENT or do you mean file isn't there but can be created?

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

 * \param[out] hdl handle to initialize, can be NULL
 * \param start the start dentry for relative paths, or NULL (in which case it will default to
 * process' cwd)

You missed \param path

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

 * \param flags Unix open flags (see below)
 * \param mode Unix file mode, used when creating a new file/directory
 * \param[out] new pointer to retrieved dentry, can be NULL

Why new? Should be dent

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

 * will be just retrieved or created.
 *
 * If `new` is provided, on success it will be set to the files's dentry (and its reference count

Why new? Should be dent

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

 * will be just retrieved or created.
 *
 * If `new` is provided, on success it will be set to the files's dentry (and its reference count

Typo: files's dentry -> file's dentry

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

 * - O_TRUNC: truncate the file after opening
 *
 * The flags (including any not listed above), as well as file mode, are passed to the underlying

Why "including any not listed above"? Is this how Linux does it? Shouldn't we check and restrict the possible flags, just like we do in other places (e.g. mmap and futex)?

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

/*
 * \brief Open an already retrieved dentry

...and associate hdl with it

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

 * \brief Open an already retrieved dentry
 *
 * \param[out] hdl handle to initialize

Maybe not initialize but associate with the opened dent?

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

Previously, pwmarcz (Paweł Marczewski) wrote…

For now, I'm manually adding dump_dentry(NULL); in libos_clean_and_exit(). Any idea how best I can enable it without having to make local modifications to the code? I would stick it at log_trace(), but it's probably not useful if you're not working on the filesystem specifically.

Maybe just add this line that you manually add, and comment it out with a NOTE: ... comment?

I don't think we want to introduce any runtime flags for this. This is too FS-debugging-specific. To me it's fine to manually uncomment the line you'll add and do the debugging.

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

     *
     * TODO: current mount operation marks the mount root as synthetic. Once that's fixed, we'll be
     * able to disallow writes here.

I don't understand this comment. What writes?

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

         * I'm ok with a file system that doesn't care setting the
         * permission to all.
         */

What is this comment? Can we just remove it? It seems to be completely out of sync with the code.

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

err:
    if (dent)
        get_dentry(dent);

Why are we increasing the refcount on the error path? Even if we want to keep the dentry, wasn't its refcount already incremented in one of the above functions (lookup(), lookup_dcache())?

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

#define MAX_LINK_DEPTH 8

Looks like you forgot to undef it. But also, maybe we should move this macro to a header file? Looks like a rather important constant.

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

/*
 * This implementation is mostly iterative, but uses recursion to follow symlinks (which is why the
 * link depth is capped at 8).

8 -> MAX_LINK_DEPTH

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

    if ((flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
        retval &= ~LOOKUP_FOLLOW;

Could you add a comment why this subtle semantics ("do not follow symlinks if want to open exclusive")?

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

        /* Check permissions, but only if we didn't create the file: create/O_CREAT have
         * idiosyncratic semantics about opening a newly-created, read-only file for writing, but
         * only the first time. */

Could you explain this? I don't really follow this. If there is special treatment of O_CREAT during checking permissions, then where is it reflected in our code?

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

    struct shim_dentry* dent = NULL;

    if ((ret = path_lookupat(NULL, path, LOOKUP_ACCESS | LOOKUP_FOLLOW, &dent, NULL)) < 0)

This was a bug (LOOKUP_FOLLOW)?

pwmarcz

Reviewable status: 13 of 17 files reviewed, 25 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)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Instead, the code now treats such cases as a success and create a

Typo in commit message: create -> creates.

Fixed.

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This also adds a LOOKUP_NO_FOLLOW value to make the the call sites

One more typo in commit message: the the -> the

Fixed.

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Awesome PR!

Thanks!

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Is g_process.root equal to g_dentry_root? In this case, you're right, we should simply use the migration mechanism instead of copying.

I meant we migrate g_process.root, but leave g_dentry_root alone and it points to an empty directory. I reworded the comment.

Additional context:

After initialization, we use g_process.root pretty much everywhere, which is why this went undetected
The two are not necessarily the same, because we support chroot (although I'm not sure how good that support is).

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't you start this function name with _ because it assumes a lock?

Do we have this convention? I thought _func is a version of func that assumes a lock, but if there's only one of them, there's no need to mark them this way.

And from the looks of it (there are other functions such as lookup_dcache, get_new_dentry etc. that also assume the lock) I think it's not worth it to enforce a naming convention for all functions requiring the lock. (Also, while it's not checked statically, in DEBUG mode mistakes are easy to notice at runtime because of the asserts).

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

typo: and and -> and

Fixed.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

increased by one == set to one? The wording is confusing -- we just created the dentry and we set the refcount, there is nothing to increase.

This function does not necessarily create a new dentry. And even if it does, it adds it to the cache first, so there is one extra reference above the ones in cache.

I guess new is a pretty bad name. Changed tofound.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What does this mean? Could you give an example? By file-not-found errors, do you mean -ENOENT or do you mean file isn't there but can be created?

Yeah, it's not clear here. I meant that there are circumstances when the file does not exist but this function succeeds (if LOOKUP_CREATE is set).

I reworded the description: the description of LOOKUP_CREATE elaborates on this case, and here I just say that it is the only case in which we return a negative dentry. Is that better?

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You missed \param path

Fixed.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why new? Should be dent

I renamed it to found, for consistency with path_lookupat.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why new? Should be dent

I renamed it to found, for consistency with path_lookupat.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Typo: files's dentry -> file's dentry

Fixed.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why "including any not listed above"? Is this how Linux does it? Shouldn't we check and restrict the possible flags, just like we do in other places (e.g. mmap and futex)?

Is this the right place, though? This function is a proxy, and needs to handle only some of the flags. It's not the syscall entrypoint such as shim_do_openat - maybe we should restrict the flags there?

The analogous function in Linux (lookup_open) assumes that the flags are already validated, and passes them to underlying filesystem.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

...and associate hdl with it

Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe not initialize but associate with the opened dent?

Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe just add this line that you manually add, and comment it out with a NOTE: ... comment?

I don't think we want to introduce any runtime flags for this. This is too FS-debugging-specific. To me it's fine to manually uncomment the line you'll add and do the debugging.

I'd rather not add a commented-out line to libos_clean_and_exit (it's not so easy to discover, and I might need to print this from inside other places too), but I added a comment explaining that this function is to be added manually to code for debugging.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't understand this comment. What writes?

Changed to disallow W_OK here.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What is this comment? Can we just remove it? It seems to be completely out of sync with the code.

I understand it as a suggestion to remove d_ops->mode(), and instead initialize the mode together with the rest of dentry (in d_ops->lookup()). A filesystem that doesn't disallow anything could set the mode to something like 0777, and this entire branch could be removed.

I think it's a good idea, and the same might be done with d_ops->stat(). That way, the initial lookup() fills all the necessary information about a file. But I don't want to do it until I understand this part of the filesystem better.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why are we increasing the refcount on the error path? Even if we want to keep the dentry, wasn't its refcount already incremented in one of the above functions (lookup(), lookup_dcache())?

Oops, this should be put_dentry. Fixed.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Looks like you forgot to undef it. But also, maybe we should move this macro to a header file? Looks like a rather important constant.

I moved this constant to shim_fs.h.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

8 -> MAX_LINK_DEPTH

Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Could you add a comment why this subtle semantics ("do not follow symlinks if want to open exclusive")?

Okay, I added a quote from the man page.

And I just noticed that we should fail with when opening a link (with -ELOOP, apparently). I added that and tested it by trying to open /proc/self/cwd with O_NOFOLLOW.

LibOS/shim/src/fs/shim_namei.c, line 510 at r1 (raw file):
We are at the place where it's reflected in our code: this comment explains why the check is done here, and not outside of the if.

Here is the use case described in the comment, as replicated in Python:

# creating a new file
>>> os.open('xyz', os.O_CREAT | os.O_WRONLY, stat.S_IRUSR)
3
# opening it again
>>> os.open('xyz', os.O_CREAT | os.O_WRONLY, stat.S_IRUSR)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [Errno 13] Permission denied: 'xyz'

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

/* SPDX-License-Identifier: LGPL-3.0-or-later */

It's annoying to have both sys/shim_fs.h and fs/shim_fs.h, especially since they are different layers of the same thing. What do you think about renaming this file (later) to sys/shim_files.h?

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This was a bug (LOOKUP_FOLLOW)?

The man page for statfs doesn't say anything about symlinks, so I assumed it does not follow them.

But I just double-checked (in Linux sources, and experimentally) and I was wrong, it does follow symlinks. Fixed.

dimakuv

Reviewed 3 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 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):

Previously, pwmarcz (Paweł Marczewski) wrote…

Fixed.

I don't see any changes in the commit messages?

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Fixed.

ditto

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

Previously, pwmarcz (Paweł Marczewski) wrote…

I meant we migrate g_process.root, but leave g_dentry_root alone and it points to an empty directory. I reworded the comment.

Additional context:

After initialization, we use g_process.root pretty much everywhere, which is why this went undetected

The two are not necessarily the same, because we support chroot (although I'm not sure how good that support is).

OK, thanks, now the comment is good and I understand the issue.

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Do we have this convention? I thought _func is a version of func that assumes a lock, but if there's only one of them, there's no need to mark them this way.

And from the looks of it (there are other functions such as lookup_dcache, get_new_dentry etc. that also assume the lock) I think it's not worth it to enforce a naming convention for all functions requiring the lock. (Also, while it's not checked statically, in DEBUG mode mistakes are easy to notice at runtime because of the asserts).

OK, agreed. I had a feeling that "global function + assumes lock" means "start with underscore". But apparently not the case.

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

Previously, pwmarcz (Paweł Marczewski) wrote…

This function does not necessarily create a new dentry. And even if it does, it adds it to the cache first, so there is one extra reference above the ones in cache.

I guess new is a pretty bad name. Changed tofound.

OK. Thanks, this is better.

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Yeah, it's not clear here. I meant that there are circumstances when the file does not exist but this function succeeds (if LOOKUP_CREATE is set).

I reworded the description: the description of LOOKUP_CREATE elaborates on this case, and here I just say that it is the only case in which we return a negative dentry. Is that better?

OK, perfect.

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

Previously, pwmarcz (Paweł Marczewski) wrote…

Is this the right place, though? This function is a proxy, and needs to handle only some of the flags. It's not the syscall entrypoint such as shim_do_openat - maybe we should restrict the flags there?

The analogous function in Linux (lookup_open) assumes that the flags are already validated, and passes them to underlying filesystem.

OK, fair enough. I guess not for this PR, so not blocking.

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

Previously, pwmarcz (Paweł Marczewski) wrote…

I understand it as a suggestion to remove d_ops->mode(), and instead initialize the mode together with the rest of dentry (in d_ops->lookup()). A filesystem that doesn't disallow anything could set the mode to something like 0777, and this entire branch could be removed.

I think it's a good idea, and the same might be done with d_ops->stat(). That way, the initial lookup() fills all the necessary information about a file. But I don't want to do it until I understand this part of the filesystem better.

OK, let's keep it.

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

Previously, pwmarcz (Paweł Marczewski) wrote…

We are at the place where it's reflected in our code: this comment explains why the check is done here, and not outside of the if.

Here is the use case described in the comment, as replicated in Python:
# creating a new file
>>> os.open('xyz', os.O_CREAT | os.O_WRONLY, stat.S_IRUSR)
3
# opening it again
>>> os.open('xyz', os.O_CREAT | os.O_WRONLY, stat.S_IRUSR)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [Errno 13] Permission denied: 'xyz'

My confusion is more about the part of the comment opening a newly-created, read-only file for writing -- you don't check permissions at all when creating a file, but in the comment you have a very specific case. So the comment is too restrictive, whereas the code is simply "don't check permissions on newly-created file". So why do you mention this specific example in the comment (or maybe it's the only case?)?

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

Previously, pwmarcz (Paweł Marczewski) wrote…

It's annoying to have both sys/shim_fs.h and fs/shim_fs.h, especially since they are different layers of the same thing. What do you think about renaming this file (later) to sys/shim_files.h?

Which file? This file is shim_fs.c. I guess you meant these two:

shim/src/fs/shim_fs.c
shim/src/sys/shim_fs.c

I'm fine with renaming one of them. Your suggestion of sys/shim_files.c is fine.

pwmarcz

Reviewable status: all files reviewed, 4 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/src/sys/shim_fs.c, line 1 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Which file? This file is shim_fs.c. I guess you meant these two:

shim/src/fs/shim_fs.c

shim/src/sys/shim_fs.c

I'm fine with renaming one of them. Your suggestion of sys/shim_files.c is fine.

Yes, sorry, I meant .c (not .h) in all instances.

pwmarcz

Reviewable status: 16 of 17 files reviewed, 3 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)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't see any changes in the commit messages?

Oh, I didn't save the changed message before rebasing, sorry.

Should be good now.

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Done.

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

My confusion is more about the part of the comment opening a newly-created, read-only file for writing -- you don't check permissions at all when creating a file, but in the comment you have a very specific case. So the comment is too restrictive, whereas the code is simply "don't check permissions on newly-created file". So why do you mention this specific example in the comment (or maybe it's the only case?)?

That's not my comment, but I agree: this case is more general, you can even set mode to 0.

I removed the example, and reworded the comment to describe the case directly (the mode can be incompatible with acc_mode).

dimakuv

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

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

Previously, pwmarcz (Paweł Marczewski) wrote…

That's not my comment, but I agree: this case is more general, you can even set mode to 0.

I removed the example, and reworded the comment to describe the case directly (the mode can be incompatible with acc_mode).

OK. I didn't notice that you copied this comment from the previous code (I didn't dare to look at the old code). Now I understand the context for this :) Thanks for rewording the comment!

dimakuv

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

mkow

Reviewed 13 of 17 files at r1, 2 of 4 files at r2, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

LibOS/shim/include/shim_fs.h, line 389 at r5 (raw file):

but not permissions for the whole path.

So, the caller is expected to do this? Is it actually implemented in the caller or do we just skip these checks? If the latter, there should be a TODO here, I think.

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

}

#define DUMP_FLAG(flag, s, empty) buf_puts(&buf, (dent->state & flag) ? s : empty)

I know this macro is used only in the function below, but please still write it safely :) I.e. parenthesize the parameters.

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

bool make_ancestor

I think we could remove this argument and always assume it's true. But I guess you'll refactor this when fixing mountpoints.

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

    assert(start);
    dent = start;
    get_dentry(dent);

Any reason why couldn't we simplify this logic and just get the final, to-be-returned dentry right before returning?

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

is_opened

I think grammatically correct form is either is_open or is_opened.

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

        if ((ret = lookup_dentry(dent, d->name, strlen(d->name), &child)) < 0) {
            /* ENOENT from underlying lookup should be handled as DENTRY_NEGATIVE */
            assert(ret != ENOENT);

Not -ENOENT?

pwmarcz

Reviewable status: 14 of 17 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 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 389 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

but not permissions for the whole path.
So, the caller is expected to do this? Is it actually implemented in the caller or do we just skip these checks? If the latter, there should be a TODO here, I think.

That's right, the check should be here. Or actually, it probably belongs to path_lookupat. I added a TODO to both of these functions.

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

Previously, mkow (Michał Kowalczyk) wrote…

I know this macro is used only in the function below, but please still write it safely :) I.e. parenthesize the parameters.

Done.

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

Previously, mkow (Michał Kowalczyk) wrote…

bool make_ancestor
I think we could remove this argument and always assume it's true. But I guess you'll refactor this when fixing mountpoints.

Yes, I will be rewriting this function completely, when I change how mounts work. I don't want to touch the current implementation too much until then. And I agree, it should always create a synthetic node.

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

Previously, mkow (Michał Kowalczyk) wrote…

Any reason why couldn't we simplify this logic and just get the final, to-be-returned dentry right before returning?

That's because this function calls other functions that give it a dentry with increased reference count (lookup_dentry, and itself, recursively, when following a symlink). So it needs to adjust some reference counts in the middle anyway.

Also, not keeping a reference for the intermediate dentries is only correct assuming nothing can delete them in the meantime. This assumption is true now (because we keep g_dcache_lock for the whole time), but might not be later (if we e.g. release the lock during low-level filesystem lookup), and since keeping the ref counts is not a big deal here, I'd rather make the code not depend on it.

Overall, I'd say it's better to keep the "correct" reference counting discipline, unless a function is really simple (ideally doesn't call anything else).

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

Previously, mkow (Michał Kowalczyk) wrote…

is_opened
I think grammatically correct form is either is_open or is_opened.

I thought about it a bit more, and I can just check hdl->dentry. The handle really should be unassociated before that (looking at the call sites confirms it, and I added an assert just in case).

(I did not like the is_open name because it somehow didn't suggest to me that it was opened in this function. So I started thinking about names like initialized. But it's better to avoid this variable completely).

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

Previously, mkow (Michał Kowalczyk) wrote…

Not -ENOENT?

That's right, this assert did nothing. Fixed.

mkow

Reviewed 3 of 3 files at r6.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

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

Previously, pwmarcz (Paweł Marczewski) wrote…

That's because this function calls other functions that give it a dentry with increased reference count (lookup_dentry, and itself, recursively, when following a symlink). So it needs to adjust some reference counts in the middle anyway.

Also, not keeping a reference for the intermediate dentries is only correct assuming nothing can delete them in the meantime. This assumption is true now (because we keep g_dcache_lock for the whole time), but might not be later (if we e.g. release the lock during low-level filesystem lookup), and since keeping the ref counts is not a big deal here, I'd rather make the code not depend on it.

Overall, I'd say it's better to keep the "correct" reference counting discipline, unless a function is really simple (ideally doesn't call anything else).

Ok, makes sense, I was just curious why you wrote it this way :)

This commit rewrites `path_lookupat` and `open_namei` functions, as well as related helper functions. - Rewrite the algorithm to be iterative, with the exception of symlink lookup which remains recursive. - Distinguish the case when a file does not exist, but can be created (if LOOKUP_CREATE is set): previously, the code handled -ENOENT specially and checked if a dentry got created anyway, which was confusing and incorrect with respect to reference counting. Instead, the code now treats such cases as a success, and creates a negative dentry. On failures, no dentry is returned (the code sets the pointer to NULL), and the temporary dentry that has been created is ready to be garbage-collected. - Follow Linux semantics more closely: make LOOKUP_FOLLOW control only what we do with the last path segment (the intermediate segments always should follow symlinks), and treat paths ending with slash specially. This also adds a LOOKUP_NO_FOLLOW value to make the call sites more clear, and changes the call sites to always use one or the other. (Most Graphene filesystems do not handle symlinks yet, but this prepares the ground for them). - Rename DENTRY_ANCESTOR (synthetic nodes created for mount purposes) to DENTRY_SYNTHETIC. - Add a helper function (`dump_dentry`) to dump the dentry tree for debugging. Signed-off-by: Paweł Marczewski <[email protected]>

mkow

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

dimakuv

Reviewable status: complete! all files reviewed, all discussions resolved

dimakuv

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

boryspoplawski

Reviewed 8 of 17 files at r1, 2 of 4 files at r2, 3 of 3 files at r6, 4 of 4 files at r7.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @pwmarcz)

a discussion (no related file):
Sorry I'm late to the party but I have a couple of comments.

LibOS/shim/include/shim_fs.h, line 383 at r7 (raw file):

 * \brief Open a file under a given path, similar to Unix open
 *
 * \param[out] hdl handle to associate with dentry, can be NULL

I'm not sure if this is really out param. It's just modified/passed to another object IIUC, so I would omit it or use in/out (but I dont think it fits that case)

LibOS/shim/include/shim_fs.h, line 418 at r7 (raw file):

 * \brief Open an already retrieved dentry, and associate a handle with it
 *
 * \param[out] hdl handle to associate with dentry

ditto

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

 * The caller should hold `g_dcache_lock`.
 *
 * On success, returns 0 and sets `*new` to the found dentry (whose reference count is increased). A

*new -> *found

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

 *
 * On success, returns 0 and sets `*new` to the found dentry (whose reference count is increased). A
 * negative filesystem lookup (ENOENT) is also considered a success, and `*new` is set to a negative

ditto

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

 *
 * On failure (including lookup failing with any other error than ENOENT) returns the negative error
 * code, and sets `*new` to NULL.

ditto

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

        /* Relative part with no start dentry provided, use process current working directory */
        lock(&g_process.fs_lock);
        start = g_process.cwd;

This seems incorrect: we do not get this dentry and then unlock g_process.fs_lock. What if another thread changes cwd and this dentry is destroyed?

Maybe change this to set dent directly and just add an else clause at the end to handle start != NULL case (and just do dent = start there)?

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

        name++;

    while (*name != '\0') {

If path == "/" and !(flags & LOOKUP_DIRECTORY) this will still succeed and return root dentry.

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

                   struct shim_dentry** found) {
    lock(&g_dcache_lock);
    int ret = do_path_lookupat(start, path, flags, found, /*link_depth=*/0);

NP: _path_lookupat?

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

    if (dent->state & DENTRY_ISLINK) {
        /* Can happen if user specified O_NOFOLLOW, or O_TRUNC | O_EXCL. Posix requires us to fail

I guess we do not support O_PATH?

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

            goto err;

        /* Create directory or file, depending on O_DIRECTORY. Return -EINVAL if the operation is

man for open(2) states that When both O_CREAT and O_DIRECTORY are specified in flags and the file specified by pathname does not exist, open() will create a regular file (i.e., O_DIRECTORY is ignored). Is that not the case on newer kernel? Because if it is, we might need to emulate it...

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

            /* Ignore inaccessible files */
            if (ret == -EACCES)

How can we have access to only part of directory?

pwmarcz

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @boryspoplawski and @pwmarcz)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Sorry I'm late to the party but I have a couple of comments.

Thanks. I created a PR in #2354, and I will respond to your comments there.

pwmarcz force-pushed the pawel/dentry-cleanup-3 branch from 61f6983 to a9f372c Compare April 26, 2021 12:20

pwmarcz mentioned this pull request Apr 26, 2021

[LibOS] Fix deadlock after failed getdents64 #2334

Merged

pwmarcz changed the title ~~Rewrite path lookup functions~~ [LibOS] Rewrite path lookup functions Apr 26, 2021

pwmarcz force-pushed the pawel/dentry-cleanup-3 branch from eaab40f to 3aaef34 Compare April 26, 2021 14:07

pwmarcz commented Apr 26, 2021

View reviewed changes

dimakuv suggested changes Apr 27, 2021

View reviewed changes

pwmarcz force-pushed the pawel/dentry-cleanup-3 branch from 3aaef34 to c388f72 Compare April 27, 2021 14:34

pwmarcz commented Apr 27, 2021

View reviewed changes

dimakuv suggested changes Apr 27, 2021

View reviewed changes

pwmarcz commented Apr 27, 2021

View reviewed changes

pwmarcz force-pushed the pawel/dentry-cleanup-3 branch from 72a63c6 to 9559729 Compare April 28, 2021 07:42

pwmarcz commented Apr 28, 2021

View reviewed changes

dimakuv previously approved these changes Apr 28, 2021

View reviewed changes

pwmarcz dismissed dimakuv’s stale review via a4bf8a4 April 29, 2021 09:25

pwmarcz mentioned this pull request Apr 30, 2021

[LibOS] Don't store dentry paths #2342

Merged

dimakuv previously approved these changes May 3, 2021

View reviewed changes

mkow suggested changes May 5, 2021

View reviewed changes

pwmarcz dismissed dimakuv’s stale review via 4394506 May 5, 2021 08:20

pwmarcz commented May 5, 2021

View reviewed changes

mkow previously approved these changes May 5, 2021

View reviewed changes

mkow dismissed their stale review via 51dcf96 May 5, 2021 15:58

mkow force-pushed the pawel/dentry-cleanup-3 branch from 4394506 to 51dcf96 Compare May 5, 2021 15:58

mkow approved these changes May 5, 2021

View reviewed changes

dimakuv approved these changes May 5, 2021

View reviewed changes

mkow merged commit 51dcf96 into master May 5, 2021

mkow deleted the pawel/dentry-cleanup-3 branch May 5, 2021 18:12

boryspoplawski reviewed May 6, 2021

View reviewed changes

pwmarcz mentioned this pull request May 6, 2021

[LibOS] Update for path lookup functions #2354

Merged

pwmarcz commented May 6, 2021

View reviewed changes

pwmarcz mentioned this pull request May 14, 2021

Fix filesystem and unskip tests #2377

Merged

pwmarcz mentioned this pull request Sep 10, 2021

Filesystem refactoring gramineproject/gramine#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LibOS] Rewrite path lookup functions #2333

[LibOS] Rewrite path lookup functions #2333

pwmarcz commented Apr 26, 2021 •

edited by mkow

Loading

pwmarcz left a comment

dimakuv left a comment

pwmarcz left a comment

dimakuv left a comment

pwmarcz left a comment

pwmarcz left a comment

dimakuv left a comment

dimakuv left a comment

mkow left a comment

pwmarcz left a comment

mkow left a comment

mkow left a comment

dimakuv left a comment

dimakuv left a comment

boryspoplawski left a comment

pwmarcz left a comment

[LibOS] Rewrite path lookup functions #2333

[LibOS] Rewrite path lookup functions #2333

Conversation

pwmarcz commented Apr 26, 2021 • edited by mkow Loading

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

pwmarcz left a comment

Choose a reason for hiding this comment

dimakuv 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 left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

pwmarcz left a comment

Choose a reason for hiding this comment

pwmarcz commented Apr 26, 2021 •

edited by mkow

Loading