On Mon, Feb 06, 2006 at 04:49:53PM -0500, Mike Ashmore wrote:
> According to my man pages, POSIX doesn't define a behavior for fsync
> () on a file descriptor that was opened O_RDONLY. Unfortunately,
> STOP's authors chose to make fsync fail on a read-only file
> descriptor, while apparently every other implementor in the galaxy
> chose to succeed (and presumably silently no-op).
>
> Anyway, to get everything working, I had to tweak libsvn_subr/io.c
> just a bit. The patch is attached. And if there's a better way to
> find out if a file descriptor is read-only, please let me know.
>
That patch also falls foul of POSIX, I think: write(fd, ..., 0) is
supposed to return immediately if "the file is a regular file" - no
mention of whether it's open for writing or not. (Though yes, Linux
does check first).
The real question is why we're fsync()'ing a file that's not being
written to. As far as I can see, we only do it in one place, in
libsvn_fs_fs/fs_fs.c:svn_fs_fs__move_into_place(). We rename a file,
then open the new name (read-only) and fsync() it, then (on Linux only)
open the directory and fsync() that, the latter to sync the directory
contents, necessary for at least ext2, and maybe ext3.
Unfortunately, we can't open the directory read-write on Linux, since
the open will then fail. There's no reason we can't open the target
file read-write though, which is what the attached patch does.
Could you try the attached and see if it fixes the problem - STOP may
not care about fsync()'s of directories, since that's a common usage
pattern on Linux.
(Even if it doesn't, does anyone else see any objection to my committing
it? It seems strange that we'd try to fsync() an unwritable file -
even if it does work).
Regards,
Malcolm
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 18392)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -3580,25 +3580,25 @@ svn_fs_fs__move_into_place (const char *
/* 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,
+ SVN_ERR (svn_io_file_open (&file, new_filename, APR_WRITE,
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. */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Feb 8 18:47:53 2006