Issues with opal session dir caused by change to opal/util/os_dirpath.c

[tjb900]

Hi!

Sorry to hassle you guys again. We are just in the process of trying to migrate to v2.x, and are hitting a condition that appears to have been introduced in 58d1b3f.

Some of our nodes have a per-job TMPDIR on Lustre, and we are getting tripped up because when calling mkdir on an existing path, where the user would not have permissions to create the directory, Lustre returns EACCES rather than EEXIST. This breaks the new form of the check in opal/util/os_dirpath.c

[user@host ompi ((58d1b3f4d0...))]$ mkdir /p5
mkdir: cannot create directory `/p5': File exists
(i.e. root filesystem behaves as you expected)

[user@host ompi ((58d1b3f4d0...))]$ mkdir /p5/000scratch
mkdir: cannot create directory `/p5/000scratch': Permission denied
(this causes a problem)

[user@host ompi ((58d1b3f4d0...))]$ mkdir /p5/000scratch/my_temp_dir
(but if we ignore it and just carry on trying to make the deeper components, it will work fine)

Without wanting to start an argument over whether Lustre is doing the right thing here, this seems like it might be a not-terribly-uncommon configuration?

Cheers,
Tim

[rhc54]

it looks like this was intended as an optimization so we didn't check for existence before doing mkdir. should be a trivial change to check eaccess, and then if we fail in the next step it won't hurt

[rhc54]

now that is interesting - the referenced change was never done in OMPI's master! It somehow went directly into the release branch. I'll have to talk to the release managers about how they want to address it.

[tjb900]

It seems to be an ancestor of master now though?

On the topic of the return code - it seems to me that if the stat() after the mkdir failure succeeds, then we should just move on regardless of mkdir's errno? Just need to keep it in case the stat fails, so we can report why we couldn't make it exist.

Happy to submit a patch later this evening.

[rhc54]

We require that patches go into master first so we can fully check them out prior to going into a release branch, so this one somehow didn't follow the normal route. Regardless, I have a fix coming shortly.

[ggouaillardet]

@rhc54 this was committed into master before we fork the v2 branches.
if you look at 58d1b3f just above the commit date, there is an information that this commit is present in

• master (as part of PR ompio: fix misc memory leaks #601)
• v2.0.x
• v2.x

i will comment on directly on #2756

[rhc54]

Odd - the copy of master I have doesn't match that code at all. In fact, it looked exactly like the code prior to your change. Did someone later revert your change?

[ggouaillardet]

4b47dae and 8fd1c3f update that.
these commit only improve the error message (e.g. they do not change the logic of 58d1b3f, mkdir() first and then stat())

[jsquyres]

@ggouaillardet @rhc54 Is this now resolved?

[ggouaillardet]

@jsquyres per the comments on the relared pr, this is now resolved