All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Karl Moskowski <kmoskowski@me.com>,
	Jeff King <peff@peff.net>, Mike Hommey <mh@glandium.org>,
	David Turner <dturner@twopensource.com>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: [PATCH 03/20] raceproof_create_file(): new function
Date: Tue, 16 Feb 2016 14:22:16 +0100	[thread overview]
Message-ID: <338201e98a16f6c53ed1ee447de10c206f2acc33.1455626201.git.mhagger@alum.mit.edu> (raw)
In-Reply-To: <cover.1455626201.git.mhagger@alum.mit.edu>

Add a function that tries to create a file and any containing
directories in a way that is robust against races with other processes
that might be cleaning up empty directories at the same time.

The actual file creation is done by a callback function, which, if it
fails, should set errno to EISDIR or ENOENT according to the convention
of open(). raceproof_create_file() detects such failures, and
respectively either tries to delete empty directories that might be in
the way of the file or tries to create the containing directories. Then
it retries the callback function.

This function is not yet used.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
The actual file creation is done by a callback function, so I think
this function is flexible enough to be applicable in other
circumstances where similar races might occur. Perhaps when creating
loose object files in the ODB?

I was thinking about moving this function, along with
safe_create_leading_directories() and
safe_create_leading_directories_const(), to a more general module like
path.c. But it didn't seem worth the code churn.

 cache.h     | 16 ++++++++++++++
 sha1_file.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/cache.h b/cache.h
index 7d3f80c..6e53cc8 100644
--- a/cache.h
+++ b/cache.h
@@ -976,6 +976,22 @@ enum scld_error {
 enum scld_error safe_create_leading_directories(char *path);
 enum scld_error safe_create_leading_directories_const(const char *path);
 
+typedef int create_file_fn(const char *path, void *cb);
+
+/*
+ * Create a file at path using fn, creating leading directories if
+ * necessary. If fn fails with errno==ENOENT, then try to create the
+ * containing directory and call fn again. If fn fails with
+ * errno==EISDIR, then delete the directory that is in the way if it
+ * is empty and call fn again. Retry a few times in case we are racing
+ * with another process that is trying to clean up the directory
+ * that contains path.
+ *
+ * In any case, the return value of this function and the errno that
+ * it sets are those resulting from the last call of fn.
+ */
+int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
+
 int mkdir_in_gitdir(const char *path);
 extern char *expand_user_path(const char *path);
 const char *enter_repo(const char *path, int strict);
diff --git a/sha1_file.c b/sha1_file.c
index a1ac646..31dcfe8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -177,6 +177,75 @@ enum scld_error safe_create_leading_directories_const(const char *path)
 	return result;
 }
 
+int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
+{
+	/*
+	 * The number of times we will try to remove empty directories
+	 * in the way of path. This is only 1 because if another
+	 * process is racily creating directories that conflict with
+	 * us, we don't want to fight against them.
+	 */
+	int remove_directories_remaining = 1;
+
+	/*
+	 * The number of times that we will try to create the
+	 * directories containing path. We are willing to attempt this
+	 * more than once, because another process could be trying to
+	 * clean up empty directories at the same time as we are
+	 * trying to create them.
+	 */
+	int create_directories_remaining = 3;
+
+	/* A scratch copy of path, filled lazily if we need it: */
+	struct strbuf path_copy = STRBUF_INIT;
+
+	int save_errno;
+	int ret;
+
+retry_fn:
+	ret = fn(path, cb);
+	save_errno = errno;
+	if (!ret)
+		goto out;
+
+	if (errno == EISDIR && remove_directories_remaining > 0) {
+		/*
+		 * A directory is in the way. Maybe it is empty; try
+		 * to remove it:
+		 */
+		if (!path_copy.len)
+			strbuf_addstr(&path_copy, path);
+
+		if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY)) {
+			remove_directories_remaining--;
+			goto retry_fn;
+		}
+	} else if (errno == ENOENT && create_directories_remaining > 0) {
+		/*
+		 * Maybe the containing directory didn't exist, or
+		 * maybe it was just deleted by a process that is
+		 * racing with us to clean up empty directories. Try
+		 * to create it:
+		 */
+		enum scld_error scld_result;
+
+		if (!path_copy.len)
+			strbuf_addstr(&path_copy, path);
+
+		do {
+			create_directories_remaining--;
+			scld_result = safe_create_leading_directories(path_copy.buf);
+			if (scld_result == SCLD_OK)
+				goto retry_fn;
+		} while (scld_result == SCLD_VANISHED && create_directories_remaining > 0);
+	}
+
+out:
+	strbuf_release(&path_copy);
+	errno = save_errno;
+	return ret;
+}
+
 static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
 {
 	int i;
-- 
2.7.0

  parent reply	other threads:[~2016-02-16 13:30 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-16 13:22 [PATCH 00/20] Delete directories left empty after ref deletion Michael Haggerty
2016-02-16 13:22 ` [PATCH 01/20] safe_create_leading_directories_const(): preserve errno Michael Haggerty
2016-02-16 23:45   ` Jeff King
2016-02-16 13:22 ` [PATCH 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS Michael Haggerty
2016-02-17 19:23   ` Junio C Hamano
2016-02-18 15:33     ` Michael Haggerty
2016-02-16 13:22 ` Michael Haggerty [this message]
2016-02-17 19:38   ` [PATCH 03/20] raceproof_create_file(): new function Junio C Hamano
2016-02-16 13:22 ` [PATCH 04/20] lock_ref_sha1_basic(): use raceproof_create_file() Michael Haggerty
2016-02-17 20:44   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 05/20] rename_tmp_log(): " Michael Haggerty
2016-02-17 20:53   ` Junio C Hamano
2016-02-19 16:07     ` Michael Haggerty
2016-02-19 17:15       ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 06/20] rename_tmp_log(): improve error reporting Michael Haggerty
2016-02-18 22:14   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 07/20] log_ref_setup(): separate code for create vs non-create Michael Haggerty
2016-02-16 13:22 ` [PATCH 08/20] log_ref_setup(): improve robustness against races Michael Haggerty
2016-02-18 22:17   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 09/20] log_ref_setup(): pass the open file descriptor back to the caller Michael Haggerty
2016-02-18 22:21   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 10/20] log_ref_write_1(): don't depend on logfile Michael Haggerty
2016-02-16 13:22 ` [PATCH 11/20] log_ref_setup(): manage the name of the reflog file internally Michael Haggerty
2016-02-16 13:22 ` [PATCH 12/20] log_ref_write_1(): inline function Michael Haggerty
2016-02-18 22:23   ` Junio C Hamano
2016-02-16 13:22 ` [PATCH 13/20] try_remove_empty_parents(): rename parameter "name" -> "refname" Michael Haggerty
2016-02-16 13:22 ` [PATCH 14/20] try_remove_empty_parents(): don't trash argument contents Michael Haggerty
2016-02-16 13:22 ` [PATCH 15/20] try_remove_empty_parents(): don't accommodate consecutive slashes Michael Haggerty
2016-02-16 13:22 ` [PATCH 16/20] t5505: use "for-each-ref" to test for the non-existence of references Michael Haggerty
2016-02-16 13:22 ` [PATCH 17/20] delete_ref_loose(): derive loose reference path from lock Michael Haggerty
2016-02-16 13:22 ` [PATCH 18/20] delete_ref_loose(): inline function Michael Haggerty
2016-02-16 13:22 ` [PATCH 19/20] try_remove_empty_parents(): teach to remove parents of reflogs, too Michael Haggerty
2016-02-16 13:22 ` [PATCH 20/20] ref_transaction_commit(): clean up empty directories Michael Haggerty
2016-02-17  0:08 ` [PATCH 00/20] Delete directories left empty after ref deletion Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=338201e98a16f6c53ed1ee447de10c206f2acc33.1455626201.git.mhagger@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=dturner@twopensource.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kmoskowski@me.com \
    --cc=mh@glandium.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.