[PATCH] Change svn_fs_root_t to include pointer to related txn

Hi all,

As you may have noticed, John Szakmeister and I have ve been looking at
some FSFS corruption problems recently. The main problem seems to be
caused by FSFS repeatedly opening and closing the proto-revision file
(the file that will becomes the immutable revision file) during the
lifetime of a transaction, and the interaction between that and APR's
file buffering in the face of transient disk I/O errors.

So the way I'd like to avoid this is to hold the file open for the
duration of the transaction (which isn't a bad idea anyway); the
most obvious way to achieve that is to stash the apr_file_t in the
FSAP-specific private data part of the svn_fs_txn_t structure.

However (and this is the purpose of this note), I'm going to need
to access the file during calls to the FSFS implementations of
svn_fs_apply_text() and svn_fs_apply_textdelta(), but these functions
only take a pointer to a transaction root (an svn_fs_root_t object),
not to the transaction itself.

So I'd need some way to navigate from the transaction root to the
transaction. Although svn_fs_root_t contains the name of the transaction,
if I'm going to use the private-data part of svn_fs_txn_t, I'll need
the object itself.

What I'd like to do is to change svn_fs_root_t so that it references
the svn_fs_txn_t directly, rather than just the transaction name.
(Revision roots would continue to refer just to the filesystem, as
present, of course). The attached patch shows the meat of the change
(it won't compile, because it doesn't fix up all the places in BDB and
FSFS that refer to the existing txn member: these need to change from
using 'root->txn' to using 'root->txn->id')

Our compatibility rules allow us to do this: svn_fs_root_t is an opaque
structure. What I'm more concerned about is whether this is a good idea
(it causes quite a few small changes throughout both the BDB and FSFS
implementations), and whether I'd be breaking any assumptions that might
have been made.

The only thing I can think of is pool lifetimes: I'd be adding an
assumption that the transaction root doesn't outlive the transaction.
However, I don't think that's an unreasonable requirement in practice:
it doesn't make sense to be doing operations on a transaction root when
the transaction isn't 'active', does it?

Comments? Any other reasons I shouldn't make this change?

[For reference, here's the diffstat of what I'm proposing to commit.
Everything other than the attached is a mechanical 'txn' to 'txn->id'
change:

 libsvn_fs/fs-loader.c | 2 +-
 libsvn_fs/fs-loader.h | 4 ++--
 libsvn_fs_base/tree.c | 44 ++++++++++++++++++++++----------------------
 libsvn_fs_fs/tree.c | 41 +++++++++++++++++++++--------------------
 4 files changed, 46 insertions(+), 45 deletions(-)
]

Regards,
Malcolm

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org