[Ocfs2-tools-devel] Re: New patch for backup superblock against r1273 of ocfs2-tools

[Sunil Mushran]

tao.ma wrote:
> But I disagree with your order of writing backup sb. We may need to 
> reset the bits in cluster bitmap if the write of backup sbs fails.
> So I think the order is:
> ==> First initialize all the cluster(s) with zeroes.
> ==> Then write the backup sbs.
> ==> Then set/write the bits in the cluster bitmap.
>  If we meet some error in the writing of backup sbs, we don't need to 
> reset the bits and it is harmless for us to write some data to an 
> unallocated clusters.
Ok that makes sense.

My comments and two patches are attached. I have not yet reviewed
the fsck patch. Will get to that later.

Meanwhile we are missing the mkfs.ocfs2 -n option. But we could make that
a separate task apart from this.

Also, you have to add a test in ocfs2-test repo. You cannot escape that.
The test should accept a device as a param and test mkfs, tune and fsck.

1. mkfs:
make the volume smaller than 1G, 1G and larger than 1G. Use debugfs
to ensure that the backup super was created (or not). Do the same around
each backup super location till the size of the volume.

2. tunefs:
make a volume under 1G. Then resize to 1G+ and see if tunefs also adds
the backup super. Do the same at each backup super location till volume 
size.

3. tunefs:
Check if just updating the label, node slots, etc on a volume having 
backup sb
set, the backup supers are updated.

This is just for starters.
-------------- next part --------------
1. ocfs2_set_backup_sb():
+       ret = ocfs2_malloc_block(fs->fs_io, &buf);
+       if (ret)
+               goto bail;
+
+       memset(buf, 0, fs->fs_blocksize);
+
+       for (i = 0; i < len; i++, blkno++) {
+               if (!*blkno)
+                       break;
+
+               /* zero the whole cluster first */
+               for (j = 0; j < bpc; j++) {
+                       ret = io_write_block(fs->fs_io, *blkno + j, 1, buf);
+                       if (ret)
+                               goto bail;
+               }
How about allocating fs->fs_clustersize and initializing in one chunk.

The other bug here is that it assumes the cluster start matches the block start.
Are you sure about that? It would be better if we generated a list of cluster offsets
for all block offsets and then explicitly verified those cluster offsets in
is_cluster_unused() rather than the current verify_block_allocation().

2. ocfs2_set_backup_sb():
+       if (fs->fs_cluster_alloc) {
+               ocfs2_free_cached_inode(fs, fs->fs_cluster_alloc);
+               fs->fs_cluster_alloc = NULL;
+       }
Is this necessary?

3. In format_backup_sb(): 
+       for (i = 0; i < len && blocks[i]; i++)
+               printf(" %"PRIu64" ", blocks[i] * fs->fs_blocksize);
We cannot have this as we have to honor the quiet mode.

Check backup_mkfs_1a.patch for more.

4. tunefs:
+               { "backup-sb", 0, 0, 'b'},
Rename "backup-sb" to "backup-super" and don't waste a single letter option
for this. This option will only be useful only for a short while as this
feature will be default going forward.

5. tunefs: In main():
	/* If operation requires touching the global bitmap, ensure it is good */
	/* This is to handle failed resize */
	if (opts.num_blocks || opts.num_slots || opts.jrnl_size) {
		if (global_bitmap_check(fs)) {
			com_err(opts.progname, 0, "Global bitmap check failed. "
				"Run fsck.ocfs2 -f <device>.");
			goto unlock;
		}
	}
As backup-super also involves the global bitmap, it should be included.

6. tunefs: In get_options() ==>
Disallow backup-super with other options. As in, the user cannot resize the volume
or add slots alongwith backup-super. This is a one time operation. Keeps things simple.
(The code currently has a bug that it verifies for backup sb before adding the
node slots. This is a problem as adding a node slot consumes global bitmap space.
By disallowing this, we avoid this problem.)

7. tunefs:
The superblock update code is too convoluted. Check backup_tunefs_1a.patch.
While I have not tested the code, especially not the edge conditions, the flow
should be fine.

8. debugfs:
+       g_print ("usage: %s [-f cmdfile] [-R request] [-s block] [-V] [-w] [-n] [-?] [device]\n", progname);
+       g_print ("\t-s, --superblock <num>\tOpen the device using another superblock\n");
+       printf ("open <device> [-s num]\t\t\t\tOpen a device\n");
Inconsistent option display. Change "num" and "block" to "backup#".

9. debugfs:
+               if (opts.sb_num)
+                       line = g_strdup_printf ("open %s -s %d", opts.device, opts.sb_num);
+               else
+                       line = g_strdup_printf ("open %s", opts.device);
Cute.

10. debugfs:
+       /* iterate all the blocksize and get the right one. */
+       for (blksize = OCFS2_MIN_BLOCKSIZE;
+               blksize <= OCFS2_MAX_BLOCKSIZE; blksize <<= 1) {
+               sb = byte_off[s-1] / blksize;
+               ret = ocfs2_open(args[1], OCFS2_FLAG_RO, sb, blksize, &fs);
+               if (!ret)
+                       break;
+       }
Why? Why not just io_read_block(4k) at the offset and read the blocksize off the sb.

11. debugfs:
+       uint64_t superblock, block_size;
Initialize both to zeroes.

12. debugfs:
+       s = strtol(args[ind], &ptr, 0);
Make it strtoul and make other changes approriately.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: backup_mkfs_1a.patch
Type: text/x-patch
Size: 1495 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-tools-devel/attachments/20061207/82694c4d/backup_mkfs_1a.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: backup_tunefs_1a.patch
Type: text/x-patch
Size: 2875 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-tools-devel/attachments/20061207/82694c4d/backup_tunefs_1a.bin