Libguestfs - Oct 2013 - Re: [PATCH 1/2] Preallocate output file

---
 pxzcat.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pxzcat.c b/pxzcat.c
index 4ab8689..9bcdc36 100644
--- a/pxzcat.c
+++ b/pxzcat.c
@@ -29,10 +29,11 @@
  * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
  * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
 +#define _GNU_SOURCE
 #include <config.h>
  #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -166,12 +167,12 @@ xzfile_uncompress (const char *filename, const
char *outputfile,
   debug ("uncompressed size = %" PRIu64 " bytes", size);
    ofd = open (outputfile, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0644);
   if (ofd == -1)
     error (EXIT_FAILURE, errno, "open: %s", outputfile);
-  if (ftruncate (ofd, size) == -1)
-    error (EXIT_FAILURE, errno, "ftruncate: %s", outputfile);
+  if (fallocate (ofd, 0, 0, size) == -1)
+    error (EXIT_FAILURE, errno, "fallocate: %s", outputfile);
    /* Tell the kernel we won't read the output file. */
   posix_fadvise (fd, 0, 0, POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED);
    /* Iterate over blocks. */
-- 
1.8.4.1.563.g8e6fc32

On 10/22/2013 05:56 PM, Gabriel de Perthuis wrote:
> ---
>  pxzcat.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/pxzcat.c b/pxzcat.c
> index 4ab8689..9bcdc36 100644
> --- a/pxzcat.c
> +++ b/pxzcat.c
> @@ -29,10 +29,11 @@
>   * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
>   * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
>  +#define _GNU_SOURCE
>  #include <config.h>
>   #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -166,12 +167,12 @@ xzfile_uncompress (const char *filename, const
> char *outputfile,
>    debug ("uncompressed size = %" PRIu64 " bytes",
size);
>     ofd = open (outputfile, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0644);
>    if (ofd == -1)
>      error (EXIT_FAILURE, errno, "open: %s", outputfile);
> -  if (ftruncate (ofd, size) == -1)
> -    error (EXIT_FAILURE, errno, "ftruncate: %s", outputfile);
> +  if (fallocate (ofd, 0, 0, size) == -1)
> +    error (EXIT_FAILURE, errno, "fallocate: %s", outputfile);
Why exit here.
Maybe just warn and fall back to truncate, for ENOSYS or ENOTSUP
as the (file) system may not support this operation.

Also this adds an upfront space reservation,
which could be seen as a benefit to exit early
if we would not have enough space for the operation,
but also problematic if we don't actually need that
space due to sparseness. Do we get prior knowledge
of the sparseness to be able to fallocate() and appropriate amount?
>     /* Tell the kernel we won't read the output file. */
>    posix_fadvise (fd, 0, 0, POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED);
I know this isn't related to your patch,
but I don't think you can set a flag like this.
These are rather operations that need to be done
while writing I think.

For ref I made the above call after writing various chunks to disk in:
https://github.com/pixelb/dvd-vr/blob/master/dvd-vr.c#L322
and also dd:
http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commit;h=5f31155

BTW dd has this as an option, and dvd-vr is a specialized tool
often handling large files, so in both these cases FADV_DONTNEED
is appropriate. If pxzcat will always be used for virt images then
that does seem appropriate there too, though if used for general
files then probably not.

cheers,
Pádraig.

On Wed, Oct 23, 2013 at 03:38:30AM +0100, Pádraig Brady wrote:
[...]

By the way, Eric Sandeen solved the problem.  It's a genuine
misfeature in ext4 called auto_da_alloc which causes a flush on close
if the file has been truncated (ftruncate or O_TRUNC) and the file
size is zero bytes.  I added these patches which work around the
issue:

http://git.annexia.org/?p=pxzcat.git;a=commitdiff;h=68640d56b2ea96401a1355ab56603b0837058d21
http://git.annexia.org/?p=pxzcat.git;a=commitdiff;h=05f0de58de6cbcbdc40f5a661d406b3fbe5a9060
> >     /* Tell the kernel we won't read the output file. */
> >    posix_fadvise (fd, 0, 0, POSIX_FADV_RANDOM|POSIX_FADV_DONTNEED);
> 
> I know this isn't related to your patch,
> but I don't think you can set a flag like this.
> These are rather operations that need to be done
> while writing I think.
> 
> For ref I made the above call after writing various chunks to disk in:
> https://github.com/pixelb/dvd-vr/blob/master/dvd-vr.c#L322
> and also dd:
> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commit;h=5f31155
This does explain why the posix_fadvise call has no effect.

How about POSIX_FADV_RANDOM?  I would have thought that should be done
before the write operations ...
> BTW dd has this as an option, and dvd-vr is a specialized tool
> often handling large files, so in both these cases FADV_DONTNEED
> is appropriate. If pxzcat will always be used for virt images then
> that does seem appropriate there too, though if used for general
> files then probably not.
> 
> cheers,
> Pádraig.
Thanks, Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW