You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2007/10/19 03:06:49 UTC
error-handling problem in FSFS?
There's the following function in fs_fs.c:
svn_error_t *
svn_fs_fs__move_into_place(const char *old_filename,
const char *new_filename,
const char *perms_reference,
apr_pool_t *pool)
{
svn_error_t *err;
SVN_ERR(svn_fs_fs__dup_perms(old_filename, perms_reference, pool));
/* Move the file into place. */
err = svn_io_file_rename(old_filename, new_filename, pool);
if (err && APR_STATUS_IS_EXDEV(err->apr_err))
{
apr_file_t *file;
/* Can't rename across devices; fall back to copying. */
svn_error_clear(err);
SVN_ERR(svn_io_copy_file(old_filename, new_filename, TRUE, pool));
/* Flush the target of the copy to disk. */
SVN_ERR(svn_io_file_open(&file, new_filename, APR_READ,
APR_OS_DEFAULT, pool));
SVN_ERR(svn_io_file_flush_to_disk(file, pool));
SVN_ERR(svn_io_file_close(file, pool));
}
#ifdef __linux__
{
/* Linux has the unusual feature that fsync() on a file is not
enough to ensure that a file's directory entries have been
flushed to disk; you have to fsync the directory as well.
On other operating systems, we'd only be asking for trouble
by trying to open and fsync a directory. */
const char *dirname;
apr_file_t *file;
dirname = svn_path_dirname(new_filename, pool);
SVN_ERR(svn_io_file_open(&file, dirname, APR_READ, APR_OS_DEFAULT,
pool));
SVN_ERR(svn_io_file_flush_to_disk(file, pool));
SVN_ERR(svn_io_file_close(file, pool));
}
#endif
return err;
}
Let's say that there's an error returned from the rename at the top,
but it's not EXDEV. And let's say that we're on Linux.
Now, the error gets returned. But before it gets returned, the
directory gets opened, flushed, and closed.
This seems wrong to me; I think after the first block there should be
an immediate "SVN_ERR(err)", and the end of the function should just
be "return SVN_NO_ERROR". (Does that sound reasonable?)
Now, assuming that this change is a good idea, the question is, can
the current code actually have negative side effects? That is, can
flushing the parent directory when there was some error in moving the
file ever actually harm the repository? Or is it just an expensive
no-op?
--dave
--
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: error-handling problem in FSFS?
Posted by David Glasser <gl...@davidglasser.net>.
On Nov 3, 2007 3:35 PM, Julian Foad <ju...@btopenworld.com> wrote:
> David Glasser wrote:
> > There's the following function in fs_fs.c:
> >
> > svn_fs_fs__move_into_place(...)
> > {
> > /* Move the file into place. */
> > err = svn_io_file_rename(old_filename, new_filename, pool);
> > if (err && APR_STATUS_IS_EXDEV(err->apr_err))
> > {
> > ...
> > }
> >
> > #ifdef __linux__
> > {
> > ...
> > SVN_ERR(svn_io_file_open(&file, dirname, APR_READ, APR_OS_DEFAULT,
> > pool));
> > SVN_ERR(svn_io_file_flush_to_disk(file, pool));
> > SVN_ERR(svn_io_file_close(file, pool));
> > }
> > #endif
> >
> > return err;
> > }
> >
> > Let's say that there's an error returned from the rename at the top,
> > but it's not EXDEV. And let's say that we're on Linux.
> >
> > Now, the error gets returned. But before it gets returned, the
> > directory gets opened, flushed, and closed.
> >
> > This seems wrong to me; I think after the first block there should be
> > an immediate "SVN_ERR(err)", and the end of the function should just
> > be "return SVN_NO_ERROR". (Does that sound reasonable?)
>
> Yes, that sounds reasonable.
>
> > Now, assuming that this change is a good idea, the question is, can
> > the current code actually have negative side effects? That is, can
> > flushing the parent directory when there was some error in moving the
> > file ever actually harm the repository? Or is it just an expensive
> > no-op?
>
> The directory operations might well fail for the same reason that the file
> rename failed (maybe path not found, file system is read-only, ...), which
> could result in the function returning with the directory opened, which could
> be bad, but I feel almost anything like this that could happen with this wierd
> code ordering could also happen without it.
>
> Perhaps if we can't demonstrate a problem it's safer to wait till 1.5 just in
> case it's actually serving some purpose the way it is.
>
> But what got me interested enough to reply is:
>
> Should we move this OS-specific knowledge to svn_io (at least, or eventually
> APR)? The "flush the directory entry for file F" part in particular, but pretty
> much the whole move-file-into-place body of this function probably. Don't we
> have something very similar (with not quite so strict flushing) in the WC code?
I committed the suggested fix in r28449 and opened issue 3045 to look
into the latter.
--dave
--
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: error-handling problem in FSFS?
Posted by Julian Foad <ju...@btopenworld.com>.
David Glasser wrote:
> There's the following function in fs_fs.c:
>
> svn_fs_fs__move_into_place(...)
> {
> /* Move the file into place. */
> err = svn_io_file_rename(old_filename, new_filename, pool);
> if (err && APR_STATUS_IS_EXDEV(err->apr_err))
> {
> ...
> }
>
> #ifdef __linux__
> {
> ...
> SVN_ERR(svn_io_file_open(&file, dirname, APR_READ, APR_OS_DEFAULT,
> pool));
> SVN_ERR(svn_io_file_flush_to_disk(file, pool));
> SVN_ERR(svn_io_file_close(file, pool));
> }
> #endif
>
> return err;
> }
>
> Let's say that there's an error returned from the rename at the top,
> but it's not EXDEV. And let's say that we're on Linux.
>
> Now, the error gets returned. But before it gets returned, the
> directory gets opened, flushed, and closed.
>
> This seems wrong to me; I think after the first block there should be
> an immediate "SVN_ERR(err)", and the end of the function should just
> be "return SVN_NO_ERROR". (Does that sound reasonable?)
Yes, that sounds reasonable.
> Now, assuming that this change is a good idea, the question is, can
> the current code actually have negative side effects? That is, can
> flushing the parent directory when there was some error in moving the
> file ever actually harm the repository? Or is it just an expensive
> no-op?
The directory operations might well fail for the same reason that the file
rename failed (maybe path not found, file system is read-only, ...), which
could result in the function returning with the directory opened, which could
be bad, but I feel almost anything like this that could happen with this wierd
code ordering could also happen without it.
Perhaps if we can't demonstrate a problem it's safer to wait till 1.5 just in
case it's actually serving some purpose the way it is.
But what got me interested enough to reply is:
Should we move this OS-specific knowledge to svn_io (at least, or eventually
APR)? The "flush the directory entry for file F" part in particular, but pretty
much the whole move-file-into-place body of this function probably. Don't we
have something very similar (with not quite so strict flushing) in the WC code?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org