[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: Eric Gillespie <epg_at_pretzelnet.org>
Date: Tue, 19 Feb 2008 10:37:30 -0800

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
Received on 2008-02-19 19:37:43 CET

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