Re: [PATCH v4 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 15, 2014, 17:19 UTC
Message-ID: <534D6A05.8040609@alum.mit.edu>
In-Reply-To: <1397500163-7617-3-git-send-email-sahlberg@google.com>

On 04/14/2014 08:29 PM, Ronnie Sahlberg wrote:

Show 5 quoted lines

> Change delete_ref_loose()) to just flag that a ref is to be deleted but do
> not actually unlink the files.
> Change commit_ref_lock() so that it will unlink refs that are flagged for
> deletion.
> Change all callers of delete_ref_loose() to explicitely call commit_ref_lock()

s/explicitely/explicitly/

Show 5 quoted lines

> to commit the deletion.
> 
> The new pattern for deleting loose refs thus become:
> 
> lock = lock_ref_sha1_basic() (or varient of)

s/varient/variant/

> delete_ref_loose(lock)
> unlock_ref(lock) | commit_ref_lock(lock)

Formatting: sentences should be flowed together if they are within a
paragraph, or separated with blank lines if they constitute separate
paragraphs, or have "bullet" characters and be indented if they are a
bullet list.

Code should be indented to set it off from the prose.

Show 38 quoted lines

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c | 32 ++++++++++++++++++++------------
>  refs.h |  2 ++
>  2 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 646afd7..a14addb 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2484,16 +2484,9 @@ static int repack_without_ref(const char *refname)
>  
>  static int delete_ref_loose(struct ref_lock *lock, int flag)
>  {
> -	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> -		/* loose */
> -		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
> -
> -		lock->lk->filename[i] = 0;
> -		err = unlink_or_warn(lock->lk->filename);
> -		lock->lk->filename[i] = '.';
> -		if (err && errno != ENOENT)
> -			return 1;
> -	}
> +	lock->delete_ref = 1;
> +	lock->delete_flag = flag;
> +
>  	return 0;
>  }
>  
> @@ -2515,7 +2508,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>  
>  	unlink_or_warn(git_path("logs/%s", lock->ref_name));
>  	clear_loose_ref_cache(&ref_cache);
> -	unlock_ref(lock);
> +	ret |= commit_ref_lock(lock);
>  	return ret;
>  }

Before this patch, the sequence for deleting a reference was

    acquire lock on loose ref file
    delete loose ref file
    acquire lock on packed-refs file
    rewrite packed-refs file, omitting ref
    activate packed-refs file and release its lock
    release lock on loose ref file

Another process that tries to read the reference's value between steps 2 and 4 sees some old value of the reference from the packed-refs file rather than seeing either its recent value or seeing it undefined. The value that it sees can be arbitrarily old, and might even point at an object that has long-since been garbage-collected. If the packed-refs lock acquisition fails, then the old value can be left in the packed-refs file and becomes the value of the reference permanently. So this is not correct (it's a known problem).

After the patch, it is

    acquire lock on loose ref file
    acquire lock on packed-refs file
    rewrite packed-refs file, omitting ref
    activate packed-refs file and release its lock
    delete loose ref file          <-- now this happens later
    release lock on loose ref file

A pack-refs process that runs between steps 4 and 5 might be able to acquire the packed-refs file lock, see the doomed loose value of the reference, and pack it, with the end effect that the reference is not deleted after all. But this is less bad than what can happen now. Can anybody think of any new races that the new sequence would open?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/