| Message ID | 1329428112-8911-1-git-send-email-psusi@ubuntu.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Feb 16, 2012 at 04:35:11PM -0500, Phillip Susi wrote: > At the end of writing the raw image file, output_meta_data_blocks() > wrote a single zero byte. Not only does this cause the last block > of the image file to be non sparse, but this was being skipped if > there were no leftover sparse bytes from the main loop. This would > happen if the source fs happened to have an even multiple of 1MiB > of free blocks at the end, leaving the sparse image file shorter > than it should be. I don't see the bug here. If there are no leftover sparse bytes, there's no need to write the last zero byte. The whole point was to make sure i_size was set correctly, and if sparse==0, then i_size is correct. > Instead of writing a null byte, just truncate() the file instead, > whether or not there are any leftover sparse bytes. ftruncate() happens to work today for Linux, but it's not guaranteed to do anything on all operating systems or even all file systems. Per the standards spec: If the file previously was smaller than this size, ftruncate() shall either increase the size of the file or fail. Speaking of which, you're not checking the return value from ftruncate in your patch. So I'd be happy if you checked ftruncate, and if it failed, falling back to the if (sparse) write_block(fd, zero_buf, sparse-1, 1, -1); code path. That way, if ftruncate() happens to fail on NFS, or ceph, or some random file system that chose to meet the standards spec, but by failing if someone tries to increase the size of the file using ftruncate. (Or some other OS; there are other operating systems, including GNU Hurd and BSD, and I don't know for sure how ftruncate behaves on all of those other OS's and file systems.) - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/16/2012 05:58 PM, Ted Ts'o wrote: > I don't see the bug here. If there are no leftover sparse bytes, > there's no need to write the last zero byte. The whole point was to > make sure i_size was set correctly, and if sparse==0, then i_size is > correct. - From what I can see, when sparse == 0, the last write does a seek to move the file pointer, but doesn't write anything beyond the last hole, so i_size is not updated. This resulted in an image file I took of a 20gb fs being 124 MiB too small. I can only assume that this is to be expected, and is the reason for passing one byte of zero_buff to write_block instead of not giving it any bytes to write, and just asking it to do the seek the way the loop does. > ftruncate() happens to work today for Linux, but it's not guaranteed > to do anything on all operating systems or even all file systems. Per > the standards spec: > > If the file previously was smaller than this size, ftruncate() > shall either increase the size of the file or fail. > > Speaking of which, you're not checking the return value from ftruncate > in your patch. So I'd be happy if you checked ftruncate, and if it > failed, falling back to the > > if (sparse) > write_block(fd, zero_buf, sparse-1, 1, -1); > > code path. That way, if ftruncate() happens to fail on NFS, or ceph, > or some random file system that chose to meet the standards spec, but > by failing if someone tries to increase the size of the file using > ftruncate. (Or some other OS; there are other operating systems, > including GNU Hurd and BSD, and I don't know for sure how ftruncate > behaves on all of those other OS's and file systems.) Good idea. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJPPY0BAAoJEJrBOlT6nu75ZpQH+QH0OGFfY0Tde0SZ3gl1QPeo pbzYRA6Io6uxOMqLwYlOxfmxoaHByuQQupVDAyNtSLVEdGXvaixTNH/omu4TTYcR 54YARfgnsNlpfiJ8cYEP5jqNUvvIfqjqgBZncFYGiP/1smMh8uz56ThkHJtjaaSV hEs1R9F6PdF+cplmsQooRAWedIR8f/Nd9KncKaKHOPiUKDr+3kbneBfbYMrqz6U9 ftoKVYNXNIb0+u9KxOFZybYMkiKoDQrMIUDkXCI39Mgkga/+3SelDZ+vl9Ax142e oEAQfC6RrI86Oh+OjF3YeBQdreyz4ddkEnjFv/EgsfW8PPBw+iYt1NiaXiCUgd4= =Kz4m -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 16, 2012 at 06:10:57PM -0500, Phillip Susi wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 02/16/2012 05:58 PM, Ted Ts'o wrote: > > I don't see the bug here. If there are no leftover sparse bytes, > > there's no need to write the last zero byte. The whole point was to > > make sure i_size was set correctly, and if sparse==0, then i_size is > > correct. > > - From what I can see, when sparse == 0, the last write does a seek > to move the file pointer, but doesn't write anything beyond the last > hole, so i_size is not updated. This resulted in an image file I > took of a 20gb fs being 124 MiB too small. I can only assume that > this is to be expected, and is the reason for passing one byte of > zero_buff to write_block instead of not giving it any bytes to > write, and just asking it to do the seek the way the loop does. Sorry, I'm still not understanding what you're concerned about. The last write should seek to the end of the file system, and write a single byte --- which would be past the last hole. The goal is to make sure the file system is large enough that e2fsck doesn't complain about the file system image being apparently too small. And in fact, it's doing the right thing: tytso.root@tytso-glaptop.cam.corp.google.com> {/home/tytso} 2007# strace -o /tmp/foo /sbin/e2image -r /dev/funarg/library /kbuild/test.img e2image 1.42 (29-Nov-2011) <tytso.root@tytso-glaptop.cam.corp.google.com> {/home/tytso} 2008# tail /tmp/foo lseek(5, 1048576, SEEK_CUR) = 16102031360 lseek(5, 1048576, SEEK_CUR) = 16103079936 lseek(5, 1048576, SEEK_CUR) = 16104128512 lseek(5, 1048576, SEEK_CUR) = 16105177088 lseek(5, 950271, SEEK_CUR) = 16106127359 write(5, "\0", 1) = 1 <===== munmap(0x7f7b6dace000, 495616) = 0 close(4) = 0 close(3) = 0 exit_group(0) = ? I don't understand why you're saying that it's not writing anything beyond the last hole, and why you're saying i_size is not being updated. It's working for me; I can run e2fsck on the generated image, and it's not complaining that the file system is too small. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/16/2012 06:30 PM, Ted Ts'o wrote: > Sorry, I'm still not understanding what you're concerned about. The > last write should seek to the end of the file system, and write a > single byte --- which would be past the last hole. The goal is to > make sure the file system is large enough that e2fsck doesn't > complain about the file system image being apparently too small. That single byte write doesn't happen when sparse == 0. > I don't understand why you're saying that it's not writing anything > beyond the last hole, and why you're saying i_size is not being > updated. It's working for me; I can run e2fsck on the generated > image, and it's not complaining that the file system is too small. It works for you because sparse != 0, thus triggering that one byte write. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJPPZ2CAAoJEJrBOlT6nu75P30H/1KxTJu1NoFk2Wp/JcUwSkj/ gKq+83ykggPvtcDFcGFECRcIBgFdgPyY7pt08RTrvbSjwGLDGbK1zKOFvpYyHHak yBOC96NTmtLAs1oesCVV+C/zjDehhojeh6xuKCC5EpMoL8KdAdREAWOcsr42fuhj Qr/XKlQX8xFTrvmFQLORAOV1e7hHFQKOs1R29pCudQcppm7ZnlAWKHD7pGz9478U tJjxg2B8YUrGriZFe0oAjotxTTTUIK7vErs+tfNgsYmvSFf0XbyC6V0DSpjbK7rf 9RPHaSukNY9ax2dKwGGQp9jQpRZ+RIp5GoZCOSCEOScOviRvHyfdOCgsqEsOYsk= =9RZA -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 16 Feb 2012, Ted Ts'o wrote: > On Thu, Feb 16, 2012 at 06:10:57PM -0500, Phillip Susi wrote: > > -----BEGIN PGP SIGNED MESSAGE----- > > Hash: SHA1 > > > > On 02/16/2012 05:58 PM, Ted Ts'o wrote: > > > I don't see the bug here. If there are no leftover sparse bytes, > > > there's no need to write the last zero byte. The whole point was to > > > make sure i_size was set correctly, and if sparse==0, then i_size is > > > correct. > > > > - From what I can see, when sparse == 0, the last write does a seek > > to move the file pointer, but doesn't write anything beyond the last > > hole, so i_size is not updated. This resulted in an image file I > > took of a 20gb fs being 124 MiB too small. I can only assume that > > this is to be expected, and is the reason for passing one byte of > > zero_buff to write_block instead of not giving it any bytes to > > write, and just asking it to do the seek the way the loop does. > > Sorry, I'm still not understanding what you're concerned about. The > last write should seek to the end of the file system, and write a > single byte --- which would be past the last hole. The goal is to > make sure the file system is large enough that e2fsck doesn't > complain about the file system image being apparently too small. > > And in fact, it's doing the right thing: > > tytso.root@tytso-glaptop.cam.corp.google.com> {/home/tytso} > 2007# strace -o /tmp/foo /sbin/e2image -r /dev/funarg/library /kbuild/test.img > e2image 1.42 (29-Nov-2011) > <tytso.root@tytso-glaptop.cam.corp.google.com> {/home/tytso} > 2008# tail /tmp/foo > lseek(5, 1048576, SEEK_CUR) = 16102031360 > lseek(5, 1048576, SEEK_CUR) = 16103079936 > lseek(5, 1048576, SEEK_CUR) = 16104128512 > lseek(5, 1048576, SEEK_CUR) = 16105177088 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ So if blocks count ends right here^^, then the last write would not happen, because sparse == 0. The reason is that we will seek when the sparse >= 1024*1024 and then set sparse = 0 if (sparse >= 1024*1024) { write_block(fd, 0, sparse, 0, 0); sparse = 0; } if the file system ends right after that point, then we will not write that one last byte. We can easily fix that by doing this instead: if (sparse > 1024*1024) { write_block(fd, 0, 1024*1024, 0, 0); sparse -= 1024*1024; } Thanks! -Lukas > lseek(5, 950271, SEEK_CUR) = 16106127359 > write(5, "\0", 1) = 1 <===== > munmap(0x7f7b6dace000, 495616) = 0 > close(4) = 0 > close(3) = 0 > exit_group(0) = ? > > I don't understand why you're saying that it's not writing anything > beyond the last hole, and why you're saying i_size is not being > updated. It's working for me; I can run e2fsck on the generated > image, and it's not complaining that the file system is too small. > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 17, 2012 at 11:04:22AM +0100, Lukas Czerner wrote: > > lseek(5, 1048576, SEEK_CUR) = 16105177088 > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > So if blocks count ends right here^^, then the last write would not > happen, because sparse == 0. The reason is that we will seek when the > sparse >= 1024*1024 and then set sparse = 0 Yes, there are two cases when sparse can get set to zero. I was thinking of the other one; thanks for pointing that out. > We can easily fix that by doing this instead: > > if (sparse > 1024*1024) { > write_block(fd, 0, 1024*1024, 0, 0); > sparse -= 1024*1024; > } Yep, thanks for the suggestion; I like that. I'll chain to this message the two patches that I've put together which I think should address this issue. The first uses your suggestion so sparse never gets set to zero when we are still extending the hole. The second uses ftruncate64() if possible to set i_size (which saves allocating a block, which is cool when it works). What do folks think? - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 2/17/2012 5:04 AM, Lukas Czerner wrote: > We can easily fix that by doing this instead: > > if (sparse > 1024*1024) { write_block(fd, 0, 1024*1024, 0, 0); > sparse -= 1024*1024; } That occurred to me last night while I slept. I still would like to try to truncate instead of write the last zero though. Patch coming up. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJPPmTOAAoJEJrBOlT6nu75TekH/RAG5FgOHsUU0uMFG3v9Z1av sw/KTmaLpnFGOkQDBYzgZ8+1StfhVdZOUpjNduNWots5cv5+9YoMwyBaLgw51YPN 1HIi5UucRiW5KT8j+FShbHbv4SBtU/PaMvqxangNETttT+MMbPj288QSldEYv1Zf E/0ZPnYxmtd55vs0tee+q0XdSJNLBpMV64nOVIMDPXwOYbjZr+nhAv8LrRGvIk5a 2kduQ2zbN47CjPSod8PjlHIq3MQ+SqYgI9/7aHye9a/K3sGMVMAekacs7vCQquFe lLx593a6F32p6f2ee+3uqleftzUzxRsDAE3Myollq0STLsfdm9+P+5fdfp8iLEs= =BYbM -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 2/17/2012 9:30 AM, Ted Ts'o wrote: > Yep, thanks for the suggestion; I like that. I'll chain to this > message the two patches that I've put together which I think > should address this issue. The first uses your suggestion so > sparse never gets set to zero when we are still extending the > hole. > > The second uses ftruncate64() if possible to set i_size (which > saves allocating a block, which is cool when it works). > > What do folks think? Sounds good to me. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBAgAGBQJPPmg0AAoJEJrBOlT6nu757WsH/Akcc1iQH70NzwKcyxgkGGJf b7/as/olbJE+ZQhmYswMzcwQjwYDsbTfQZLLZM5KRZZGNpmaGhW9FtrV5U7ayiYf FU9duik/BUXfH1pmg6jmyM+QDgIuxFghFT3WOAWS5PosECJ0P0ORQ1sxARp9+P2W jSiaJCeHZZ37dDej1qEOy8S6IKwLPmlIroSuUBckkOcGewBzaZWvQYjxkmz4Dv8V PTmeTFwpcz8mBECHXyun1mE5Ym44yfD+bYazQZO3sVRqF5O+T5uktmte10905g+4 Qluv7rv/6ZT2j/S9MIMylA7pOfJNgcdkjKjSlxZLIofmRVm/1yeA70YJWfkFpCg= =+I+P -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/misc/e2image.c b/misc/e2image.c index d888e5a..722737c 100644 --- a/misc/e2image.c +++ b/misc/e2image.c @@ -511,8 +511,7 @@ static void output_meta_data_blocks(ext2_filsys fs, int fd) } } } - if (sparse) - write_block(fd, zero_buf, sparse-1, 1, -1); + ftruncate(fd, lseek(fd, sparse, SEEK_CUR)); ext2fs_free_mem(&zero_buf); ext2fs_free_mem(&buf); }
At the end of writing the raw image file, output_meta_data_blocks() wrote a single zero byte. Not only does this cause the last block of the image file to be non sparse, but this was being skipped if there were no leftover sparse bytes from the main loop. This would happen if the source fs happened to have an even multiple of 1MiB of free blocks at the end, leaving the sparse image file shorter than it should be. Instead of writing a null byte, just truncate() the file instead, whether or not there are any leftover sparse bytes. Signed-off-by: Phillip Susi <psusi@ubuntu.com> --- misc/e2image.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)