You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Dan Christian <dc...@google.com> on 2008/02/14 00:15:39 UTC
[PATCH] Retry if NFS gives NOENT or EIO
In theory, this patch isn't needed. In practice it may be a good idea.
Some NFS implementations (e.g. linux-2.6 client with netapp server)
can return NOENT or EIO on files that have been renamed (e.g.
db/current). This is a NFS bug and only happens under load, but it
still might be wise to apply the work around.
[[[
* subversion/libsvn_fs_fs/fs_fs.c
Work around buggy NFS implementations.
In addition to retrying on ESTALE, also retry on NOENT and EIO
]]]
Re: [PATCH] Retry if NFS gives NOENT or EIO
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@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 <kf...@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 <ep...@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@subversion.tigris.org
>> > For additional commands, e-mail: dev-help@subversion.tigris.org
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>> For additional commands, e-mail: dev-help@subversion.tigris.org
>>
>>
>
>
>
> --
> David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Retry if NFS gives NOENT or EIO
Posted by David Glasser <gl...@davidglasser.net>.
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 <kf...@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 <ep...@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@subversion.tigris.org
> > For additional commands, e-mail: dev-help@subversion.tigris.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Retry if NFS gives NOENT or EIO
Posted by Karl Fogel <kf...@red-bean.com>.
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 <ep...@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@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Retry if NFS gives NOENT or EIO
Posted by Eric Gillespie <ep...@pretzelnet.org>.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Retry if NFS gives NOENT or EIO
Posted by Dan Christian <dc...@google.com>.
Updated log entry.
[[[
* subversion/libsvn_fs_fs/fs_fs.c
(SVN_RETRY_ESTALE): also retry on NOENT and EIO.
(SVN_IGNORE_ESTALE): also ignore EIO.
(read_current, svn_fs_fs__revision_proplist, get_and_increment_txn_key_body):
New filehandle argument to SVN_RETYRY_ESTALE.
]]]
On Wed, Feb 13, 2008 at 4:15 PM, Dan Christian <dc...@google.com> wrote:
> In theory, this patch isn't needed. In practice it may be a good idea.
>
> Some NFS implementations (e.g. linux-2.6 client with netapp server)
> can return NOENT or EIO on files that have been renamed (e.g.
> db/current). This is a NFS bug and only happens under load, but it
> still might be wise to apply the work around.
>
> [[[
> * subversion/libsvn_fs_fs/fs_fs.c
> Work around buggy NFS implementations.
> In addition to retrying on ESTALE, also retry on NOENT and EIO
> ]]]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Retry if NFS gives NOENT or EIO
Posted by Eric Gillespie <ep...@google.com>.
"Dan Christian" <dc...@google.com> writes:
> In theory, this patch isn't needed. In practice it may be a good idea.
>
> Some NFS implementations (e.g. linux-2.6 client with netapp server)
> can return NOENT or EIO on files that have been renamed (e.g.
> db/current). This is a NFS bug and only happens under load, but it
> still might be wise to apply the work around.
I don't think the server matters, but I'm not sure. At least,
merely changing the client can make it go away. We see the
problem with Linux 2.6.18.5 (Ubuntu Dapper) but not 2.6.24 (as
yet unreleased Ubuntu Hardy). I'll add that to the comments
before committing (assuming no one objects :).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org