Re: FSFS7 repository corruption during commit with background upgrade

On 28 September 2014 22:28, Stefan Fuhrmann
<stefan.fuhrmann_at_wandisco.com> wrote:
> On Fri, Sep 26, 2014 at 4:38 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>
>> I was looking how fsfs upgrade code works and found particular fsfs7
>> log-addressing repository corruption bug:
>> 0. Repository has 999 revisions.
>> 1. Client begins committing some data to fsfs v6 repository through Apache
>> 2. Apache web server opens svn_fs_t, reads format file. At this point
>> repository has format 6 and uses physical (classic) addressing.
>> 3. Client changes txn content
>> 4. Before committing change, admin upgrades this repository to fsfs7. FSFS
>> upgrade code marks that log-addressing will be available from next
>> shard,
>> i.e. from revision 1000.
>> 5. Apache web server starting committing txn: obtaining write-lock and
>> writes
>> protorev for r1000. Since svn_fs_t instance was cached for connection
>> so
>> it didn't know that revision 1000 should be log addressing and writes
>> physical addressing revision without any error (!)
>
>
> Thanks for the test case. I committed it as part of r1627949.

Thanks for the fix!

> While the latter also fixes the upgrade scenario above, I think
> we should put a section into the release notes telling users that
> upgrading hot repositories is a bad idea.
>
> I've been going through the code looking at how an outdated
> svn_fs_t instance would interact with an upgraded repo and it
> seems that it usually either does not care (e.g. rep sharing) or
> errors out (global IDs, packed shards). But I don't see a way
> to guarantee that this is always the case.

I think it is almost useless to tell users do not upgrade hot
repositories if they used to do this all the time before.

I think there are the following viable options:
1) make the svnadmin upgrade work properly for hot repositories
2) do not enable log-addressing during svnadmin upgrade.

I think (1) is almost impossible to implement since Subversion
does not use locks for read operations. I'm also think
that (2) is the best option in current situation. As an
additional bonus, it give us oportunity to remove some
tricky code like txn upgrade during commit.

Btw the same approach was used in upgrade to FSFS
format 3 where sharded layout was introduced.

Anyway I think it is a serious issue and any sort of
mention in the release notes will not solve it.

-- 
Ivan Zhakov