Re: [PATCH/RFC v3 01/13] Move index v2 specific functions to their own file

Junio C Hamano [off-list ref] writes:

If you found that an entry you read halfway has an inconsistent crc,
and if you suspect that is because somebody else was writing to the
same index, it is a _sure_ sign that you are not alone, and all the
entries you read so far to the core, even if they weren't touched by
that sombody else when you read them, may be stale, and worse yet,
what you are going to read may be inconsistent with what you've read
and have in-core (e.g. you may have read "f" before somebody else
that is racing with you have turned it into a directory, and your
next read may find "f/d" in the index without crc error).

One sane way to avoid reading such an inconsistent state may be to
redo this whole function, starting from the part that calls mmap().
IOW,

	do {
		fd = open()
		mmap = xmmap(fd);
		close(fd);
                verify_various_fields(mmap);
                status = istate->ops->read_index(istate, mmap, mmap_size));
	} while (status == READ_AGAIN);

I do not think the "pass fd around so that we can redo the mapping
deep inside the callchain" is either a good idea or necessary.

By the way, you can only detect such inconsistency when you are
lucky enough that you catch the other person in the middle of
writing.

If the index you are looking at holds a large tree with very many
paths, it is possible that there are two large directories, and
after you read all entries from one, the other process starts
modifying the paths in that directory, without you ever finding it
out.  If the goal of the topic is to make the index work better in
projects with large trees, it may be wise to think about locking the
whole thing, so that you do not have to rely on the per-entry crc
and you being lucky to detect such a race.  The per-entry crc, as
far as I understand, may have been introduced primarily to detect
on-disk data corruption; it is not a suitable mechanism to detect
conflicting accesses.

On Fri, Aug 10, 2012 at 7:13 AM, Junio C Hamano [off-list ref] wrote:

By the way, you can only detect such inconsistency when you are
lucky enough that you catch the other person in the middle of
writing.

If the index you are looking at holds a large tree with very many
paths, it is possible that there are two large directories, and
after you read all entries from one, the other process starts
modifying the paths in that directory, without you ever finding it
out.  If the goal of the topic is to make the index work better in
projects with large trees, it may be wise to think about locking the
whole thing, so that you do not have to rely on the per-entry crc
and you being lucky to detect such a race.  The per-entry crc, as
far as I understand, may have been introduced primarily to detect
on-disk data corruption; it is not a suitable mechanism to detect
conflicting accesses.

So we acquire the lock just before we need to write, or at the time we
open for reading?
-- 
Duy