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

Re: FSFS commit failure should release txn proto-rev lock

From: Julian Foad <julianfoad_at_apache.org>
Date: Thu, 30 Jul 2020 09:11:03 +0000 (UTC)

Thanks for the response, Daniel. I'm away and it will be a few days before I can check and respond completely.

(Off the top of my head, did sub tests 1-8 fail with "already locked" and 9 is the odd one out?)

30 Jul 2020 00:00:32 Daniel Shahaf <d.s_at_daniel.shahaf.name>:
> (I wasn't going to comment on this since I don't know this part of the
> code too well, but given the crickets…)
>
> So, the undesired behaviour is this:
>
>> This commit failed unexpectedly:
>> Traceback (most recent call last):
>> File "/home/julianfoad/tmp/svn/wd-nv-7983/test-release-proto-rev-lock-7/test-commits.py", line 59, in <module>
>> fs.commit_txn(txn2)
>> File "/home/julianfoad/build/subversion-c/subversion/bindings/swig/python/libsvn/fs.py", line 324, in svn_fs_commit_txn
>> return _fs.svn_fs_commit_txn(*args)
>> svn.core.SubversionException:
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs/fs-loader.c:885
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/tree.c:2358
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:4010
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:368
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:240
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:228
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:3813
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:475
>> 2 - Can't open file '/home/julianfoad/tmp/svn/wd-nv-7983/repo/db/txn-protorevs/1-a.rev': No such file or directory
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_subr/io.c:3931
>> + echo 'FAILED: sub-test 9'
>> FAILED: sub-test 9
>>
> First of all, you said the error message read "already locked".  The
> error above is ENOENT.  Can you clarify this?
>
> The change proposed is this:
>
> Julian Foad wrote on Wed, 10 Jun 2020 17:10 +0100:
>> @@ -3881,30 +3898,32 @@ commit_body(void *baton, apr_pool_t *poo
>> /* Move the finished rev file into place.
>>
>> ### This "breaks" the transaction by removing the protorev file
>> ### but the revision is not yet complete.  If this commit does
>> ### not complete for any reason the transaction will be lost. */
>> old_rev_filename = svn_fs_fs__path_rev_absolute(cb->fs, old_rev, pool);
>> rev_filename = svn_fs_fs__path_rev(cb->fs, new_rev, pool);
>> proto_filename = svn_fs_fs__path_txn_proto_rev(cb->fs, txn_id, pool);
>> -  SVN_ERR(svn_fs_fs__move_into_place(proto_filename, rev_filename,
>> +  ERR_PROTO_REV_LOCKED(svn_fs_fs__move_into_place(proto_filename, rev_filename,
>> old_rev_filename, ffd->flush_to_disk,
>> pool));
>> +  TEST_COMMIT_FAIL(9);
>>
>> /* Now that we've moved the prototype revision file out of the way,
>> we can unlock it (since further attempts to write to the file
>> will fail as it no longer exists).  We must do this so that we can
>> remove the transaction directory later. */
>> SVN_ERR(unlock_proto_rev(cb->fs, txn_id, proto_file_lockcookie, pool));
>> @@ -3939,12 +3958,20 @@ commit_body(void *baton, apr_pool_t *poo
>> SVN_ERR(promote_cached_directories(cb->fs, directory_ids, pool));
>>
>> /* Remove this transaction directory. */
>> SVN_ERR(svn_fs_fs__purge_txn(cb->fs, cb->txn->id, pool));
>>
>> return SVN_NO_ERROR;
>> +
>> +
>> +on_err_proto_rev_locked:
>> +  /* If any error occurred while the proto-rev file was writable (locked),
>> +   * unlock it before returning to clean up as best we can. */
>> +  err1 = svn_error_compose_create(err1,
>> +           unlock_proto_rev(cb->fs, txn_id, proto_file_lockcookie, pool));
>> +  return err1;
>> }
>>
>> Why is it correct to call unlock_proto_rev() without checking the error
> code svn_fs_fs__move_into_place() returned?
>
> HTH,
>
> Daniel
>
> P.S.  Suggest to pass the name of the local variable ERR1 as
> a parameter to the macro to avoid action at a distance.
>
Received on 2020-07-30 11:11:04 CEST

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