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

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

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Tue, 19 Feb 2008 18:00:42 -0500

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
Received on 2008-02-20 00:01:02 CET

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