"David Glasser" <glasser_at_davidglasser.net> writes:
> Or, um, no SVN at all, since they're file-local. Unless we are very
> worried about some system header defining them.
Well yes, that was the second half of my recommendation :-).
-K
> On Feb 19, 2008 3:00 PM, Karl Fogel <kfogel_at_red-bean.com> wrote:
>> Heck, why are these macros even "SVN_"? They should be "SVN__", or
>> else no prefix at all, since tihs is a private namespace...
>>
>> -Karl
>>
>>
>> Eric Gillespie <epg_at_pretzelnet.org> writes:
>> > I realized having ESTALE in the names of these macros no longer
>> > made sense, so I renamed them. I also expanded the comments. I
>> > suspect ESTALE is defined on all platforms svn would run on, but
>> > I'm not sure. So, use of the macros is still dependent on
>> > whether ESTALE is defined; see comments. I'll commit this
>> > tomorrow if no one objects.
>> >
>> > [[[
>> > * subversion/libsvn_fs_fs/fs_fs.c
>> > (SVN_RECOVERABLE_RETRY_COUNT): Rename from SVN_ESTALE_RETRY_COUNT.
>> > (SVN_RETRY_RECOVERABLE): Rename from SVN_RETRY_ESTALE; also retry
>> > on NOENT and EIO; and expand comments.
>> > (SVN_IGNORE_RECOVERABLE): Rename from SVN_IGNORE_ESTALE and also ignore EIO.
>> > (read_current, svn_fs_fs__revision_proplist,
>> > get_and_increment_txn_key_body): Pass new filehandle argument to
>> > SVN_RETYRY_RECOVERABLE.
>> > ]]]
>> >
>> > Index: subversion/libsvn_fs_fs/fs_fs.c
>> > ===================================================================
>> > --- subversion/libsvn_fs_fs/fs_fs.c (revision 29393)
>> > +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
>> > @@ -1069,8 +1069,19 @@
>> > }
>> >
>> >
>> > -/* SVN_ERR-like macros for dealing with ESTALE
>> > +/* SVN_ERR-like macros for dealing with recoverable errors on mutable files
>> > *
>> > + * Revprops, current, and txn-current files are mutable; that is, they
>> > + * change as part of normal fsfs operation, in constrat to revs files, or
>> > + * the format file, which are written once at create (or upgrade) time.
>> > + * When more than one host writes to the same repository, we will
>> > + * sometimes see these recoverable errors when accesssing these files.
>> > + *
>> > + * These errors all relate to NFS, and thus we only use this retry code if
>> > + * ESTALE is defined.
>> > + *
>> > + ** ESTALE
>> > + *
>> > * In NFS v3 and under, the server doesn't track opened files. If you
>> > * unlink(2) or rename(2) a file held open by another process *on the
>> > * same host*, that host's kernel typically renames the file to
>> > @@ -1080,49 +1091,60 @@
>> > * For obvious reasons, this does not work *across hosts*. No one
>> > * knows about the opened file; not the server, and not the deleting
>> > * client. So the file vanishes, and the reader gets stale NFS file
>> > - * handle. We have this problem with revprops files, current, and
>> > - * txn-current.
>> > + * handle.
>> > *
>> > - * Wrap opens and reads of such files with SVN_RETRY_ESTALE and closes
>> > - * with SVN_IGNORE_ESTALE. Call these macros within a loop of
>> > - * SVN_ESTALE_RETRY_COUNT iterations (though, realistically, the
>> > - * second try will succeed). Make sure you put a break statement
>> > - * after the close, at the end of your loop. Immediately after your
>> > - * loop, return err if err.
>> > + ** EIO, ENOENT
>> > *
>> > - * You must initialize err to SVN_NO_ERROR, as these macros do not.
>> > + * Some client implementations (at least the 2.6.18.5 kernel that ships
>> > + * with Ubuntu Dapper) sometimes give spurious ENOENT (only on open) or
>> > + * even EIO errors when trying to read these files that have been renamed
>> > + * over on some other host.
>> > + *
>> > + ** Solution
>> > + *
>> > + * Wrap opens and reads of such files with SVN_RETRY_RECOVERABLE and
>> > + * closes with SVN_IGNORE_RECOVERABLE. Call these macros within a loop of
>> > + * SVN_RECOVERABLE_RETRY_COUNT iterations (though, realistically, the
>> > + * second try will succeed). Make sure you put a break statement after
>> > + * the close, at the end of your loop. Immediately after your loop,
>> > + * return err if err.
>> > + *
>> > + * You must initialize err to SVN_NO_ERROR and filehandle to NULL, as
>> > + * these macros do not.
>> > */
>> >
>> > -#define SVN_ESTALE_RETRY_COUNT 10
>> > +#define SVN_RECOVERABLE_RETRY_COUNT 10
>> >
>> > #ifdef ESTALE
>> > -#define SVN_RETRY_ESTALE(err, expr) \
>> > - { \
>> > - /* Clear err here (svn_error_clear can safely be passed
>> > - * SVN_NO_ERROR) rather than after finding ESTALE so we can return
>> > - * the ESTALE error on the last iteration of the loop. */ \
>> > - svn_error_clear(err); \
>> > - err = (expr); \
>> > - if (err) \
>> > - { \
>> > - if (APR_TO_OS_ERROR(err->apr_err) == ESTALE) \
>> > - continue; \
>> > - return err; \
>> > - } \
>> > +#define SVN_RETRY_RECOVERABLE(err, filehandle, expr) \
>> > + { \
>> > + svn_error_clear(err); \
>> > + err = (expr); \
>> > + if (err) \
>> > + { \
>> > + apr_status_t _e = APR_TO_OS_ERROR(err->apr_err); \
>> > + if ((_e == ESTALE) || (_e == EIO) || (_e == ENOENT)) { \
>> > + if (NULL != filehandle) \
>> > + (void)apr_file_close(filehandle); \
>> > + continue; \
>> > + } \
>> > + return err; \
>> > + } \
>> > }
>> > -#define SVN_IGNORE_ESTALE(err, expr) \
>> > - { \
>> > - svn_error_clear(err); \
>> > - err = (expr); \
>> > - if (err) \
>> > - { \
>> > - if (APR_TO_OS_ERROR(err->apr_err) != ESTALE) \
>> > - return err; \
>> > - } \
>> > +#define SVN_IGNORE_RECOVERABLE(err, expr) \
>> > + { \
>> > + svn_error_clear(err); \
>> > + err = (expr); \
>> > + if (err) \
>> > + { \
>> > + apr_status_t _e = APR_TO_OS_ERROR(err->apr_err); \
>> > + if ((_e != ESTALE) && (_e != EIO)) \
>> > + return err; \
>> > + } \
>> > }
>> > #else
>> > -#define SVN_RETRY_ESTALE(err, expr) SVN_ERR(expr)
>> > -#define SVN_IGNORE_ESTALE(err, expr) SVN_ERR(expr)
>> > +#define SVN_RETRY_RECOVERABLE(err, filehandle, expr) SVN_ERR(expr)
>> > +#define SVN_IGNORE_RECOVERABLE(err, expr) SVN_ERR(expr)
>> > #endif
>> >
>> > /* Long enough to hold: "<svn_revnum_t> <node id> <copy id>\0"
>> > @@ -1139,7 +1161,7 @@
>> > static svn_error_t *
>> > read_current(const char *fname, char **buf, apr_pool_t *pool)
>> > {
>> > - apr_file_t *revision_file;
>> > + apr_file_t *revision_file = NULL;
>> > apr_size_t len;
>> > int i;
>> > svn_error_t *err = SVN_NO_ERROR;
>> > @@ -1147,18 +1169,20 @@
>> >
>> > *buf = apr_palloc(pool, CURRENT_BUF_LEN);
>> > iterpool = svn_pool_create(pool);
>> > - for (i = 0; i < SVN_ESTALE_RETRY_COUNT; i++)
>> > + for (i = 0; i < SVN_RECOVERABLE_RETRY_COUNT; i++)
>> > {
>> > svn_pool_clear(iterpool);
>> >
>> > - SVN_RETRY_ESTALE(err, svn_io_file_open(&revision_file, fname,
>> > + SVN_RETRY_RECOVERABLE(err, revision_file,
>> > + svn_io_file_open(&revision_file, fname,
>> > APR_READ | APR_BUFFERED,
>> > APR_OS_DEFAULT, iterpool));
>> >
>> > len = CURRENT_BUF_LEN;
>> > - SVN_RETRY_ESTALE(err, svn_io_read_length_line(revision_file,
>> > + SVN_RETRY_RECOVERABLE(err, revision_file,
>> > + svn_io_read_length_line(revision_file,
>> > *buf, &len, iterpool));
>> > - SVN_IGNORE_ESTALE(err, svn_io_file_close(revision_file, iterpool));
>> > + SVN_IGNORE_RECOVERABLE(err, svn_io_file_close(revision_file, iterpool));
>> >
>> > break;
>> > }
>> > @@ -2148,7 +2172,7 @@
>> > svn_revnum_t rev,
>> > apr_pool_t *pool)
>> > {
>> > - apr_file_t *revprop_file;
>> > + apr_file_t *revprop_file = NULL;
>> > apr_hash_t *proplist;
>> > svn_error_t *err = SVN_NO_ERROR;
>> > int i;
>> > @@ -2158,13 +2182,12 @@
>> >
>> > proplist = apr_hash_make(pool);
>> > iterpool = svn_pool_create(pool);
>> > - for (i = 0; i < SVN_ESTALE_RETRY_COUNT; i++)
>> > + for (i = 0; i < SVN_RECOVERABLE_RETRY_COUNT; i++)
>> > {
>> > svn_pool_clear(iterpool);
>> >
>> > - /* Clear err here (svn_error_clear can safely be passed
>> > - * SVN_NO_ERROR) rather than after finding ESTALE so we can
>> > - * return the ESTALE error on the last iteration of the loop. */
>> > + /* Clear err here rather than after finding a recoverable error so
>> > + * we can return that error on the last iteration of the loop. */
>> > svn_error_clear(err);
>> > err = svn_io_file_open(&revprop_file, path_revprops(fs, rev, iterpool),
>> > APR_READ | APR_BUFFERED, APR_OS_DEFAULT,
>> > @@ -2178,20 +2201,22 @@
>> > _("No such revision %ld"), rev);
>> > }
>> > #ifdef ESTALE
>> > - else if (APR_TO_OS_ERROR(err->apr_err) == ESTALE)
>> > + else if (APR_TO_OS_ERROR(err->apr_err) == ESTALE
>> > + || APR_TO_OS_ERROR(err->apr_err) == EIO
>> > + || APR_TO_OS_ERROR(err->apr_err) == ENOENT)
>> > continue;
>> > #endif
>> > return err;
>> > }
>> >
>> > SVN_ERR(svn_hash__clear(proplist));
>> > - SVN_RETRY_ESTALE(err,
>> > - svn_hash_read2(proplist,
>> > - svn_stream_from_aprfile(revprop_file,
>> > - iterpool),
>> > - SVN_HASH_TERMINATOR, pool));
>> > + SVN_RETRY_RECOVERABLE(err, revprop_file,
>> > + svn_hash_read2(proplist,
>> > + svn_stream_from_aprfile(revprop_file,
>> > + iterpool),
>> > + SVN_HASH_TERMINATOR, pool));
>> >
>> > - SVN_IGNORE_ESTALE(err, svn_io_file_close(revprop_file, iterpool));
>> > + SVN_IGNORE_RECOVERABLE(err, svn_io_file_close(revprop_file, iterpool));
>> >
>> > break;
>> > }
>> > @@ -3519,7 +3544,7 @@
>> > {
>> > struct get_and_increment_txn_key_baton *cb = baton;
>> > const char *txn_current_filename = path_txn_current(cb->fs, pool);
>> > - apr_file_t *txn_current_file;
>> > + apr_file_t *txn_current_file = NULL;
>> > const char *tmp_filename;
>> > char next_txn_id[MAX_KEY_SIZE+3];
>> > svn_error_t *err = SVN_NO_ERROR;
>> > @@ -3530,20 +3555,23 @@
>> > cb->txn_id = apr_palloc(cb->pool, MAX_KEY_SIZE);
>> >
>> > iterpool = svn_pool_create(pool);
>> > - for (i = 0; i < SVN_ESTALE_RETRY_COUNT; ++i)
>> > + for (i = 0; i < SVN_RECOVERABLE_RETRY_COUNT; ++i)
>> > {
>> > svn_pool_clear(iterpool);
>> >
>> > - SVN_RETRY_ESTALE(err, svn_io_file_open(&txn_current_file,
>> > + SVN_RETRY_RECOVERABLE(err, txn_current_file,
>> > + svn_io_file_open(&txn_current_file,
>> > txn_current_filename,
>> > APR_READ | APR_BUFFERED,
>> > APR_OS_DEFAULT, iterpool));
>> > len = MAX_KEY_SIZE;
>> > - SVN_RETRY_ESTALE(err, svn_io_read_length_line(txn_current_file,
>> > + SVN_RETRY_RECOVERABLE(err, txn_current_file,
>> > + svn_io_read_length_line(txn_current_file,
>> > cb->txn_id,
>> > &len,
>> > iterpool));
>> > - SVN_IGNORE_ESTALE(err, svn_io_file_close(txn_current_file, iterpool));
>> > + SVN_IGNORE_RECOVERABLE(err, svn_io_file_close(txn_current_file,
>> > + iterpool));
>> >
>> > break;
>> > }
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
>> > For additional commands, e-mail: dev-help_at_subversion.tigris.org
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
>> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>>
>>
>
>
>
> --
> David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-20 00:37:39 CET