[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: verify_as_revision_before_current_plus_plus() on a production repo?

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 31 Jan 2017 02:19:18 +0000

Julian Foad wrote on Mon, Jan 30, 2017 at 21:40:00 +0000:
> Please could anybody cast a second pair of eyes over this code and say
> whether it looks safe to enable in production. It looks low risk to me, but
> it is a little "tricky": in particular, it opens a second FS instance and
> fakes the "youngest revision seen" field and then relies on this FS instance
> never reading the "current" file

Right: if some code refreshes youngest_rev_cache (by open()ing
'current', reading a value from it, and setting youngest_rev_cache to
that value), that will cause root->rev to be "newer than youngest".

I think the 'verify' code already has to deal with this possibility,
since 'recover' can backdate 'current' in the following situation:
.
    1. svn_fs.h consumer opens an svn_fs_root_t for r42
    2. invisible monkeys delete db/revs/0/42 and db/current
    3. admin runs 'svnadmin recover', which regenerates 'current' as 41
    4. svn_fs.h consumer calls verify() on the root it had opened earlier

Moreover, the 'verify' code is inherently the place where violated
invariants are least likely to cause trouble, and it's read-only.
Therefore, while a bug might cause a false positive verification error
that rejects a commit, I don't see any worse outcome. (If there's
a failure mode here that I overlooked, it's most likely to be in the f7
code, since I haven't worked much with those parts of fsfs.)

All that said, I agree that checking after the _verify_root() call that
root->rev and youngest_rev_cache haven't changed would be an improvement.

(Historical note: I think the function was written on the assumption
that youngest_rev_cache is advanced by assigning MAX(youngest_rev_cache,
value_read_from_disk) to it… but that assumption, regardless of whether
it was true when the function was written, is not true today; nowadays
the code just assigns "youngest_rev_cache = value_read_from_disk"
unconditionally.)

> nor using anything that would have been done post-commit.

That's a good point: the svn_fs_fs__verify_root() must not add any
permanent references to the revision it thinks is youngest — e.g., it
must not add reps it traverses to rep-cache.db. That's true today but
not necessarily true forever.

>

I also wonder if having verify_as_revision_before_current_plus_plus()
run in a child process would gain anything.

> verify_as_revision_before_current_plus_plus() is currently compiled in to
> debug builds but not to release builds. We can say therefore it gets
> reasonable coverage in test suite runs but has had little or no real-world
> testing.

Indeed. How about enabling that function in the alpha1 release so we
can get some more feedback about it?

(CC'ing stsp)

Cheers,

Daniel
Received on 2017-01-31 03:23:16 CET

This is an archived mail posted to the Subversion Dev mailing list.