)]}'
{"nova/virt/images.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"429ae40af92c76edab284b7f62cf3963150d1f76","unresolved":false,"context_lines":[{"line_number":101,"context_line":""},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"def _convert_image(source, dest, in_format, out_format, run_as_root):"},{"line_number":104,"context_line":"    # NOTE(mdbooth): qemu-img convert defaults to cache\u003dunsafe, which means"},{"line_number":105,"context_line":"    # that data is not synced to disk at completion. We explicitly use"},{"line_number":106,"context_line":"    # cache\u003dnone here to (1) ensure that we don\u0027t interfere with other"},{"line_number":107,"context_line":"    # applications using the host\u0027s io cache, and (2) ensure that the data is"},{"line_number":108,"context_line":"    # on persistent storage when the command exits. Without (2), a host crash"},{"line_number":109,"context_line":"    # may leave a corrupt image in the image cache, which Nova cannot recover"},{"line_number":110,"context_line":"    # automatically."},{"line_number":111,"context_line":"    cmd \u003d (\u0027qemu-img\u0027, \u0027convert\u0027, \u0027-t\u0027, \u0027none\u0027, \u0027-O\u0027, out_format)"},{"line_number":112,"context_line":"    if in_format is not None:"},{"line_number":113,"context_line":"        cmd \u003d cmd + (\u0027-f\u0027, in_format)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a30ddce_6b2cdd02","line":110,"range":{"start_line":104,"start_character":0,"end_line":110,"end_character":20},"updated":"2017-03-08 16:59:01.000000000","message":"I assume there\u0027s a reason that this is done, however? What\u0027s the performance implications of none vs unsafe?","commit_id":"1301368bf2352eddcc664202d7f159f523f681e2"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"429ae40af92c76edab284b7f62cf3963150d1f76","unresolved":false,"context_lines":[{"line_number":101,"context_line":""},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"def _convert_image(source, dest, in_format, out_format, run_as_root):"},{"line_number":104,"context_line":"    # NOTE(mdbooth): qemu-img convert defaults to cache\u003dunsafe, which means"},{"line_number":105,"context_line":"    # that data is not synced to disk at completion. We explicitly use"},{"line_number":106,"context_line":"    # cache\u003dnone here to (1) ensure that we don\u0027t interfere with other"},{"line_number":107,"context_line":"    # applications using the host\u0027s io cache, and (2) ensure that the data is"},{"line_number":108,"context_line":"    # on persistent storage when the command exits. Without (2), a host crash"},{"line_number":109,"context_line":"    # may leave a corrupt image in the image cache, which Nova cannot recover"},{"line_number":110,"context_line":"    # automatically."},{"line_number":111,"context_line":"    cmd \u003d (\u0027qemu-img\u0027, \u0027convert\u0027, \u0027-t\u0027, \u0027none\u0027, \u0027-O\u0027, out_format)"},{"line_number":112,"context_line":"    if in_format is not None:"},{"line_number":113,"context_line":"        cmd \u003d cmd + (\u0027-f\u0027, in_format)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a30ddce_2b4bc5ca","line":110,"range":{"start_line":104,"start_character":0,"end_line":110,"end_character":20},"updated":"2017-03-08 16:59:01.000000000","message":"Might also be worth mentioning that the \u0027-t\u0027 parameter is what we\u0027re using here, but that could also be spoon feeding :)","commit_id":"1301368bf2352eddcc664202d7f159f523f681e2"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"7021fdb29d6fdee8ecbbcda0eed94f0e5cf667e8","unresolved":false,"context_lines":[{"line_number":101,"context_line":""},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"def _convert_image(source, dest, in_format, out_format, run_as_root):"},{"line_number":104,"context_line":"    # NOTE(mdbooth): qemu-img convert defaults to cache\u003dunsafe, which means"},{"line_number":105,"context_line":"    # that data is not synced to disk at completion. We explicitly use"},{"line_number":106,"context_line":"    # cache\u003dnone here to (1) ensure that we don\u0027t interfere with other"},{"line_number":107,"context_line":"    # applications using the host\u0027s io cache, and (2) ensure that the data is"},{"line_number":108,"context_line":"    # on persistent storage when the command exits. Without (2), a host crash"},{"line_number":109,"context_line":"    # may leave a corrupt image in the image cache, which Nova cannot recover"},{"line_number":110,"context_line":"    # automatically."},{"line_number":111,"context_line":"    cmd \u003d (\u0027qemu-img\u0027, \u0027convert\u0027, \u0027-t\u0027, \u0027none\u0027, \u0027-O\u0027, out_format)"},{"line_number":112,"context_line":"    if in_format is not None:"},{"line_number":113,"context_line":"        cmd \u003d cmd + (\u0027-f\u0027, in_format)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a30ddce_4e1d67bc","line":110,"range":{"start_line":104,"start_character":0,"end_line":110,"end_character":20},"in_reply_to":"9a30ddce_4e1ac76f","updated":"2017-03-08 17:28:16.000000000","message":"Of course any performance benefit is dependant on the host having a bunch of free RAM to use as i/o cache. As the host becomes more committed to running VMs, there\u0027s less available for i/o cache, so performance benefit would decrease.\n\nIOW, while this would have a performance impact, it should give a performance level that is more consistent, and doesn\u0027t vary according to host RAM availability.\n\nIt could be interesting to look at the CI test results from tempest to see if this change has triggered a meaningful different in running time.","commit_id":"1301368bf2352eddcc664202d7f159f523f681e2"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b285be2e0a228ec47c738ecc8d725299fc4b43d1","unresolved":false,"context_lines":[{"line_number":101,"context_line":""},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"def _convert_image(source, dest, in_format, out_format, run_as_root):"},{"line_number":104,"context_line":"    # NOTE(mdbooth): qemu-img convert defaults to cache\u003dunsafe, which means"},{"line_number":105,"context_line":"    # that data is not synced to disk at completion. We explicitly use"},{"line_number":106,"context_line":"    # cache\u003dnone here to (1) ensure that we don\u0027t interfere with other"},{"line_number":107,"context_line":"    # applications using the host\u0027s io cache, and (2) ensure that the data is"},{"line_number":108,"context_line":"    # on persistent storage when the command exits. Without (2), a host crash"},{"line_number":109,"context_line":"    # may leave a corrupt image in the image cache, which Nova cannot recover"},{"line_number":110,"context_line":"    # automatically."},{"line_number":111,"context_line":"    cmd \u003d (\u0027qemu-img\u0027, \u0027convert\u0027, \u0027-t\u0027, \u0027none\u0027, \u0027-O\u0027, out_format)"},{"line_number":112,"context_line":"    if in_format is not None:"},{"line_number":113,"context_line":"        cmd \u003d cmd + (\u0027-f\u0027, in_format)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a30ddce_dfdc8814","line":110,"range":{"start_line":104,"start_character":0,"end_line":110,"end_character":20},"in_reply_to":"9a30ddce_4e1d67bc","updated":"2017-03-09 09:48:55.000000000","message":"OK, thanks for running up the numbers.\n\n \u003e Personally I\u0027m inclined to keep it simple (and correct) unless this turns out to be an issue in practise.\n\nAs am I, especially when you consider the additional points put forward by Daniel. Could I ask that you include a little of the above in either the commit message or the note just to indicate that we thought about this and decide the compromise was worth it?\n\nChecking out the Tempest results wouldn\u0027t be a half bad call either","commit_id":"1301368bf2352eddcc664202d7f159f523f681e2"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"eb7b0081f9ec31ecf2cf4febd9b5af842e83a431","unresolved":false,"context_lines":[{"line_number":101,"context_line":""},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"def _convert_image(source, dest, in_format, out_format, run_as_root):"},{"line_number":104,"context_line":"    # NOTE(mdbooth): qemu-img convert defaults to cache\u003dunsafe, which means"},{"line_number":105,"context_line":"    # that data is not synced to disk at completion. We explicitly use"},{"line_number":106,"context_line":"    # cache\u003dnone here to (1) ensure that we don\u0027t interfere with other"},{"line_number":107,"context_line":"    # applications using the host\u0027s io cache, and (2) ensure that the data is"},{"line_number":108,"context_line":"    # on persistent storage when the command exits. Without (2), a host crash"},{"line_number":109,"context_line":"    # may leave a corrupt image in the image cache, which Nova cannot recover"},{"line_number":110,"context_line":"    # automatically."},{"line_number":111,"context_line":"    cmd \u003d (\u0027qemu-img\u0027, \u0027convert\u0027, \u0027-t\u0027, \u0027none\u0027, \u0027-O\u0027, out_format)"},{"line_number":112,"context_line":"    if in_format is not None:"},{"line_number":113,"context_line":"        cmd \u003d cmd + (\u0027-f\u0027, in_format)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a30ddce_4e1ac76f","line":110,"range":{"start_line":104,"start_character":0,"end_line":110,"end_character":20},"in_reply_to":"9a30ddce_6b2cdd02","updated":"2017-03-08 17:23:03.000000000","message":"Yeah, I think anybody changing this should reasonably be expected to read the manpage, and know that -t is the cache option :)\n\nThe performance issue is one I briefly discussed with Dan Berrangé (and indirectly with Kevin Wolf), though. cache\u003dunsafe is definitely faster. I ran a few simple tests converting the Fedora 25 cloud image from qcow2 to raw (which is a pretty representative test):\n\n * Use default cache\u003dunsafe, don\u0027t sync\n $ time (qemu-img convert -f qcow2 -O raw Fedora-Cloud-Base-25-1.3.x86_64.qcow2 foo.raw)\n\n real\t0m2.060s\n user\t0m1.789s\n sys\t0m0.282s\n\n * Use cache\u003dnone\n $ time (qemu-img convert -t none -f qcow2 -O raw Fedora-Cloud-Base-25-1.3.x86_64.qcow2 foo.raw)\n\n real\t0m4.824s\n user\t0m2.378s\n sys\t0m0.657s\n\n * Use cache\u003dunsafe, and sync afterwards\n $ time (qemu-img convert -f qcow2 -O raw Fedora-Cloud-Base-25-1.3.x86_64.qcow2 foo.raw \u0026\u0026 sync foo.raw)\n\n real\t0m3.256s\n user\t0m1.765s\n sys\t0m0.459s\n\nI ran all of the above a bunch of times to ensure the times shown are representative. My workstation has an SSD.\n\nSo, we can see that cache\u003dunsafe has a performance benefit on an unloaded workstation even if we manually sync the output afterwards, which is also safe.\n\nThere\u0027s another potential downside of cache\u003dunsafe, though, which is that we\u0027re polluting the hosts\u0027s io cache. i.e. This may cause other operations to slow down more than expected.\n\nIt\u0027s a toss up, though. I went for -t none rather than a subsequent sync step because it\u0027s simpler and self-contained, and won\u0027t affect anything else. It\u0027s about 1.5 secs slower in this test, but I\u0027m guessing that in practise this difference will be negligible next to the time taken to download the image from glance.\n\nPersonally I\u0027m inclined to keep it simple (and correct) unless this turns out to be an issue in practise.","commit_id":"1301368bf2352eddcc664202d7f159f523f681e2"}]}