Re: [PATCH 1/4] e2fsprogs: Add undo I/O manager

On Aug 01, 2007  21:04 +0530, Aneesh Kumar K.V wrote:

+static errcode_t write_file_system_identity(io_channel undo_channel,
+							TDB_CONTEXT *tdb)
+{
+	/* Write to tdb file in the file system byte order */
+	tdb_key.dptr = "filesystem MTIME";
+	tdb_key.dsize = sizeof("filesystem MTIME");
+	tdb_data.dptr = (unsigned char *) &(super.s_mtime);
+	tdb_data.dsize = sizeof(super.s_mtime);
+
+	tdb_key.dptr = "filesystem UUID";
+	tdb_key.dsize = sizeof("filesystem UUID");
+	tdb_data.dptr = (unsigned char *)&(super.s_uuid);
+	tdb_data.dsize = sizeof(super.s_uuid);

Is this the mtime and UUID of the new filesystem or the old one?  It
should be the UUID and mtime of the new filesystem, so that the
undo file can be verified against the current superblock.  This poses
a bit of a problem, because that information isn't saved until after
the mke2fs run is finished.

One possibility is to overwrite this information at the end of mke2fs
after the new UUID and mtime are written?

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Andreas Dilger wrote:

On Aug 01, 2007  21:04 +0530, Aneesh Kumar K.V wrote:

quoted

+static errcode_t write_file_system_identity(io_channel undo_channel,
+							TDB_CONTEXT *tdb)
+{
+	/* Write to tdb file in the file system byte order */
+	tdb_key.dptr = "filesystem MTIME";
+	tdb_key.dsize = sizeof("filesystem MTIME");
+	tdb_data.dptr = (unsigned char *) &(super.s_mtime);
+	tdb_data.dsize = sizeof(super.s_mtime);
+
+	tdb_key.dptr = "filesystem UUID";
+	tdb_key.dsize = sizeof("filesystem UUID");
+	tdb_data.dptr = (unsigned char *)&(super.s_uuid);
+	tdb_data.dsize = sizeof(super.s_uuid);

Is this the mtime and UUID of the new filesystem or the old one?  It
should be the UUID and mtime of the new filesystem, so that the
undo file can be verified against the current superblock.  This poses
a bit of a problem, because that information isn't saved until after
the mke2fs run is finished.

One possibility is to overwrite this information at the end of mke2fs
after the new UUID and mtime are written?

This can be done by writing the file system identity during the the io_channel_close.
How about this patch on top of the last series. I will merge this into the patcheset

diff --git a/lib/ext2fs/undo_io.c b/lib/ext2fs/undo_io.c
index 30e2514..a80bafc 100644
--- a/lib/ext2fs/undo_io.c
+++ b/lib/ext2fs/undo_io.c

@@ -159,8 +159,8 @@ static errcode_t write_file_system_identity(io_channel undo_channel,
 	/* Also store the block size */
 	tdb_key.dptr = "filesystem BLKSIZE";
 	tdb_key.dsize = sizeof("filesystem BLKSIZE");
-	tdb_data.dptr = (unsigned char *)&(undo_channel->block_size);
-	tdb_data.dsize = sizeof(undo_channel->block_size);
+	tdb_data.dptr = (unsigned char *)&(tdb_data_size);
+	tdb_data.dsize = sizeof(tdb_data_size);
 
 	retval = tdb_store(tdb, tdb_key, tdb_data, TDB_INSERT);
 	if (retval == -1) {

@@ -199,13 +199,6 @@ static errcode_t undo_write_tdb(io_channel channel,
 	 */
 	if (!tdb_data_size) {
 		tdb_data_size = channel->block_size;
-
-		/*
-		 * First write. Write the file system identity
-		 */
-		retval = write_file_system_identity(channel, data->tdb);
-		if (retval)
-			return retval;
 	}
 
 	if (count == 1)

@@ -405,6 +398,11 @@ static errcode_t undo_close(io_channel channel)
 	if (--channel->refcount > 0)
 		return 0;
 
+	/* Before closing Write the file system identity */
+	retval = write_file_system_identity(channel, data->tdb);
+	if (retval)
+		return retval;
+
 	if (data->real)
 		retval = io_channel_close(data->real);
 

On Aug 03, 2007  00:02 +0530, Aneesh Kumar K.V wrote:

Andreas Dilger wrote:

quoted

Is this the mtime and UUID of the new filesystem or the old one?  It
should be the UUID and mtime of the new filesystem, so that the
undo file can be verified against the current superblock.  This poses
a bit of a problem, because that information isn't saved until after
the mke2fs run is finished.

One possibility is to overwrite this information at the end of mke2fs
after the new UUID and mtime are written?

This can be done by writing the file system identity during the the 
io_channel_close.
How about this patch on top of the last series. I will merge this into the 
patcheset

I thought about this also, but in fact for most uses of the undo manager
we want to save the information at the start instead of the end, so it
is possible to undo e.g. a partial e2fsck that crashes before it finishes.
Only with mke2fs (and, I guess tune2fs -U) does the UUID change at the
end.

Also, can you check if mke2fs does any non-iomanager output?  I think
there is code to "zap" the old superblock at the start and old RAID info
at the end of the block device, and I'm not sure if this uses the normal
IO manager or not.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Andreas Dilger wrote:

On Aug 03, 2007  00:02 +0530, Aneesh Kumar K.V wrote:

quoted

Andreas Dilger wrote:

quoted

Is this the mtime and UUID of the new filesystem or the old one?  It
should be the UUID and mtime of the new filesystem, so that the
undo file can be verified against the current superblock.  This poses
a bit of a problem, because that information isn't saved until after
the mke2fs run is finished.

One possibility is to overwrite this information at the end of mke2fs
after the new UUID and mtime are written?

This can be done by writing the file system identity during the the 
io_channel_close.
How about this patch on top of the last series. I will merge this into the 
patcheset

I thought about this also, but in fact for most uses of the undo manager
we want to save the information at the start instead of the end, so it
is possible to undo e.g. a partial e2fsck that crashes before it finishes.
Only with mke2fs (and, I guess tune2fs -U) does the UUID change at the
end.

I am not sure whether saving the information at start is needed. I understand
that what we are looking for is the case when the application crashes without
doing a io_channel_close. In that case i would say the user can use the
--force option and replay the data from the tdb file. The UUID could very well
be changed on the disk before the application crashed. So even if we save
UUID at the start, there are cases where it won't match with the disk UUID.


That actually brings me to another change. I would be moving the block size
recording changes from write_file_system_identity to a separate function 
and will be calling it at the first write. That make sure we have a record
that carry the blocksize even though we don't have one with mtime and UUID
in the tdb file.

Also, can you check if mke2fs does any non-iomanager output?  I think
there is code to "zap" the old superblock at the start and old RAID info
at the end of the block device, and I'm not sure if this uses the normal
IO manager or not.

The zap_sector and zap_zero uses the io manager to zero out the blocks. So
they should be ok. I found that when we use -J device=<journal-device>. mke2fs
uses unix I/O manager to write to the journal super block. I guess that is ok
because we are not tracking changes to journal device.

I found that the journal_super_block have only space for 48 s_users
UUID entries. But in ext2fs_add_journal_device we are not checking
the limit. Does that mean repeated mke2fs with -J can lead to corruption ?

-aneesh 

On Aug 03, 2007  10:19 +0530, Aneesh Kumar K.V wrote:

Andreas Dilger wrote:

quoted

I thought about this also, but in fact for most uses of the undo manager
we want to save the information at the start instead of the end, so it
is possible to undo e.g. a partial e2fsck that crashes before it finishes.
Only with mke2fs (and, I guess tune2fs -U) does the UUID change at the
end.

I am not sure whether saving the information at start is needed. I 
understand that what we are looking for is the case when the application
crashes without doing a io_channel_close. In that case i would say the
user can use the --force option and replay the data from the tdb file.
The UUID could very well be changed on the disk before the application
crashed. So even if we save UUID at the start, there are cases where it
won't match with the disk UUID.

While this is true, I don't think it is harmful to save the UUID at the
start.  The UUID changing is the rare case, so to make this safer saving
the UUID at the start and the end is best.

That actually brings me to another change. I would be moving the block size
recording changes from write_file_system_identity to a separate function 
and will be calling it at the first write.

Definitely, yes.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.