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

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

[Phillip Susi]

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(-)

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

[PATCH 2/2] e2image: add -a switch to include all data

[Phillip Susi]

Normally the raw and QCOW2 images only contain fs metadata.
Add a new switch ( -a ) to include all data.  This makes it
possible to use e2image to clone a whole filesystem.

Signed-off-by: Phillip Susi <psusi@ubuntu.com>
---
 misc/e2image.8.in |   21 ++++++++++++++++++++-
 misc/e2image.c    |   12 ++++++++----
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/misc/e2image.8.in b/misc/e2image.8.in
index 74d2a0b..b76dbe8 100644
--- a/misc/e2image.8.in
+++ b/misc/e2image.8.in
@@ -8,7 +8,7 @@ e2image \- Save critical ext2/ext3/ext4 filesystem metadata to a file
 .SH SYNOPSIS
 .B e2image
 [
-.B \-rsI
+.B \-rsIQa
 ]
 .I device
 .I image-file
@@ -171,6 +171,25 @@ is regular QCOW2 image and can be processed by tools aware of QCOW2 format
 such as for example
 .BR qemu-img .
 .PP
+You can convert a qcow2 image into a raw image with:
+.PP
+.br
+\	\fBe2image \-r hda1.qcow2 hda1.raw\fR
+.br
+.PP
+This can be useful to write a qcow2 image containing all data to a
+sparse image file where it can be loop mounted, or to a disk partition
+.PP
+.SH INCLUDING DATA
+Normally
+.B e2image
+only includes fs metadata, not regular file data.  The 
+.B \-a
+option can be specified to include all data.  This will
+give an image that is suitible to use to clone the entire FS or
+for backup purposes.  Note that this option only works with the
+raw or QCOW2 formats.
+.PP
 .SH AUTHOR
 .B e2image 
 was written by Theodore Ts'o (tytso@mit.edu).
diff --git a/misc/e2image.c b/misc/e2image.c
index 722737c..b8f10a1 100644
--- a/misc/e2image.c
+++ b/misc/e2image.c
@@ -52,6 +52,7 @@ extern int optind;
 
 const char * program_name = "e2image";
 char * device_name = NULL;
+char all_data;
 
 static void lseek_error_and_exit(int errnum)
 {
@@ -84,7 +85,7 @@ static int get_bits_from_size(size_t size)
 
 static void usage(void)
 {
-	fprintf(stderr, _("Usage: %s [-rsIQ] device image_file\n"),
+	fprintf(stderr, _("Usage: %s [-rsIQa] device image_file\n"),
 		program_name);
 	exit (1);
 }
@@ -309,7 +310,7 @@ static int process_file_block(ext2_filsys fs EXT2FS_ATTR((unused)),
 			      int ref_offset EXT2FS_ATTR((unused)),
 			      void *priv_data EXT2FS_ATTR((unused)))
 {
-	if (blockcnt < 0) {
+	if (blockcnt < 0 || all_data) {
 		ext2fs_mark_block_bitmap2(meta_block_map, *block_nr);
 		meta_blocks_count++;
 	}
@@ -1114,7 +1115,7 @@ static void write_raw_image_file(ext2_filsys fs, int fd, int type, int flags)
 			if ((inode.i_flags & EXT4_EXTENTS_FL) ||
 			    inode.i_block[EXT2_IND_BLOCK] ||
 			    inode.i_block[EXT2_DIND_BLOCK] ||
-			    inode.i_block[EXT2_TIND_BLOCK]) {
+			    inode.i_block[EXT2_TIND_BLOCK] || all_data) {
 				retval = ext2fs_block_iterate3(fs,
 				       ino, BLOCK_FLAG_READ_ONLY, block_buf,
 				       process_file_block, &pb);
@@ -1242,7 +1243,7 @@ int main (int argc, char ** argv)
 	if (argc && *argv)
 		program_name = *argv;
 	add_error_table(&et_ext2_error_table);
-	while ((c = getopt(argc, argv, "rsIQ")) != EOF)
+	while ((c = getopt(argc, argv, "rsIQa")) != EOF)
 		switch (c) {
 		case 'I':
 			flags |= E2IMAGE_INSTALL_FLAG;
@@ -1260,6 +1261,9 @@ int main (int argc, char ** argv)
 		case 's':
 			flags |= E2IMAGE_SCRAMBLE_FLAG;
 			break;
+		case 'a':
+			all_data = 1;
+			break;
 		default:
 			usage();
 		}
-- 
1.7.5.4

Re: [PATCH 2/2] e2image: add -a switch to include all data

[Ted Ts'o]

> +You can convert a qcow2 image into a raw image with:
> +.PP
> +.br
> +\	\fBe2image \-r hda1.qcow2 hda1.raw\fR
> +.br
> +.PP
> +This can be useful to write a qcow2 image containing all data to a
> +sparse image file where it can be loop mounted, or to a disk partition

As I recall Lukas disclaimed a guarantee that the code would work on
qcow2 images that weren't generated by e2image.  (In particular, it
definitely doesn't support compressed or encrypted qcow2 images.)

So we need to make sure we add the appropriate disclaimers that this
might not work on qcow2 images generated by tools other than e2image.

If someone would like to work on improving lib/ext2fs/qcow2.c to add
those missing features, and more importantly, add test cases, it would
be great if our qcow2 support could be made more complete.

> +.PP
> +.SH INCLUDING DATA
> +Normally
> +.B e2image
> +only includes fs metadata, not regular file data.  The 
> +.B \-a
> +option can be specified to include all data.  This will
> +give an image that is suitible to use to clone the entire FS or
> +for backup purposes.  Note that this option only works with the
> +raw or QCOW2 formats.

It will only work for raw images.  The QCOW2 format uses an entirely
different code path, since we don't have an QCOW2 io_manager
abstraction.  That was my original hope, but that's not how our qcow2
support was implemented, so it won't work, and we should probably give
a reasonable warning if someone tries to use the -a flag with anything
other than a raw file system image for e2image's input.

						- Ted

Re: [PATCH 2/2] e2image: add -a switch to include all data

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/16/2012 06:17 PM, Ted Ts'o wrote:
> As I recall Lukas disclaimed a guarantee that the code would work on
> qcow2 images that weren't generated by e2image.  (In particular, it
> definitely doesn't support compressed or encrypted qcow2 images.)
> 
> So we need to make sure we add the appropriate disclaimers that this
> might not work on qcow2 images generated by tools other than e2image.

Would you care to craft such a disclaimer?

> It will only work for raw images.  The QCOW2 format uses an entirely
> different code path, since we don't have an QCOW2 io_manager
> abstraction.  That was my original hope, but that's not how our qcow2
> support was implemented, so it won't work, and we should probably give
> a reasonable warning if someone tries to use the -a flag with anything
> other than a raw file system image for e2image's input.

It seemed to work fine with qcow2 when I tested it.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJPPZyzAAoJEJrBOlT6nu75U2UIAMgAF/+pIFQCiXCQGMqt+6rR
4k2P2Q/NfRIIf5lSuNaMCuV88ZuUU002ywEN6z9hPfxXDIagDAzOArtVI2nAhwfz
Z4BF7uJ+7eJDCDQycNDb4JmAQDRs2sBuu3Y0dHKWthyd39KYMR7B0ARwCSeYLczB
eigCiQwTVechP2jacWwNnv53BP2xbw/BYy5IHfQViSKijxGNdheQkYqOTIO5eGGB
NdPx5A6yogLOHyB3K3E9cqEnAjXSlelLH7sRCPAmHmQiJVscAiNZ6JvyP/oV0uek
HlmvP0LyZwzMvy8XlgoybHNRLysw+VEVVuLIrAzZJHiW7aWH3WPtqrPEQp6XoJY=
=cmZg
-----END PGP SIGNATURE-----

Re: [PATCH 2/2] e2image: add -a switch to include all data

[Lukas Czerner]

On Thu, 16 Feb 2012, Phillip Susi wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/16/2012 06:17 PM, Ted Ts'o wrote:
> > As I recall Lukas disclaimed a guarantee that the code would work on
> > qcow2 images that weren't generated by e2image.  (In particular, it
> > definitely doesn't support compressed or encrypted qcow2 images.)
> > 
> > So we need to make sure we add the appropriate disclaimers that this
> > might not work on qcow2 images generated by tools other than e2image.
> 
> Would you care to craft such a disclaimer?
> 
> > It will only work for raw images.  The QCOW2 format uses an entirely
> > different code path, since we don't have an QCOW2 io_manager
> > abstraction.  That was my original hope, but that's not how our qcow2
> > support was implemented, so it won't work, and we should probably give
> > a reasonable warning if someone tries to use the -a flag with anything
> > other than a raw file system image for e2image's input.
> 
> It seemed to work fine with qcow2 when I tested it.

So the problem actually is that I have made some assumptions, like for
example that all data are laid out sequentially (because it is how we
are creating the qcow2 images with e2image) and possibly more. It also
does not support encrypted and compressed images, but it will warn you
about that. And most importantly I have never tested other qcow2 images
other than those generated with e2image, simply because that this is not
the intention of this tool (you have qemu to do that). But if you're able
to test it reliably and create a test case so we can be sure that it
actually works, then we would not need such disclaimer I guess.

Note that you'll have to test that qcow2 images generated by e2image
actually works in other tools reliably (qemu) (again this was not the
intention, we just used qemu2 format because it is space efficient and
non-sparse, so te image can be easily transferred), and that e2image
works well with images generated by other tools. I am not saying that it
would not work (I guess it should), I just never tested it.

The good thing about the qcow2 -> raw conversion done by e2image is that
it is *a lot* faster than qemu implementation (at least when I tested
it). I have never tried to find out why, unfortunately I have not even
tried to see what could be fixed in qemu (I really should:)).

Thanks!
-Lukas

Re: [PATCH 2/2] e2image: add -a switch to include all data

[Ted Ts'o]

On Fri, Feb 17, 2012 at 10:17:43AM +0100, Lukas Czerner wrote:
> 
> So the problem actually is that I have made some assumptions, like for
> example that all data are laid out sequentially (because it is how we
> are creating the qcow2 images with e2image) and possibly more. 

So if your code doesn't work correctly if the block numbers are
monotonically increasing in the qcow2 image, I would *have* to assume
there would be plenty of qemu-generated images files that don't have
this constraint (i.e., suppose you write blocks 10000, 42, 16, and 24
while running under qemu, presumably that is the order the blocks
would be written and would show up in the qcow2 file).

Or am I misunderstanding what you are saying here; is it that you are
fairly sure the code will break if the data is not laid out
sequentially, or that you've never tested this case?

Regards,

						- Ted

Re: [PATCH 2/2] e2image: add -a switch to include all data

[Lukas Czerner]

On Fri, 17 Feb 2012, Ted Ts'o wrote:

> On Fri, Feb 17, 2012 at 10:17:43AM +0100, Lukas Czerner wrote:
> > 
> > So the problem actually is that I have made some assumptions, like for
> > example that all data are laid out sequentially (because it is how we
> > are creating the qcow2 images with e2image) and possibly more. 
> 
> So if your code doesn't work correctly if the block numbers are
> monotonically increasing in the qcow2 image, I would *have* to assume
> there would be plenty of qemu-generated images files that don't have
> this constraint (i.e., suppose you write blocks 10000, 42, 16, and 24
> while running under qemu, presumably that is the order the blocks
> would be written and would show up in the qcow2 file).
> 
> Or am I misunderstanding what you are saying here; is it that you are
> fairly sure the code will break if the data is not laid out
> sequentially, or that you've never tested this case?

Hi Ted,

as I said I have never tested this. Well, actually I did some very
limited testing, but since this was not (and still is not right?) the
scope of e2image I did not care enough to test that extensively enough
to be sure that it generally works with qcow2 images other than those
generated with e2image. But I think it should work.

Also I suppose that qemu is being a bit smarter than just writing every
extent out so it does not seek madly back and forth when the image is
not laid out sequentially. e2image does not do that.

Thanks!
-Lukas

> 
> Regards,
> 
> 						- 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
> 

Re: [PATCH 2/2] e2image: add -a switch to include all data

[Lukas Czerner]

On Fri, 17 Feb 2012, Lukas Czerner wrote:

> On Fri, 17 Feb 2012, Ted Ts'o wrote:
> 
> > On Fri, Feb 17, 2012 at 10:17:43AM +0100, Lukas Czerner wrote:
> > > 
> > > So the problem actually is that I have made some assumptions, like for
> > > example that all data are laid out sequentially (because it is how we
> > > are creating the qcow2 images with e2image) and possibly more. 
> > 
> > So if your code doesn't work correctly if the block numbers are
> > monotonically increasing in the qcow2 image, I would *have* to assume
> > there would be plenty of qemu-generated images files that don't have
> > this constraint (i.e., suppose you write blocks 10000, 42, 16, and 24
> > while running under qemu, presumably that is the order the blocks
> > would be written and would show up in the qcow2 file).
> > 
> > Or am I misunderstanding what you are saying here; is it that you are
> > fairly sure the code will break if the data is not laid out
> > sequentially, or that you've never tested this case?
> 
> Hi Ted,
> 
> as I said I have never tested this. Well, actually I did some very
> limited testing, but since this was not (and still is not right?) the
> scope of e2image I did not care enough to test that extensively enough
> to be sure that it generally works with qcow2 images other than those
> generated with e2image. But I think it should work.
> 
> Also I suppose that qemu is being a bit smarter than just writing every
> extent out so it does not seek madly back and forth when the image is
> not laid out sequentially. e2image does not do that.

ehm..I meant, e2image does not do any smart things there, just seek and
write.

> 
> Thanks!
> -Lukas
> 
> > 
> > Regards,
> > 
> > 						- 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
> 

-- 

Re: [PATCH 2/2] e2image: add -a switch to include all data

[Ted Ts'o]

On Fri, Feb 17, 2012 at 04:35:20PM +0100, Lukas Czerner wrote:
> as I said I have never tested this. Well, actually I did some very
> limited testing, but since this was not (and still is not right?) the
> scope of e2image

I don't think it's within the scope of e2image to be guaranteed way of
converting a general-purpose qcow2 image file from qemu to a raw image
file.  It would be *nice* if this worked, but I don't think that's the
main thrust of e2image's functionality, no.

I'd consider it more within e2fsprogs's scope if e2fsck, dumpe2fs,
debugfs, etc., could operate directly on a qcow2 file (i.e., if the
qcow2 functionality was implemented as an io_manager).  Combined with
Darrick's fuse2fs, or the e2tools package, it would mean that we could
manipulate qcow2 files directly from the host OS.  I think sysadmins
who are using virtualization would find that to be a useful feature.

I just keep hoping that someone will be able to convince their
management that it's useful enough to put in the development effort.  :-)

	   	     	    	      - Ted

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

[Ted Ts'o]

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

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

[Phillip Susi]

-----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-----

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

[Ted Ts'o]

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

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

[Phillip Susi]

-----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-----

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

[Lukas Czerner]

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
> 

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

[Ted Ts'o]

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

[PATCH 1/2] e2image: fix logic bug which could cause a raw image not to be extended

[Theodore Ts'o]

If the size of the last "hole" in the raw file was an exact multiple
of a megabyte, then we wouldn't write a null at the end of the file in
order to extend the size of the raw image to correspond with the file
system size.  Thanks to Lukas Czerner for suggesting the fix, and
Phillip Susi for pointing out the problem.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 misc/e2image.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/misc/e2image.c b/misc/e2image.c
index d888e5a..3cb92fe 100644
--- a/misc/e2image.c
+++ b/misc/e2image.c
@@ -504,10 +504,9 @@ static void output_meta_data_blocks(ext2_filsys fs, int fd)
 				continue;
 			}
 			sparse += fs->blocksize;
-			if (sparse >= 1024*1024) {

[PATCH 2/2] e2image: attempt to use ftruncate64 to set i_size for raw images

[Theodore Ts'o]

If ftruncate64() exists, try to use it to set i_size.  This isn't
guaranteed to work, per SuSv3, but if it doesn't work, it's guaranteed
to return an error.  So for file systems and/or operating systems that
don't support extending i_size via ftruncate64(), then fall back to
writing the trailing null.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 misc/e2image.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/misc/e2image.c b/misc/e2image.c
index 3cb92fe..90d69ec 100644
--- a/misc/e2image.c
+++ b/misc/e2image.c
@@ -510,6 +510,16 @@ static void output_meta_data_blocks(ext2_filsys fs, int fd)
 			}
 		}
 	}
+#ifdef HAVE_FTRUNCATE64
+	if (sparse) {
+		ext2_loff_t offset = ext2fs_llseek(fd, sparse, SEEK_CUR);
+
+		if (offset > 0 &&
+		    ftruncate64(fd, offset) < 0)
+			write_block(fd, zero_buf, -1, 1, -1);
+		sparse = 0;
+	}
+#endif
 	if (sparse)
 		write_block(fd, zero_buf, sparse-1, 1, -1);
 	ext2fs_free_mem(&zero_buf);
-- 
1.7.9.107.g97f9a

[PATCH 2/2 -v2] e2image: attempt to use ftruncate64 to set i_size for raw images

[Theodore Ts'o]

If ftruncate64() exists, try to use it to set i_size.  This isn't
guaranteed to work, per SuSv3, but if it doesn't work, it's guaranteed
to return an error.  So for file systems and/or operating systems that
don't support extending i_size via ftruncate64(), fall back to writing
the trailing null.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---

Oops, sent the wrong version of the patch.  My bad.

 misc/e2image.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/misc/e2image.c b/misc/e2image.c
index 3cb92fe..93359cf 100644
--- a/misc/e2image.c
+++ b/misc/e2image.c
@@ -510,8 +510,19 @@ static void output_meta_data_blocks(ext2_filsys fs, int fd)
 			}
 		}
 	}
+#ifdef HAVE_FTRUNCATE64
+	if (sparse) {
+		ext2_loff_t offset = ext2fs_llseek(fd, sparse, SEEK_CUR);
+
+		if (offset < 0)
+			lseek_error_and_exit(errno);
+		if (ftruncate64(fd, offset) < 0)
+			write_block(fd, zero_buf, -1, 1, -1);
+	}
+#else
 	if (sparse)
 		write_block(fd, zero_buf, sparse-1, 1, -1);
+#endif
 	ext2fs_free_mem(&zero_buf);
 	ext2fs_free_mem(&buf);
 }
-- 
1.7.9.107.g97f9a

Re: [PATCH 2/2 -v2] e2image: attempt to use ftruncate64 to set i_size for raw images

[Lukas Czerner]

On Fri, 17 Feb 2012, Theodore Ts'o wrote:

> If ftruncate64() exists, try to use it to set i_size.  This isn't
> guaranteed to work, per SuSv3, but if it doesn't work, it's guaranteed
> to return an error.  So for file systems and/or operating systems that
> don't support extending i_size via ftruncate64(), fall back to writing
> the trailing null.

Looks good, thanks.

-Lukas

> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
> 
> Oops, sent the wrong version of the patch.  My bad.
> 
>  misc/e2image.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/misc/e2image.c b/misc/e2image.c
> index 3cb92fe..93359cf 100644
> --- a/misc/e2image.c
> +++ b/misc/e2image.c
> @@ -510,8 +510,19 @@ static void output_meta_data_blocks(ext2_filsys fs, int fd)
>  			}
>  		}
>  	}
> +#ifdef HAVE_FTRUNCATE64
> +	if (sparse) {
> +		ext2_loff_t offset = ext2fs_llseek(fd, sparse, SEEK_CUR);
> +
> +		if (offset < 0)
> +			lseek_error_and_exit(errno);
> +		if (ftruncate64(fd, offset) < 0)
> +			write_block(fd, zero_buf, -1, 1, -1);
> +	}
> +#else
>  	if (sparse)
>  		write_block(fd, zero_buf, sparse-1, 1, -1);
> +#endif
>  	ext2fs_free_mem(&zero_buf);
>  	ext2fs_free_mem(&buf);
>  }
> 

Re: [PATCH 1/2] e2image: fix logic bug which could cause a raw image not to be extended

[Lukas Czerner]

On Fri, 17 Feb 2012, Theodore Ts'o wrote:

> If the size of the last "hole" in the raw file was an exact multiple
> of a megabyte, then we wouldn't write a null at the end of the file in
> order to extend the size of the raw image to correspond with the file
> system size.  Thanks to Lukas Czerner for suggesting the fix, and
> Phillip Susi for pointing out the problem.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  misc/e2image.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/misc/e2image.c b/misc/e2image.c
> index d888e5a..3cb92fe 100644
> --- a/misc/e2image.c
> +++ b/misc/e2image.c
> @@ -504,10 +504,9 @@ static void output_meta_data_blocks(ext2_filsys fs, int fd)
>  				continue;
>  			}
>  			sparse += fs->blocksize;
> -			if (sparse >= 1024*1024) {
> -
> -				write_block(fd, 0, sparse, 0, 0);
> -				sparse = 0;
> +			if (sparse > 1024*1024) {
> +				write_block(fd, 0, 1024*1024, 0, 0);
> +				sparse -= 1024*1024;
>  			}
>  		}
>  	}
> 

Looks good, thanks!

-Lukas

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

[Phillip Susi]

-----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-----

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

[Phillip Susi]

-----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-----