Richard W.M. Jones
2010-Feb-09 18:38 UTC
[Libguestfs] [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 2001From: 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
Matthew Booth
2010-Feb-10 10:32 UTC
[Libguestfs] [PATCH] Use mount-options instead of mount to avoid implicit -o sync.
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
Reasonably Related Threads
- [PATCH 0/6] Update the way that API versions are generated for the man page.
- [PATCH 1/2] Close all file descriptors in the recovery process.
- Re: [PATCH v2] rescue: add --autosysroot option RHBZ#1183493
- Re: [PATCH] edit: add -m option
- [common PATCH v3 0/1] options: add '--blocksize' option for C-based tools