hpc / mpifileutils

File utilities designed for scalability and performance.
https://hpc.github.io/mpifileutils
BSD 3-Clause "New" or "Revised" License
168 stars 66 forks source link

daos: remove prefix from documentation and update call to duns_resolve_path #487

Closed dsikich closed 2 years ago

dsikich commented 3 years ago

This just removes the daos-prefix option from the documentation because it is no longer necessary after a recent DAOS PR that allows passing a subset of a UNS path to duns_resolve_path. The option is removed from the documentation but not removed from the code in order to preserve backwards compatibility with the DAOS tests for the datamover.

The call to duns_resolve_path was updated in the case that the last directory or file in the UNS path does not yet exist, since it doesn't recurse the rest of the paths in the directory tree in that case.

Signed-off-by: Danielle Sikich danielle.sikich@intel.com

daltonbohning commented 3 years ago

@dsikich Doing some testing of the new duns_resolve_path updates.... I'm seeing a hole in the way we are using duns_resolve_path. A problem is that this path: /path/to/uns1/new_dir Does NOT resolve, because new_dir does not yet exist. duns_resolve_path doesn't bother recursing up the path if it doesn't exist. I'm thinking we'll probably want to call duns_resolve_path on the dirname(path). And if that resolves, then concat basename(path) to the resolved path. E.g.

tmp_path1 = strdup(path)
path_dirname = dirname(tmp_path1)
rc = duns_resolve_path(path_dirname)
if rc == 0
    tmp_path2 = strdup(path)
    path_basename = basename(tmp_path2)
    concat attr.da_rel_path / path_basename

The strdup usage for dirname and basename is necessary because those function implementation are wonky.

So maybe we should do something like this before removing daos-prefix from the documentation?

dsikich commented 3 years ago

@dsikich Doing some testing of the new duns_resolve_path updates.... I'm seeing a hole in the way we are using duns_resolve_path. A problem is that this path: /path/to/uns1/new_dir Does NOT resolve, because new_dir does not yet exist. duns_resolve_path doesn't bother recursing up the path if it doesn't exist. I'm thinking we'll probably want to call duns_resolve_path on the dirname(path). And if that resolves, then concat basename(path) to the resolved path. E.g.

tmp_path1 = strdup(path)
path_dirname = dirname(tmp_path1)
rc = duns_resolve_path(path_dirname)
if rc == 0
    tmp_path2 = strdup(path)
    path_basename = basename(tmp_path2)
    concat attr.da_rel_path / path_basename

The strdup usage for dirname and basename is necessary because those function implementation are wonky.

So maybe we should do something like this before removing daos-prefix from the documentation?

@daltonbohning I thought that duns_resolve_path would succeed for something like /path/to/uns1/new_dir, because I thought it checked each level and if one succeeds then returns true. Since you tested it and this happens for directories that don't exist I can update. Did you try this for regular files?

daltonbohning commented 3 years ago

@daltonbohning I thought that duns_resolve_path would succeed for something like /path/to/uns1/new_dir, because I thought it checked each level and if one succeeds then returns true. Since you tested it and this happens for directories that don't exist I can update. Did you try this for regular files?

If the path given doesn't exist at all, then it doesn't check the higher levels (parents). I.e. if stat fails, it doesn't check if the parent is a UNS entry: https://github.com/daos-stack/daos/blob/861c98b25df27c3676f1367903da9eb3aacaae90/src/client/dfs/duns.c#L419-L425

It seems the behavior is the same for regular files: if it doesn't already already exist then resolve fails, but if it does exist, then resolve works as expected. But I don't think our logic needs to explicitly handle directories vs files. I think it can just blindly use dirname and then append basename to the rel_path

dsikich commented 3 years ago

@daltonbohning I thought that duns_resolve_path would succeed for something like /path/to/uns1/new_dir, because I thought it checked each level and if one succeeds then returns true. Since you tested it and this happens for directories that don't exist I can update. Did you try this for regular files?

If the path given doesn't exist at all, then it doesn't check the higher levels (parents). I.e. if stat fails, it doesn't check if the parent is a UNS entry: https://github.com/daos-stack/daos/blob/861c98b25df27c3676f1367903da9eb3aacaae90/src/client/dfs/duns.c#L419-L425

It seems the behavior is the same for regular files: if it doesn't already already exist then resolve fails, but if it does exist, then resolve works as expected. But I don't think our logic needs to explicitly handle directories vs files. I think it can just blindly use dirname and then append basename to the rel_path

@daltonbohning I see, yeah passing dirname to duns_resolve_path then appending should work. I will update this today.

dsikich commented 3 years ago

@daltonbohning the calling of duns_resolve_path has been updated in the case that the last file/dir does not exist

dsikich commented 2 years ago

@daltonbohning the calling of duns_resolve_path has been updated in the case that the last file/dir does not exist

@daltonbohning yeah I agree. I need to update mpifileutils RPM after this is merged. Then I plan to update fs copy but I guess I will just add the test for fs copy when I submit the PR for that if the RPM isn't merged yet. If it is merged I can add them both to the same PR.

dsikich commented 2 years ago

@adammoody does this look ok to you?

adammoody commented 2 years ago

Looks good to me. Thanks @dsikich and @daltonbohning . Happy Thanksgiving to you both!

dsikich commented 2 years ago

Looks good to me. Thanks @dsikich and @daltonbohning . Happy Thanksgiving to you both!

@adammoody thanks you too!

daltonbohning commented 2 years ago

Looks good to me. Thanks @dsikich and @daltonbohning . Happy Thanksgiving to you both!

You as well!