[PATCH 0/2] Add perf debug dir locking

[PATCH 0/2] Add perf debug dir locking

[Milos Vyletel]

Hi,

this series is follow up on my last submission where Jirka mentioned it would
be nice to have some locking around .debug dir operations.

https://lkml.org/lkml/2015/3/20/126

Following two patches add such functionality by utilizing flock. Since flock
supports both shared(read) and exclusive(write) locks both variants are being
added so that we can use multiple readers while only one writer.

Having said that any comments/suggestions on the code are welcome.

I did some basic testing. Mostly manual tool testing as well as the recreator
from above mentioned URL. I do not expect any functional change however more
testing is welcome.

Milos

Milos Vyletel (2):
  perf/tools: add read/write buildid dir locks
  perf/tools: put new buildid locks to use

 tools/perf/builtin-buildid-cache.c | 12 +++++
 tools/perf/util/build-id.c         | 97 ++++++++++++++++++++++++++++++++++----
 tools/perf/util/build-id.h         |  5 ++
 3 files changed, 106 insertions(+), 8 deletions(-)

-- 
2.4.0

[PATCH 1/2] perf/tools: add read/write buildid dir locks

[Milos Vyletel]

Protect access to buildid dir by flock to prevent race conditions. This
patch adds buildid_dir_read/write_lock/unlock functions.

Signed-off-by: Milos Vyletel <milos@redhat.com>
---
 tools/perf/util/build-id.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/build-id.h |  5 +++++
 2 files changed, 61 insertions(+)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 61867df..46c41e1 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -536,3 +536,59 @@ bool perf_session__read_build_ids(struct perf_session *session, bool with_hits)
 
 	return ret;
 }
+
+static int buildid_dir_lock(int op)
+{
+	int lockfd, wait = 30;
+	const size_t size = PATH_MAX;
+	char *lockfile = zalloc(size);
+
+	scnprintf(lockfile, size, "%s/.lock", buildid_dir);
+
+	lockfd = open(lockfile, O_RDONLY|O_CREAT, S_IRUSR);
+	if (lockfd  == -1)
+		goto out;
+
+	while (1) {
+		if (flock(lockfd, op | LOCK_NB) == 0)
+			goto out;
+		else if (wait == 0) {
+			close(lockfd);
+			lockfd = -1;
+			goto out;
+		}
+		if (--wait % 10 == 0)
+			pr_info("Waiting for %s lock to be available\n", buildid_dir);
+		usleep(100);
+	}
+
+out:
+	free(lockfile);
+	return lockfd;
+}
+
+static void buildid_dir_unlock(int fd)
+{
+	flock(fd, LOCK_UN);
+	close(fd);
+}
+
+void buildid_dir_write_lock(int *fd)
+{
+	*fd = buildid_dir_lock(LOCK_EX);
+}
+
+void buildid_dir_read_lock(int *fd)
+{
+	*fd = buildid_dir_lock(LOCK_SH);
+}
+
+void buildid_dir_write_unlock(int fd)
+{
+	buildid_dir_unlock(fd);
+}
+
+void buildid_dir_read_unlock(int fd)
+{
+	buildid_dir_unlock(fd);
+}
diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index 8501122..326cd2e 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -31,4 +31,9 @@ int build_id_cache__add_s(const char *sbuild_id,
 int build_id_cache__remove_s(const char *sbuild_id);
 void disable_buildid_cache(void);
 
+void buildid_dir_write_lock(int *);
+void buildid_dir_read_lock(int *);
+void buildid_dir_write_unlock(int);
+void buildid_dir_read_unlock(int);
+
 #endif
-- 
2.4.0

[PATCH 2/2] perf/tools: put new buildid locks to use

[Milos Vyletel]

Use new read/write locks when accesing buildid directory on places where
we may race if multiple instances are run simultaneously.

Signed-off-by: Milos Vyletel <milos@redhat.com>
---
 tools/perf/builtin-buildid-cache.c | 12 +++++++++++
 tools/perf/util/build-id.c         | 41 ++++++++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index d47a0cd..4cf0a1d 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -94,8 +94,13 @@ static int build_id_cache__kcore_existing(const char *from_dir, char *to_dir,
 	char to_subdir[PATH_MAX];
 	struct dirent *dent;
 	int ret = -1;
+	int lockfd;
 	DIR *d;
 
+	buildid_dir_read_lock(&lockfd);
+	if (lockfd == -1)
+		return -1;
+
 	d = opendir(to_dir);
 	if (!d)
 		return -1;
@@ -121,6 +126,7 @@ static int build_id_cache__kcore_existing(const char *from_dir, char *to_dir,
 	}
 
 	closedir(d);
+	buildid_dir_read_unlock(lockfd);
 
 	return ret;
 }
@@ -130,6 +136,7 @@ static int build_id_cache__add_kcore(const char *filename, bool force)
 	char dir[32], sbuildid[BUILD_ID_SIZE * 2 + 1];
 	char from_dir[PATH_MAX], to_dir[PATH_MAX];
 	char *p;
+	int lockfd;
 
 	strlcpy(from_dir, filename, sizeof(from_dir));
 
@@ -156,6 +163,10 @@ static int build_id_cache__add_kcore(const char *filename, bool force)
 	scnprintf(to_dir, sizeof(to_dir), "%s/[kernel.kcore]/%s/%s",
 		  buildid_dir, sbuildid, dir);
 
+	buildid_dir_write_lock(&lockfd);
+	if (lockfd == -1)
+		return -1;
+
 	if (mkdir_p(to_dir, 0755))
 		return -1;
 
@@ -176,6 +187,7 @@ static int build_id_cache__add_kcore(const char *filename, bool force)
 		}
 		return -1;
 	}
+	buildid_dir_write_unlock(lockfd);
 
 	pr_debug("kcore added to build-id cache directory %s\n", to_dir);
 
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 46c41e1..13d23aa 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -17,6 +17,7 @@
 #include "tool.h"
 #include "header.h"
 #include "vdso.h"
+#include <sys/file.h>
 
 
 static bool no_buildid_cache;
@@ -308,6 +309,7 @@ int build_id_cache__list_build_ids(const char *pathname,
 	DIR *dir;
 	struct dirent *d;
 	int ret = 0;
+	int lockfd = -1;
 
 	list = strlist__new(true, NULL);
 	dir_name = build_id_cache__dirname_from_path(pathname, false, false);
@@ -316,11 +318,17 @@ int build_id_cache__list_build_ids(const char *pathname,
 		goto out;
 	}
 
+	buildid_dir_read_lock(&lockfd);
+	if (lockfd == -1) {
+		ret = -errno;
+		goto out;
+	}
+
 	/* List up all dirents */
 	dir = opendir(dir_name);
 	if (!dir) {
 		ret = -errno;
-		goto out;
+		goto out_unlock;
 	}
 
 	while ((d = readdir(dir)) != NULL) {
@@ -330,6 +338,8 @@ int build_id_cache__list_build_ids(const char *pathname,
 	}
 	closedir(dir);
 
+out_unlock:
+	buildid_dir_read_unlock(lockfd);
 out:
 	free(dir_name);
 	if (ret)
@@ -347,6 +357,7 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 	char *realname = NULL, *filename = NULL, *dir_name = NULL,
 	     *linkname = zalloc(size), *targetname, *tmp;
 	int err = -1;
+	int lockfd;
 
 	if (!is_kallsyms) {
 		realname = realpath(name, NULL);
@@ -358,30 +369,34 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 	if (!dir_name)
 		goto out_free;
 
-	if (mkdir_p(dir_name, 0755))
+	buildid_dir_write_lock(&lockfd);
+	if (lockfd == -1)
 		goto out_free;
 
+	if (mkdir_p(dir_name, 0755))
+		goto out_unlock;
+
 	if (asprintf(&filename, "%s/%s", dir_name, sbuild_id) < 0) {
 		filename = NULL;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	if (access(filename, F_OK)) {
 		if (is_kallsyms) {
 			 if (copyfile("/proc/kallsyms", filename))
-				goto out_free;
+				goto out_unlock;
 		} else if (link(realname, filename) && errno != EEXIST &&
 				copyfile(name, filename))
-			goto out_free;
+			goto out_unlock;
 	}
 
 	if (!build_id__filename(sbuild_id, linkname, size))
-		goto out_free;
+		goto out_unlock;
 	tmp = strrchr(linkname, '/');
 	*tmp = '\0';
 
 	if (access(linkname, X_OK) && mkdir_p(linkname, 0755))
-		goto out_free;
+		goto out_unlock;
 
 	*tmp = '/';
 	targetname = filename + strlen(buildid_dir) - 5;
@@ -389,6 +404,8 @@ int build_id_cache__add_s(const char *sbuild_id, const char *name,
 
 	if (symlink(targetname, linkname) == 0)
 		err = 0;
+out_unlock:
+	buildid_dir_write_unlock(lockfd);
 out_free:
 	if (!is_kallsyms)
 		free(realname);
@@ -427,6 +444,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
 	char *filename = zalloc(size),
 	     *linkname = zalloc(size), *tmp;
 	int err = -1;
+	int lockfd = -1;
 
 	if (filename == NULL || linkname == NULL)
 		goto out_free;
@@ -449,10 +467,17 @@ int build_id_cache__remove_s(const char *sbuild_id)
 	tmp = strrchr(linkname, '/') + 1;
 	snprintf(tmp, size - (tmp - linkname), "%s", filename);
 
-	if (unlink(linkname))
+	buildid_dir_write_lock(&lockfd);
+	if (lockfd == -1)
 		goto out_free;
 
+	if (unlink(linkname))
+		goto out_unlock;
+
+
 	err = 0;
+out_unlock:
+	buildid_dir_write_unlock(lockfd);
 out_free:
 	free(filename);
 	free(linkname);
-- 
2.4.0

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Ingo Molnar]

* Milos Vyletel <milos@redhat.com> wrote:

> Use new read/write locks when accesing buildid directory on places where
> we may race if multiple instances are run simultaneously.

Dunno, this will create locking interaction between multiple instances 
of perf - hanging each other, etc.

And it seems unnecessary: the buildid hierarchy is already spread out. 
What kind of races might there be?

Thanks,

	Ingo

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Jiri Olsa]

On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> 
> * Milos Vyletel <milos@redhat.com> wrote:
> 
> > Use new read/write locks when accesing buildid directory on places where
> > we may race if multiple instances are run simultaneously.
> 
> Dunno, this will create locking interaction between multiple instances 
> of perf - hanging each other, etc.
> 
> And it seems unnecessary: the buildid hierarchy is already spread out. 
> What kind of races might there be?

there was just recently one fixed by commit:
  0635b0f71424 perf tools: Fix race in build_id_cache__add_s()

havent checked the final patch yet, but the idea is to
protect us from similar bugs

jirka

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Milos Vyletel]

On Thu, May 14, 2015 at 01:38:21PM +0200, Jiri Olsa wrote:

> On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> > 
> > * Milos Vyletel <milos@redhat.com> wrote:
> > 
> > > Use new read/write locks when accesing buildid directory on places where
> > > we may race if multiple instances are run simultaneously.
> > 
> > Dunno, this will create locking interaction between multiple instances 
> > of perf - hanging each other, etc.
> > 
> > And it seems unnecessary: the buildid hierarchy is already spread out. 
> > What kind of races might there be?
> 
> there was just recently one fixed by commit:
>   0635b0f71424 perf tools: Fix race in build_id_cache__add_s()
> 
> havent checked the final patch yet, but the idea is to
> protect us from similar bugs

right. on top of race with EEXIST couple more are possible (EMLINK,
ENOSPC, EDQUOT, ENOMEM... the only way to prevent them all is to lock
this kind of operations and make sure we run one at a time.

Milos

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Ingo Molnar]

* Milos Vyletel <milos@redhat.com> wrote:

> On Thu, May 14, 2015 at 01:38:21PM +0200, Jiri Olsa wrote:
> 
> > On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> > > 
> > > * Milos Vyletel <milos@redhat.com> wrote:
> > > 
> > > > Use new read/write locks when accesing buildid directory on places where
> > > > we may race if multiple instances are run simultaneously.
> > > 
> > > Dunno, this will create locking interaction between multiple instances 
> > > of perf - hanging each other, etc.
> > > 
> > > And it seems unnecessary: the buildid hierarchy is already spread out. 
> > > What kind of races might there be?
> > 
> > there was just recently one fixed by commit:
> >   0635b0f71424 perf tools: Fix race in build_id_cache__add_s()
> > 
> > havent checked the final patch yet, but the idea is to
> > protect us from similar bugs
> 
> right. on top of race with EEXIST couple more are possible (EMLINK, 
> ENOSPC, EDQUOT, ENOMEM... the only way to prevent them all is to 
> lock this kind of operations and make sure we run one at a time.

Yeah, so the race pointed out in 0635b0f71424 can be (and should be) 
fixed without locking:

 - first create the file under a process-private name under 
   ~/.debug/tmp/ if the target does not exist yet

 - then fully fill it in with content

 - then link(2) it to the public target name, which VFS operation is
   atomic and may fail safely: at which point it got already created
   by someone else.

 - finally unlink() the private instance name and the target will now
   be the only instance left: either created by us, or by some other 
   perf instance in the rare racy case.

Since all of ~/.debug is on the same filesystem this should work fine.

Beyond avoiding locking this approach has another advantage: it's 
transaction safe, so a crashed/interrupted perf instance won't corrupt 
the debug database, it will only put fully constructed files into the 
public build-id namespace. It at most leaves a stale private file 
around in ~/.debug/tmp/.

Really, we should be following the example of Git, which is using a 
similar append-mostly flow to handle data, and generally avoids file 
locking as much as possible - which is a whole new can of worms.

Thanks,

	Ingo

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Ingo Molnar]

* Milos Vyletel <milos@redhat.com> wrote:

> On Thu, May 14, 2015 at 01:38:21PM +0200, Jiri Olsa wrote:
> 
> > On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> > > 
> > > * Milos Vyletel <milos@redhat.com> wrote:
> > > 
> > > > Use new read/write locks when accesing buildid directory on places where
> > > > we may race if multiple instances are run simultaneously.
> > > 
> > > Dunno, this will create locking interaction between multiple instances 
> > > of perf - hanging each other, etc.
> > > 
> > > And it seems unnecessary: the buildid hierarchy is already spread out. 
> > > What kind of races might there be?
> > 
> > there was just recently one fixed by commit:
> >   0635b0f71424 perf tools: Fix race in build_id_cache__add_s()
> > 
> > havent checked the final patch yet, but the idea is to
> > protect us from similar bugs
> 
> right. on top of race with EEXIST couple more are possible (EMLINK, 
> ENOSPC, EDQUOT, ENOMEM... the only way to prevent them all is to 
> lock this kind of operations and make sure we run one at a time.

No, that's not the only way to avoid the race - see my previous mail.

Thanks,

	Ingo

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Milos Vyletel]

On Thu, May 14, 2015 at 07:38:08PM +0200, Ingo Molnar wrote:

> 
> * Milos Vyletel <milos@redhat.com> wrote:
> 
> > On Thu, May 14, 2015 at 01:38:21PM +0200, Jiri Olsa wrote:
> > 
> > > On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Milos Vyletel <milos@redhat.com> wrote:
> > > > 
> > > > > Use new read/write locks when accesing buildid directory on places where
> > > > > we may race if multiple instances are run simultaneously.
> > > > 
> > > > Dunno, this will create locking interaction between multiple instances 
> > > > of perf - hanging each other, etc.
> > > > 
> > > > And it seems unnecessary: the buildid hierarchy is already spread out. 
> > > > What kind of races might there be?
> > > 
> > > there was just recently one fixed by commit:
> > >   0635b0f71424 perf tools: Fix race in build_id_cache__add_s()
> > > 
> > > havent checked the final patch yet, but the idea is to
> > > protect us from similar bugs
> > 
> > right. on top of race with EEXIST couple more are possible (EMLINK, 
> > ENOSPC, EDQUOT, ENOMEM... the only way to prevent them all is to 
> > lock this kind of operations and make sure we run one at a time.
> 
> Yeah, so the race pointed out in 0635b0f71424 can be (and should be) 
> fixed without locking:
> 
>  - first create the file under a process-private name under 
>    ~/.debug/tmp/ if the target does not exist yet
> 
>  - then fully fill it in with content
> 
>  - then link(2) it to the public target name, which VFS operation is
>    atomic and may fail safely: at which point it got already created
>    by someone else.
> 
>  - finally unlink() the private instance name and the target will now
>    be the only instance left: either created by us, or by some other 
>    perf instance in the rare racy case.
> 
> Since all of ~/.debug is on the same filesystem this should work fine.
> 
> Beyond avoiding locking this approach has another advantage: it's 
> transaction safe, so a crashed/interrupted perf instance won't corrupt 
> the debug database, it will only put fully constructed files into the 
> public build-id namespace. It at most leaves a stale private file 
> around in ~/.debug/tmp/.
> 
> Really, we should be following the example of Git, which is using a 
> similar append-mostly flow to handle data, and generally avoids file 
> locking as much as possible - which is a whole new can of worms.
> 

Thanks Ingo. I'll take a look at this later this week.

Milos

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Milos Vyletel]

On Thu, May 14, 2015 at 07:38:08PM +0200, Ingo Molnar wrote:

> 
> * Milos Vyletel <milos@redhat.com> wrote:
> 
> > On Thu, May 14, 2015 at 01:38:21PM +0200, Jiri Olsa wrote:
> > 
> > > On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Milos Vyletel <milos@redhat.com> wrote:
> > > > 
> > > > > Use new read/write locks when accesing buildid directory on places where
> > > > > we may race if multiple instances are run simultaneously.
> > > > 
> > > > Dunno, this will create locking interaction between multiple instances 
> > > > of perf - hanging each other, etc.
> > > > 
> > > > And it seems unnecessary: the buildid hierarchy is already spread out. 
> > > > What kind of races might there be?
> > > 
> > > there was just recently one fixed by commit:
> > >   0635b0f71424 perf tools: Fix race in build_id_cache__add_s()
> > > 
> > > havent checked the final patch yet, but the idea is to
> > > protect us from similar bugs
> > 
> > right. on top of race with EEXIST couple more are possible (EMLINK, 
> > ENOSPC, EDQUOT, ENOMEM... the only way to prevent them all is to 
> > lock this kind of operations and make sure we run one at a time.
> 
> Yeah, so the race pointed out in 0635b0f71424 can be (and should be) 
> fixed without locking:
> 
>  - first create the file under a process-private name under 
>    ~/.debug/tmp/ if the target does not exist yet
> 
>  - then fully fill it in with content
> 
>  - then link(2) it to the public target name, which VFS operation is
>    atomic and may fail safely: at which point it got already created
>    by someone else.
> 
>  - finally unlink() the private instance name and the target will now
>    be the only instance left: either created by us, or by some other 
>    perf instance in the rare racy case.
> 
> Since all of ~/.debug is on the same filesystem this should work fine.
> 
> Beyond avoiding locking this approach has another advantage: it's 
> transaction safe, so a crashed/interrupted perf instance won't corrupt 
> the debug database, it will only put fully constructed files into the 
> public build-id namespace. It at most leaves a stale private file 
> around in ~/.debug/tmp/.
> 

Ingo,

I finally found some time to make this change. While going over the code
I've noticed one thing that would make concurrent creation even easier
to solve. Instead of copying the file to temp file what about simply
opening file with O_CREAT|O_EXCL? creat itself

"creat() is equivalent to open() with flags equal to O_CREAT|O_WRONLY|O_TRUNC."

addition of O_EXCL would

"Ensure that this call creates the file: if this flag is specified in
conjunction with O_CREAT, and pathname already exists, then open() will fail."

This we would prevent truncation of already linked file in case link()
races as in 0635b0f71424. What do you think?

Milos

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Ingo Molnar]

* Milos Vyletel <milos@redhat.com> wrote:

> On Thu, May 14, 2015 at 07:38:08PM +0200, Ingo Molnar wrote:
> 
> > 
> > * Milos Vyletel <milos@redhat.com> wrote:
> > 
> > > On Thu, May 14, 2015 at 01:38:21PM +0200, Jiri Olsa wrote:
> > > 
> > > > On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> > > > > 
> > > > > * Milos Vyletel <milos@redhat.com> wrote:
> > > > > 
> > > > > > Use new read/write locks when accesing buildid directory on places where
> > > > > > we may race if multiple instances are run simultaneously.
> > > > > 
> > > > > Dunno, this will create locking interaction between multiple instances 
> > > > > of perf - hanging each other, etc.
> > > > > 
> > > > > And it seems unnecessary: the buildid hierarchy is already spread out. 
> > > > > What kind of races might there be?
> > > > 
> > > > there was just recently one fixed by commit:
> > > >   0635b0f71424 perf tools: Fix race in build_id_cache__add_s()
> > > > 
> > > > havent checked the final patch yet, but the idea is to
> > > > protect us from similar bugs
> > > 
> > > right. on top of race with EEXIST couple more are possible (EMLINK, 
> > > ENOSPC, EDQUOT, ENOMEM... the only way to prevent them all is to 
> > > lock this kind of operations and make sure we run one at a time.
> > 
> > Yeah, so the race pointed out in 0635b0f71424 can be (and should be) 
> > fixed without locking:
> > 
> >  - first create the file under a process-private name under 
> >    ~/.debug/tmp/ if the target does not exist yet
> > 
> >  - then fully fill it in with content
> > 
> >  - then link(2) it to the public target name, which VFS operation is
> >    atomic and may fail safely: at which point it got already created
> >    by someone else.
> > 
> >  - finally unlink() the private instance name and the target will now
> >    be the only instance left: either created by us, or by some other 
> >    perf instance in the rare racy case.
> > 
> > Since all of ~/.debug is on the same filesystem this should work fine.
> > 
> > Beyond avoiding locking this approach has another advantage: it's 
> > transaction safe, so a crashed/interrupted perf instance won't corrupt 
> > the debug database, it will only put fully constructed files into the 
> > public build-id namespace. It at most leaves a stale private file 
> > around in ~/.debug/tmp/.
> > 
> 
> Ingo,
> 
> I finally found some time to make this change. While going over the code I've 
> noticed one thing that would make concurrent creation even easier to solve. 
> Instead of copying the file to temp file what about simply opening file with 
> O_CREAT|O_EXCL? creat itself
> 
> "creat() is equivalent to open() with flags equal to O_CREAT|O_WRONLY|O_TRUNC."
> 
> addition of O_EXCL would
> 
> "Ensure that this call creates the file: if this flag is specified in 
> conjunction with O_CREAT, and pathname already exists, then open() will fail."
> 
> This we would prevent truncation of already linked file in case link() races as 
> in 0635b0f71424. What do you think?

But it would not prevent the problem of creating a not yet fully constructed file 
- which some other tool invocation could attempt to parse in an incomplete 
fashion.

Using create+link+unlink avoids that race, the files in the publicly visible 
namespace will always be fully constructed by the time they are made visible 
(atomically).

Thanks,

	Ingo

Re: [PATCH 2/2] perf/tools: put new buildid locks to use

[Milos Vyletel]

On Wed, Jun 03, 2015 at 01:21:41PM +0200, Ingo Molnar wrote:

> 
> * Milos Vyletel <milos@redhat.com> wrote:
> 
> > On Thu, May 14, 2015 at 07:38:08PM +0200, Ingo Molnar wrote:
> > 
> > > 
> > > * Milos Vyletel <milos@redhat.com> wrote:
> > > 
> > > > On Thu, May 14, 2015 at 01:38:21PM +0200, Jiri Olsa wrote:
> > > > 
> > > > > On Thu, May 14, 2015 at 12:40:59PM +0200, Ingo Molnar wrote:
> > > > > > 
> > > > > > * Milos Vyletel <milos@redhat.com> wrote:
> > > > > > 
> > > > > > > Use new read/write locks when accesing buildid directory on places where
> > > > > > > we may race if multiple instances are run simultaneously.
> > > > > > 
> > > > > > Dunno, this will create locking interaction between multiple instances 
> > > > > > of perf - hanging each other, etc.
> > > > > > 
> > > > > > And it seems unnecessary: the buildid hierarchy is already spread out. 
> > > > > > What kind of races might there be?
> > > > > 
> > > > > there was just recently one fixed by commit:
> > > > >   0635b0f71424 perf tools: Fix race in build_id_cache__add_s()
> > > > > 
> > > > > havent checked the final patch yet, but the idea is to
> > > > > protect us from similar bugs
> > > > 
> > > > right. on top of race with EEXIST couple more are possible (EMLINK, 
> > > > ENOSPC, EDQUOT, ENOMEM... the only way to prevent them all is to 
> > > > lock this kind of operations and make sure we run one at a time.
> > > 
> > > Yeah, so the race pointed out in 0635b0f71424 can be (and should be) 
> > > fixed without locking:
> > > 
> > >  - first create the file under a process-private name under 
> > >    ~/.debug/tmp/ if the target does not exist yet
> > > 
> > >  - then fully fill it in with content
> > > 
> > >  - then link(2) it to the public target name, which VFS operation is
> > >    atomic and may fail safely: at which point it got already created
> > >    by someone else.
> > > 
> > >  - finally unlink() the private instance name and the target will now
> > >    be the only instance left: either created by us, or by some other 
> > >    perf instance in the rare racy case.
> > > 
> > > Since all of ~/.debug is on the same filesystem this should work fine.
> > > 
> > > Beyond avoiding locking this approach has another advantage: it's 
> > > transaction safe, so a crashed/interrupted perf instance won't corrupt 
> > > the debug database, it will only put fully constructed files into the 
> > > public build-id namespace. It at most leaves a stale private file 
> > > around in ~/.debug/tmp/.
> > > 
> > 
> > Ingo,
> > 
> > I finally found some time to make this change. While going over the code I've 
> > noticed one thing that would make concurrent creation even easier to solve. 
> > Instead of copying the file to temp file what about simply opening file with 
> > O_CREAT|O_EXCL? creat itself
> > 
> > "creat() is equivalent to open() with flags equal to O_CREAT|O_WRONLY|O_TRUNC."
> > 
> > addition of O_EXCL would
> > 
> > "Ensure that this call creates the file: if this flag is specified in 
> > conjunction with O_CREAT, and pathname already exists, then open() will fail."
> > 
> > This we would prevent truncation of already linked file in case link() races as 
> > in 0635b0f71424. What do you think?
> 
> But it would not prevent the problem of creating a not yet fully constructed file 
> - which some other tool invocation could attempt to parse in an incomplete 
> fashion.
> 
> Using create+link+unlink avoids that race, the files in the publicly visible 
> namespace will always be fully constructed by the time they are made visible 
> (atomically).
> 

Got it. Will use the approach proposed by you.

Milos