Libguestfs - Feb 2010 - [PATCH] Use mount-options instead of mount to avoid implicit -o sync.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
-------------- next part --------------
>From 3ec1380eb6425b4f73024200817bd6b192d3b0b0 Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Tue, 9 Feb 2010 18:00:24 +0000
Subject: [PATCH] Use mount-options instead of mount to avoid implicit -o sync.

guestfs_mount adds -o sync implicitly.  This causes a very large
performance problem for write-intensive programs (eg. virt-v2v).

Document this as a "gotcha".

Change the tests, guestfish, Sys::Guestfs::Lib, guestmount to use
mount-options instead.

(Note that this gotcha does not affect mount-ro).

The source of the performance problem was first identified by
Matthew Booth.
---
 fish/fish.c                                        |   11 ++++--
 fuse/guestmount.c                                  |   11 ++++--
 perl/lib/Sys/Guestfs/Lib.pm                        |    2 +-
 perl/t/060-readdir.t                               |    2 +-
 regressions/rhbz503169c10.sh                       |    2 +-
 regressions/rhbz503169c13.sh                       |    2 +-
 .../test-cancellation-upload-daemoncancels.sh      |    2 +-
 regressions/test-remote.sh                         |    2 +-
 src/generator.ml                                   |   34 ++++++++++----------
 src/guestfs.pod                                    |    9 +++++
 test-tool/test-tool.c                              |    2 +-
 11 files changed, 47 insertions(+), 32 deletions(-)

diff --git a/fish/fish.c b/fish/fish.c
index dd73af7..bc518da 100644
--- a/fish/fish.c
+++ b/fish/fish.c
@@ -474,10 +474,13 @@ mount_mps (struct mp *mp)
 
   if (mp) {
     mount_mps (mp->next);
-    if (!read_only)
-      r = guestfs_mount (g, mp->device, mp->mountpoint);
-    else
-      r = guestfs_mount_ro (g, mp->device, mp->mountpoint);
+
+    /* Don't use guestfs_mount here because that will default to mount
+     * options -o sync,noatime.  For more information, see guestfs(3)
+     * section "LIBGUESTFS GOTCHAS".
+     */
+    const char *options = !read_only ? "" : "ro";
+    r = guestfs_mount_options (g, options, mp->device, mp->mountpoint);
     if (r == -1)
       exit (EXIT_FAILURE);
   }
diff --git a/fuse/guestmount.c b/fuse/guestmount.c
index c935493..cbd39f2 100644
--- a/fuse/guestmount.c
+++ b/fuse/guestmount.c
@@ -1160,10 +1160,13 @@ mount_mps (struct mp *mp)
 
   if (mp) {
     mount_mps (mp->next);
-    if (!read_only)
-      r = guestfs_mount (g, mp->device, mp->mountpoint);
-    else
-      r = guestfs_mount_ro (g, mp->device, mp->mountpoint);
+
+    /* Don't use guestfs_mount here because that will default to mount
+     * options -o sync,noatime.  For more information, see guestfs(3)
+     * section "LIBGUESTFS GOTCHAS".
+     */
+    const char *options = !read_only ? "" : "ro";
+    r = guestfs_mount_options (g, options, mp->device, mp->mountpoint);
     if (r == -1)
       exit (EXIT_FAILURE);
   }
diff --git a/perl/lib/Sys/Guestfs/Lib.pm b/perl/lib/Sys/Guestfs/Lib.pm
index 49c08b3..21c9a64 100644
--- a/perl/lib/Sys/Guestfs/Lib.pm
+++ b/perl/lib/Sys/Guestfs/Lib.pm
@@ -1282,7 +1282,7 @@ sub mount_operating_system
             if($ro) {
                 $g->mount_ro ($mounts->{$_}, $_)
             } else {
-                $g->mount ($mounts->{$_}, $_)
+                $g->mount_options ("", $mounts->{$_}, $_)
             }
         }
     }
diff --git a/perl/t/060-readdir.t b/perl/t/060-readdir.t
index 5ed5b7a..dc8278b 100644
--- a/perl/t/060-readdir.t
+++ b/perl/t/060-readdir.t
@@ -38,7 +38,7 @@ $h->part_disk ("/dev/sda", "mbr");
 ok (1);
 $h->mkfs ("ext2", "/dev/sda1");
 ok (1);
-$h->mount ("/dev/sda1", "/");
+$h->mount_options ("", "/dev/sda1", "/");
 ok (1);
 $h->mkdir ("/p");
 ok (1);
diff --git a/regressions/rhbz503169c10.sh b/regressions/rhbz503169c10.sh
index 5cf7069..0a32749 100755
--- a/regressions/rhbz503169c10.sh
+++ b/regressions/rhbz503169c10.sh
@@ -28,7 +28,7 @@ dd if=/dev/zero of=test1.img bs=1024k count=10
 launch
 part-disk /dev/sda mbr
 mkfs ext2 /dev/sda1
-mount /dev/sda1 /
+mount-options "" /dev/sda1 /
 ll /../dev/console
 ll /../dev/full
 ll /../dev/mapper/
diff --git a/regressions/rhbz503169c13.sh b/regressions/rhbz503169c13.sh
index d360d5c..f7ad9e4 100755
--- a/regressions/rhbz503169c13.sh
+++ b/regressions/rhbz503169c13.sh
@@ -33,7 +33,7 @@ dd if=/dev/zero of=test1.img bs=1024k count=10
 run
 part-disk /dev/sda mbr
 mkfs ext2 /dev/sda1
-mount /dev/sda1 /
+mount-options "" /dev/sda1 /
 mkdir /dev
 -command /ignore-this-error
 unmount-all
diff --git a/regressions/test-cancellation-upload-daemoncancels.sh
b/regressions/test-cancellation-upload-daemoncancels.sh
index 296d368..4962c25 100755
--- a/regressions/test-cancellation-upload-daemoncancels.sh
+++ b/regressions/test-cancellation-upload-daemoncancels.sh
@@ -30,7 +30,7 @@ run
 
 part-disk /dev/sda mbr
 mkfs ext2 /dev/sda1
-mount /dev/sda1 /
+mount-options "" /dev/sda1 /
 
 # Upload image, daemon should cancel because the image is too large
 # to upload into itself.
diff --git a/regressions/test-remote.sh b/regressions/test-remote.sh
index d778a07..40c2ee9 100755
--- a/regressions/test-remote.sh
+++ b/regressions/test-remote.sh
@@ -28,7 +28,7 @@ eval `../fish/guestfish --listen`
 ../fish/guestfish --remote run
 ../fish/guestfish --remote part-disk /dev/sda mbr
 ../fish/guestfish --remote mkfs ext2 /dev/sda1
-../fish/guestfish --remote mount /dev/sda1 /
+../fish/guestfish --remote mount-options "" /dev/sda1 /
 
 # Failure of the above commands will cause the guestfish listener to exit.
 # Incorrect return from echo_daemon will not, so need to ensure the listener
diff --git a/src/generator.ml b/src/generator.ml
index b4bbf1e..44dc8a5 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -1432,7 +1432,7 @@ on the volume group C<volgroup>, with C<size>
megabytes.");
    [InitEmpty, Always, TestOutput (
       [["part_disk"; "/dev/sda"; "mbr"];
        ["mkfs"; "ext2"; "/dev/sda1"];
-       ["mount"; "/dev/sda1"; "/"];
+       ["mount_options"; ""; "/dev/sda1";
"/"];
        ["write_file"; "/new"; "new file
contents"; "0"];
        ["cat"; "/new"]], "new file contents")],
    "make a filesystem",
@@ -1508,12 +1508,12 @@ use C<guestfs_upload>.");
    [InitEmpty, Always, TestOutputListOfDevices (
       [["part_disk"; "/dev/sda"; "mbr"];
        ["mkfs"; "ext2"; "/dev/sda1"];
-       ["mount"; "/dev/sda1"; "/"];
+       ["mount_options"; ""; "/dev/sda1";
"/"];
        ["mounts"]], ["/dev/sda1"]);
     InitEmpty, Always, TestOutputList (
       [["part_disk"; "/dev/sda"; "mbr"];
        ["mkfs"; "ext2"; "/dev/sda1"];
-       ["mount"; "/dev/sda1"; "/"];
+       ["mount_options"; ""; "/dev/sda1";
"/"];
        ["umount"; "/"];
        ["mounts"]], [])],
    "unmount a filesystem",
@@ -1544,11 +1544,11 @@ See also: C<guestfs_mountpoints>");
        ["mkfs"; "ext2"; "/dev/sda1"];
        ["mkfs"; "ext2"; "/dev/sda2"];
        ["mkfs"; "ext2"; "/dev/sda3"];
-       ["mount"; "/dev/sda1"; "/"];
+       ["mount_options"; ""; "/dev/sda1";
"/"];
        ["mkdir"; "/mp1"];
-       ["mount"; "/dev/sda2"; "/mp1"];
+       ["mount_options"; ""; "/dev/sda2";
"/mp1"];
        ["mkdir"; "/mp1/mp2"];
-       ["mount"; "/dev/sda3"; "/mp1/mp2"];
+       ["mount_options"; ""; "/dev/sda3";
"/mp1/mp2"];
        ["mkdir"; "/mp1/mp2/mp3"];
        ["umount_all"];
        ["mounts"]], [])],
@@ -2405,11 +2405,11 @@ the human-readable, canonical hex dump of the
file.");
    [InitNone, Always, TestOutput (
       [["part_disk"; "/dev/sda"; "mbr"];
        ["mkfs"; "ext3"; "/dev/sda1"];
-       ["mount"; "/dev/sda1"; "/"];
+       ["mount_options"; ""; "/dev/sda1";
"/"];
        ["write_file"; "/new"; "test file";
"0"];
        ["umount"; "/dev/sda1"];
        ["zerofree"; "/dev/sda1"];
-       ["mount"; "/dev/sda1"; "/"];
+       ["mount_options"; ""; "/dev/sda1";
"/"];
        ["cat"; "/new"]], "test file")],
    "zero unused inodes and disk blocks on ext2/3 filesystem",
    "\
@@ -2510,13 +2510,13 @@ are activated or deactivated.");
        ["vgcreate"; "VG"; "/dev/sda1"];
        ["lvcreate"; "LV"; "VG"; "10"];
        ["mkfs"; "ext2"; "/dev/VG/LV"];
-       ["mount"; "/dev/VG/LV"; "/"];
+       ["mount_options"; ""; "/dev/VG/LV";
"/"];
        ["write_file"; "/new"; "test content";
"0"];
        ["umount"; "/"];
        ["lvresize"; "/dev/VG/LV"; "20"];
        ["e2fsck_f"; "/dev/VG/LV"];
        ["resize2fs"; "/dev/VG/LV"];
-       ["mount"; "/dev/VG/LV"; "/"];
+       ["mount_options"; ""; "/dev/VG/LV";
"/"];
        ["cat"; "/new"]], "test content")],
    "resize an LVM logical volume",
    "\
@@ -3560,7 +3560,7 @@ and C<guestfs_setcon>");
    [InitEmpty, Always, TestOutput (
       [["part_disk"; "/dev/sda"; "mbr"];
        ["mkfs_b"; "ext2"; "4096";
"/dev/sda1"];
-       ["mount"; "/dev/sda1"; "/"];
+       ["mount_options"; ""; "/dev/sda1";
"/"];
        ["write_file"; "/new"; "new file
contents"; "0"];
        ["cat"; "/new"]], "new file contents")],
    "make a filesystem with block size",
@@ -3575,7 +3575,7 @@ are C<1024>, C<2048> or C<4096>
only.");
       [["sfdiskM"; "/dev/sda"; ",100 ,"];
        ["mke2journal"; "4096"; "/dev/sda1"];
        ["mke2fs_J"; "ext2"; "4096";
"/dev/sda2"; "/dev/sda1"];
-       ["mount"; "/dev/sda2"; "/"];
+       ["mount_options"; ""; "/dev/sda2";
"/"];
        ["write_file"; "/new"; "new file
contents"; "0"];
        ["cat"; "/new"]], "new file contents")],
    "make ext2/3/4 external journal",
@@ -3590,7 +3590,7 @@ to the command:
       [["sfdiskM"; "/dev/sda"; ",100 ,"];
        ["mke2journal_L"; "4096"; "JOURNAL";
"/dev/sda1"];
        ["mke2fs_JL"; "ext2"; "4096";
"/dev/sda2"; "JOURNAL"];
-       ["mount"; "/dev/sda2"; "/"];
+       ["mount_options"; ""; "/dev/sda2";
"/"];
        ["write_file"; "/new"; "new file
contents"; "0"];
        ["cat"; "/new"]], "new file contents")],
    "make ext2/3/4 external journal with label",
@@ -3603,7 +3603,7 @@ This creates an ext2 external journal on C<device>
with label C<label>.");
        [["sfdiskM"; "/dev/sda"; ",100 ,"];
         ["mke2journal_U"; "4096"; uuid;
"/dev/sda1"];
         ["mke2fs_JU"; "ext2"; "4096";
"/dev/sda2"; uuid];
-        ["mount"; "/dev/sda2"; "/"];
+        ["mount_options"; ""; "/dev/sda2";
"/"];
         ["write_file"; "/new"; "new file
contents"; "0"];
         ["cat"; "/new"]], "new file contents")]),
    "make ext2/3/4 external journal with UUID",
@@ -4218,7 +4218,7 @@ Rename a logical volume C<logvol> with the new name
C<newlogvol>.");
        ["vg_activate"; "false"; "VG"];
        ["vgrename"; "VG"; "VG2"];
        ["vg_activate"; "true"; "VG2"];
-       ["mount"; "/dev/VG2/LV"; "/"];
+       ["mount_options"; ""; "/dev/VG2/LV";
"/"];
        ["vgs"]], ["VG2"])],
    "rename an LVM volume group",
    "\
@@ -6500,7 +6500,7 @@ and generate_one_test_body name i test_name init test     
["lvm_remove_all"];
           ["part_disk"; "/dev/sda"; "mbr"];
           ["mkfs"; "ext2"; "/dev/sda1"];
-          ["mount"; "/dev/sda1"; "/"]]
+          ["mount_options"; ""; "/dev/sda1";
"/"]]
    | InitBasicFSonLVM ->
        pr "  /* InitBasicFSonLVM for %s: create ext2 on /dev/VG/LV
*/\n"
          test_name;
@@ -6513,7 +6513,7 @@ and generate_one_test_body name i test_name init test     
["vgcreate"; "VG"; "/dev/sda1"];
           ["lvcreate"; "LV"; "VG";
"8"];
           ["mkfs"; "ext2"; "/dev/VG/LV"];
-          ["mount"; "/dev/VG/LV"; "/"]]
+          ["mount_options"; ""; "/dev/VG/LV";
"/"]]
    | InitISOFS ->
        pr "  /* InitISOFS for %s */\n" test_name;
        List.iter (generate_test_command_call test_name)
diff --git a/src/guestfs.pod b/src/guestfs.pod
index 84eec63..7f810f3 100644
--- a/src/guestfs.pod
+++ b/src/guestfs.pod
@@ -558,6 +558,15 @@ Note that in L<guestfish(3)> I<autosync is the
default>.  So quick and
 dirty guestfish scripts that forget to sync will work just fine, which
 can make this extra-puzzling if you are trying to debug a problem.
 
+=item Mount option C<-o sync> should not be the default.
+
+If you use C<guestfs_mount>, then C<-o sync,noatime> are added
+implicitly.  However C<-o sync> does not add any reliability benefit,
+but does have a very large performance impact.
+
+The work around is to use C<guestfs_mount_options> and set the mount
+options that you actually want to use.
+
 =item Read-only should be the default.
 
 In L<guestfish(3)>, I<--ro> should be the default, and you should
diff --git a/test-tool/test-tool.c b/test-tool/test-tool.c
index 39139e5..cc47c01 100644
--- a/test-tool/test-tool.c
+++ b/test-tool/test-tool.c
@@ -229,7 +229,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
 
-  if (guestfs_mount (g, "/dev/sda1", "/") == -1) {
+  if (guestfs_mount_options (g, "", "/dev/sda1",
"/") == -1) {
     fprintf (stderr,
              _("libguestfs-test-tool: failed to mount /dev/sda1 on
/\n"));
     exit (EXIT_FAILURE);
-- 
1.6.5.2

On 09/02/10 18:38, Richard W.M. Jones wrote:
> Subject: [PATCH] Use mount-options instead of mount to avoid implicit -o
sync.
> 
> guestfs_mount adds -o sync implicitly.  This causes a very large
> performance problem for write-intensive programs (eg. virt-v2v).
> 
> Document this as a "gotcha".
> 
> Change the tests, guestfish, Sys::Guestfs::Lib, guestmount to use
> mount-options instead.
> 
> (Note that this gotcha does not affect mount-ro).
> 
> The source of the performance problem was first identified by
> Matthew Booth.
> ---
>  fish/fish.c                                        |   11 ++++--
>  fuse/guestmount.c                                  |   11 ++++--
>  perl/lib/Sys/Guestfs/Lib.pm                        |    2 +-
>  perl/t/060-readdir.t                               |    2 +-
>  regressions/rhbz503169c10.sh                       |    2 +-
>  regressions/rhbz503169c13.sh                       |    2 +-
>  .../test-cancellation-upload-daemoncancels.sh      |    2 +-
>  regressions/test-remote.sh                         |    2 +-
>  src/generator.ml                                   |   34
++++++++++----------
>  src/guestfs.pod                                    |    9 +++++
>  test-tool/test-tool.c                              |    2 +-
>  11 files changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/fish/fish.c b/fish/fish.c
> index dd73af7..bc518da 100644
> --- a/fish/fish.c
> +++ b/fish/fish.c
> @@ -474,10 +474,13 @@ mount_mps (struct mp *mp)
>  
>    if (mp) {
>      mount_mps (mp->next);
> -    if (!read_only)
> -      r = guestfs_mount (g, mp->device, mp->mountpoint);
> -    else
> -      r = guestfs_mount_ro (g, mp->device, mp->mountpoint);
> +
> +    /* Don't use guestfs_mount here because that will default to mount
> +     * options -o sync,noatime.  For more information, see guestfs(3)
> +     * section "LIBGUESTFS GOTCHAS".
> +     */
> +    const char *options = !read_only ? "" : "ro";
Can you make that:

const char *options = read_only ? "ro" : "";

Extra negations hurt my feeble mind ;)
> +    r = guestfs_mount_options (g, options, mp->device,
mp->mountpoint);
>      if (r == -1)
>        exit (EXIT_FAILURE);
>    }
> diff --git a/fuse/guestmount.c b/fuse/guestmount.c
> index c935493..cbd39f2 100644
> --- a/fuse/guestmount.c
> +++ b/fuse/guestmount.c
> @@ -1160,10 +1160,13 @@ mount_mps (struct mp *mp)
>  
>    if (mp) {
>      mount_mps (mp->next);
> -    if (!read_only)
> -      r = guestfs_mount (g, mp->device, mp->mountpoint);
> -    else
> -      r = guestfs_mount_ro (g, mp->device, mp->mountpoint);
> +
> +    /* Don't use guestfs_mount here because that will default to mount
> +     * options -o sync,noatime.  For more information, see guestfs(3)
> +     * section "LIBGUESTFS GOTCHAS".
> +     */
> +    const char *options = !read_only ? "" : "ro";
And again.
> +    r = guestfs_mount_options (g, options, mp->device,
mp->mountpoint);
>      if (r == -1)
>        exit (EXIT_FAILURE);
>    }
ACK, regardless of above.

Matt

-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

M:       +44 (0)7977 267231
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490