tag 567089 + patch
thanks
Hi,
On Sun, 14 Feb 2010, Jean-Baptiste Lallement wrote:
> Here is a patch which adds some fsync on files and directories to
> prevent delayed allocation and the zero-length file problem on ext4
> filesystem.
Great, thanks for this!
> This patch is build against dpkg 1.15.5.6ubuntu1, I'll try to provide a
> version for debian soon.
Please make it conform the coding style (see doc/coding-style.txt in git
repo), I saw several stylistic problems:
- missing space before comma, operator, or =
- unwanted spaces after ( or before )
- invalid indenting (be consistent with the rest of the file)
- use "" or '' quotes in new strings (and not `')
On question though, is that the minimal set of fsync needed to get the
right result? I fear that we loose lots of performance in particular for
the change in src/archive.c (fsync newly extracted files). Maybe it's best
to not do it at this point but add a generic sync() somewhere when the
database is synced to disk. Not sure though. Also even ext4 implicitely
sync files on rename() IIRC so it's not strictly needed AFAIK.
Can you explain your changes in more details, because I'm not sure I
understand it completely:
> + * Add fsync on files and directories to prevent delayed allocation and the
> + zero-length file problem after a system crash LP: #512096 (Debian
> #567089)
> + - dpkg-deb/extract.c: fsync control files
You modify extracthalf() which is used for extracting control files but
also the main files of the package itself (depending on whether admin is
true or not). You call fsync on all files of the directory indicated.
In the case of the control archives (control.tar.gz), it's fine we only
have files at the root directory but in the case of data.tar.gz you
definitely won't fsync all installed files.
This lead me to believe that it's not the proper place for this code.
Instead I think (but I would want Guillem's opinion) that it should
be in process_archive() just after the call to dpkg-deb --control.
And you probably want a separate function to sync all files of a
directory.
> + - src/archive.c: fsync newly extracted files
See comment in first para.
> + - lib/dpkg/cleanup.c: fsync directory before closing
I doubt that this is the proper place. What directory needs to be
fsynced() and at what time of the installation/upgrade process ?
Probably only the pop_cleanup() calls in src/processarc.c and not the others
so it really needs to be moved there directly.
> + - lib/dpkg/dump.c: fsync db directory
This one looks okay.
Code review:
> + while ((de = readdir(dsd)) != NULL){
> + if (strchr(de->d_name,'.'))
> + continue;
Why skipping files containing a dot? Maybe you wanted to skip "." and ".."
but that's not what this code does.
> diff -Nru dpkg-1.15.5.6ubuntu1/lib/dpkg/dump.c
> dpkg-1.15.5.6ubuntu2/lib/dpkg/dump.c
> --- dpkg-1.15.5.6ubuntu1/lib/dpkg/dump.c 2010-02-14 02:40:26.000000000
> +0100
> +++ dpkg-1.15.5.6ubuntu2/lib/dpkg/dump.c 2010-02-14 12:08:08.000000000
> +0100
> + /* We need to sync the directory */
> + fname = strdup( filename );
> + dname = strrchr( fname, '/' );
> + if ( dname && *dname )
> + *dname = '\0';
> +
> + if ( (dirp = opendir(fname))==NULL)
> + ohshite(_("failed to opendir `%.250s'"), fname );
> + if ( fsync(dirfd(dirp)) == -1 )
> + ohshite(_("failed to fsync `%.250s'"), fname );
> + closedir( dirp );
> + free( fname );
Can't we open the directory directly instead of using opendir + dirfd? Or
is that non-portable (I don't know)?
Cheers,
--
Raphaël Hertzog
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
Archive: http://lists.debian.org/20100214165032.ga1...@rivendell
