Re: [PATCH] Retry if NFS gives NOENT or EIO

Or, um, no SVN at all, since they're file-local. Unless we are very
worried about some system header defining them.

--dave

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