Patchwork [1/2] e2image: truncate raw image file to correct size

login
register
mail settings
Submitter Phillip Susi
Date Feb. 16, 2012, 9:35 p.m.
Message ID <1329428112-8911-1-git-send-email-psusi@ubuntu.com>
Download mbox | patch
Permalink /patch/141703/
State Superseded
Headers show

Comments

Phillip Susi - Feb. 16, 2012, 9:35 p.m.
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(-)
Theodore Ts'o - Feb. 16, 2012, 10:58 p.m.
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
Phillip Susi - Feb. 16, 2012, 11:10 p.m.
-----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
Theodore Ts'o - Feb. 16, 2012, 11:30 p.m.
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
Phillip Susi - Feb. 17, 2012, 12:21 a.m.
-----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
Lukas Czerner - Feb. 17, 2012, 10:04 a.m.
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
Theodore Ts'o - Feb. 17, 2012, 2:30 p.m.
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
Phillip Susi - Feb. 17, 2012, 2:31 p.m.
-----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
Phillip Susi - Feb. 17, 2012, 2:46 p.m.
-----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

Patch

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);
 }