REVIEW: Write primary superblock info to ALL secondaries during mkfs

REVIEW: Write primary superblock info to ALL secondaries during mkfs

[Barry Naujok]

Secondaries should contain redundant information from the primary
superblock. It does this for the filesystem geometry information,
but not inode values (rootino, rt inos, quota inos).

This patch updates all the secondaries from the primary just before
it marks the filesystem as good to go.

Unfortunately, this also affects the output of xfs_repair during
QA 030 and 178 which restores the primary superblock from the
secondaries.

Now that the secondaries have valid inode values, xfs_repair
does not have to restore them to the correct values after copying
the secondary into the primary.

Attached is the mkfs.xfs patch and also the updated golden
outputs for QA 030 and 178.

The next step after this is to enhance xfs_repair to be more
thorough in checking the secondaries during Phase 1.

--

Index: ci/xfsprogs/mkfs/xfs_mkfs.c
===================================================================
--- ci.orig/xfsprogs/mkfs/xfs_mkfs.c	2008-03-25 13:30:53.000000000 +1100
+++ ci/xfsprogs/mkfs/xfs_mkfs.c	2008-03-25 16:29:44.811095380 +1100
@@ -2397,48 +2397,32 @@
  	}

  	/*
-	 * Write out multiple secondary superblocks with rootinode field set
+	 * Write out secondary superblocks with inode fields set
  	 */
-	if (mp->m_sb.sb_agcount > 1) {
-		/*
-		 * the last superblock
-		 */
-		buf = libxfs_readbuf(mp->m_dev,
-				XFS_AGB_TO_DADDR(mp, mp->m_sb.sb_agcount-1,
-					XFS_SB_DADDR),
-				XFS_FSS_TO_BB(mp, 1),
-				LIBXFS_EXIT_ON_FAILURE);
-		INT_SET((XFS_BUF_TO_SBP(buf))->sb_rootino,
-				ARCH_CONVERT, mp->m_sb.sb_rootino);
-		libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
-		/*
-		 * and one in the middle for luck
-		 */
-		if (mp->m_sb.sb_agcount > 2) {
-			buf = libxfs_readbuf(mp->m_dev,
-				XFS_AGB_TO_DADDR(mp, (mp->m_sb.sb_agcount-1)/2,
-					XFS_SB_DADDR),
-				XFS_FSS_TO_BB(mp, 1),
-				LIBXFS_EXIT_ON_FAILURE);
-			INT_SET((XFS_BUF_TO_SBP(buf))->sb_rootino,
-				ARCH_CONVERT, mp->m_sb.sb_rootino);
-			libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
-		}
+	buf = libxfs_getsb(mp, LIBXFS_EXIT_ON_FAILURE);
+	XFS_BUF_TO_SBP(buf)->sb_inprogress = 0;
+
+	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
+		xfs_buf_t	*sbuf;
+
+		sbuf = libxfs_getbuf(mp->m_dev,
+				XFS_AGB_TO_DADDR(mp, agno, XFS_SB_DADDR),
+				XFS_FSS_TO_BB(mp, 1));
+		memcpy(XFS_BUF_PTR(sbuf), XFS_BUF_PTR(buf),
+				XFS_BUF_SIZE(sbuf));
+		libxfs_writebuf(sbuf, LIBXFS_EXIT_ON_FAILURE);
  	}

  	/*
-	 * Dump all inodes and buffers before marking us all done.
-	 * Need to drop references to inodes we still hold, first.
+	 * Flush out all inodes and buffers before marking us all done.
  	 */
  	libxfs_rtmount_destroy(mp);
  	libxfs_icache_purge();
-	libxfs_bcache_purge();
+	libxfs_bcache_flush();

  	/*
-	 * Mark the filesystem ok.
+	 * Finalize the filesystem (sb_inprogress = 0 from above).
  	 */
-	buf = libxfs_getsb(mp, LIBXFS_EXIT_ON_FAILURE);
-	(XFS_BUF_TO_SBP(buf))->sb_inprogress = 0;
  	libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);

  	libxfs_umount(mp);
Index: ci/xfstests/030.out.linux
===================================================================
--- ci.orig/xfstests/030.out.linux	2007-10-10 16:12:52.000000000 +1000
+++ ci/xfstests/030.out.linux	2008-03-25 16:30:54.926056313 +1100
@@ -14,12 +14,6 @@
  found candidate secondary superblock...
  verified secondary superblock...
  writing modified primary superblock
-sb root inode value INO inconsistent with calculated value INO
-resetting superblock root inode pointer to INO
-sb realtime bitmap inode INO inconsistent with calculated value INO
-resetting superblock realtime bitmap ino pointer to INO
-sb realtime summary inode INO inconsistent with calculated value INO
-resetting superblock realtime summary ino pointer to INO
  Phase 2 - using <TYPEOF> log
          - zero log...
          - scan filesystem freespace and inode maps...
@@ -132,12 +126,6 @@
  found candidate secondary superblock...
  verified secondary superblock...
  writing modified primary superblock
-sb root inode value INO inconsistent with calculated value INO
-resetting superblock root inode pointer to INO
-sb realtime bitmap inode INO inconsistent with calculated value INO
-resetting superblock realtime bitmap ino pointer to INO
-sb realtime summary inode INO inconsistent with calculated value INO
-resetting superblock realtime summary ino pointer to INO
  Phase 2 - using <TYPEOF> log
          - zero log...
          - scan filesystem freespace and inode maps...
Index: ci/xfstests/178.out
===================================================================
--- ci.orig/xfstests/178.out	2007-10-10 16:12:56.000000000 +1000
+++ ci/xfstests/178.out	2008-03-25 16:31:09.944120144 +1100
@@ -12,12 +12,6 @@
  found candidate secondary superblock...
  verified secondary superblock...
  writing modified primary superblock
-sb root inode value INO inconsistent with calculated value INO
-resetting superblock root inode pointer to INO
-sb realtime bitmap inode INO inconsistent with calculated value INO
-resetting superblock realtime bitmap ino pointer to INO
-sb realtime summary inode INO inconsistent with calculated value INO
-resetting superblock realtime summary ino pointer to INO
  Phase 2 - using <TYPEOF> log
          - zero log...
          - scan filesystem freespace and inode maps...
@@ -48,12 +42,6 @@
  found candidate secondary superblock...
  verified secondary superblock...
  writing modified primary superblock
-sb root inode value INO inconsistent with calculated value INO
-resetting superblock root inode pointer to INO
-sb realtime bitmap inode INO inconsistent with calculated value INO
-resetting superblock realtime bitmap ino pointer to INO
-sb realtime summary inode INO inconsistent with calculated value INO
-resetting superblock realtime summary ino pointer to INO
  Phase 2 - using <TYPEOF> log
          - zero log...
          - scan filesystem freespace and inode maps...

Re: REVIEW: Write primary superblock info to ALL secondaries during mkfs

[David Chinner]

On Tue, Mar 25, 2008 at 04:39:02PM +1100, Barry Naujok wrote:
> Secondaries should contain redundant information from the primary
> superblock. It does this for the filesystem geometry information,
> but not inode values (rootino, rt inos, quota inos).
> 
> This patch updates all the secondaries from the primary just before
> it marks the filesystem as good to go.

So it's got all the inodes, geometry, etc correct in them?

So what about the fact that the kernel code doesn't keep all
copies up to date? e.g. growfs will only write new values into
a handful of superblocks, changing sunit/swidth via mount options
only change the primary, etc....

If you are going to change mkfs to keep them all up to date, the
kernel code really needs to do the same thing....

> Unfortunately, this also affects the output of xfs_repair during
> QA 030 and 178 which restores the primary superblock from the
> secondaries.

So do a version check and have a different golden output for the
new version....

> Index: ci/xfsprogs/mkfs/xfs_mkfs.c
> ===================================================================
> --- ci.orig/xfsprogs/mkfs/xfs_mkfs.c	2008-03-25 13:30:53.000000000 +1100
> +++ ci/xfsprogs/mkfs/xfs_mkfs.c	2008-03-25 16:29:44.811095380 +1100
> @@ -2397,48 +2397,32 @@
>  	}
> 
>  	/*
> -	 * Write out multiple secondary superblocks with rootinode field set
> +	 * Write out secondary superblocks with inode fields set
>  	 */
> -	if (mp->m_sb.sb_agcount > 1) {
> -		/*
> -		 * the last superblock
> -		 */
> -		buf = libxfs_readbuf(mp->m_dev,
> -				XFS_AGB_TO_DADDR(mp, mp->m_sb.sb_agcount-1,
> -					XFS_SB_DADDR),
> -				XFS_FSS_TO_BB(mp, 1),
> -				LIBXFS_EXIT_ON_FAILURE);
> -		INT_SET((XFS_BUF_TO_SBP(buf))->sb_rootino,
> -				ARCH_CONVERT, mp->m_sb.sb_rootino);
> -		libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
> -		/*
> -		 * and one in the middle for luck
> -		 */
> -		if (mp->m_sb.sb_agcount > 2) {
> -			buf = libxfs_readbuf(mp->m_dev,
> -				XFS_AGB_TO_DADDR(mp, 
> (mp->m_sb.sb_agcount-1)/2,
> -					XFS_SB_DADDR),
> -				XFS_FSS_TO_BB(mp, 1),
> -				LIBXFS_EXIT_ON_FAILURE);
> -			INT_SET((XFS_BUF_TO_SBP(buf))->sb_rootino,
> -				ARCH_CONVERT, mp->m_sb.sb_rootino);
> -			libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);
> -		}
> +	buf = libxfs_getsb(mp, LIBXFS_EXIT_ON_FAILURE);
> +	XFS_BUF_TO_SBP(buf)->sb_inprogress = 0;
> +
> +	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
> +		xfs_buf_t	*sbuf;

sbuf is a bad name. I immediately think "source buffer", when in fact
it's the destination buffer.

> +
> +		sbuf = libxfs_getbuf(mp->m_dev,
> +				XFS_AGB_TO_DADDR(mp, agno, XFS_SB_DADDR),
> +				XFS_FSS_TO_BB(mp, 1));
> +		memcpy(XFS_BUF_PTR(sbuf), XFS_BUF_PTR(buf),
> +				XFS_BUF_SIZE(sbuf));
> +		libxfs_writebuf(sbuf, LIBXFS_EXIT_ON_FAILURE);
>  	}
> 
>  	/*
> -	 * Dump all inodes and buffers before marking us all done.
> -	 * Need to drop references to inodes we still hold, first.
> +	 * Flush out all inodes and buffers before marking us all done.
>  	 */
>  	libxfs_rtmount_destroy(mp);
>  	libxfs_icache_purge();
> -	libxfs_bcache_purge();
> +	libxfs_bcache_flush();

Don't you still need a purge there to free all the objects in the
cache?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: REVIEW: Write primary superblock info to ALL secondaries during mkfs

[Barry Naujok]

On Tue, 25 Mar 2008 17:00:17 +1100, David Chinner <dgc@sgi.com> wrote:

> On Tue, Mar 25, 2008 at 04:39:02PM +1100, Barry Naujok wrote:
>> Secondaries should contain redundant information from the primary
>> superblock. It does this for the filesystem geometry information,
>> but not inode values (rootino, rt inos, quota inos).
>>
>> This patch updates all the secondaries from the primary just before
>> it marks the filesystem as good to go.
>
> So it's got all the inodes, geometry, etc correct in them?
>
> So what about the fact that the kernel code doesn't keep all
> copies up to date? e.g. growfs will only write new values into
> a handful of superblocks, changing sunit/swidth via mount options
> only change the primary, etc....
>
> If you are going to change mkfs to keep them all up to date, the
> kernel code really needs to do the same thing....

Yes it should :) Geometry information was already done across all
the AGs.

>>
>>  	/*
>> -	 * Dump all inodes and buffers before marking us all done.
>> -	 * Need to drop references to inodes we still hold, first.
>> +	 * Flush out all inodes and buffers before marking us all done.
>>  	 */
>>  	libxfs_rtmount_destroy(mp);
>>  	libxfs_icache_purge();
>> -	libxfs_bcache_purge();
>> +	libxfs_bcache_flush();
>
> Don't you still need a purge there to free all the objects in the
> cache?

No, the flush does what is required. There is no libxfs_icache_flush
at the moment, so I left the purge there for that.

libxfs_umount later on does a libxfs_*_purge() anyway.

The main thing is to make sure all objects are written to disk before
the sb_inprogress field in the primary superblock is zeroed.

Barry.

Re: REVIEW: Write primary superblock info to ALL secondaries during mkfs

[Eric Sandeen]

Barry Naujok wrote:
> Secondaries should contain redundant information from the primary
> superblock. It does this for the filesystem geometry information,
> but not inode values (rootino, rt inos, quota inos).
> 
> This patch updates all the secondaries from the primary just before
> it marks the filesystem as good to go.
> 
> Unfortunately, this also affects the output of xfs_repair during
> QA 030 and 178 which restores the primary superblock from the
> secondaries.
> 
> Now that the secondaries have valid inode values, xfs_repair
> does not have to restore them to the correct values after copying
> the secondary into the primary.
> 
> Attached is the mkfs.xfs patch and also the updated golden
> outputs for QA 030 and 178.
> 
> The next step after this is to enhance xfs_repair to be more
> thorough in checking the secondaries during Phase 1.

One related thing I'd always wondered about was stamping a secondary at
the very end of the device (and therefore shrinking the fs by just a
bit) - repair could then do a quick check at the end of the device
before resorting to scanning for the 2nd backup... would this make any
sense?

-Eric

Re: REVIEW: Write primary superblock info to ALL secondaries during mkfs

[Mark Goodwin]

Eric Sandeen wrote:
> Barry Naujok wrote:
>> Secondaries should contain redundant information from the primary
>> superblock. It does this for the filesystem geometry information,
>> but not inode values (rootino, rt inos, quota inos).
>>
>> This patch updates all the secondaries from the primary just before
>> it marks the filesystem as good to go.
>>
>> Unfortunately, this also affects the output of xfs_repair during
>> QA 030 and 178 which restores the primary superblock from the
>> secondaries.
>>
>> Now that the secondaries have valid inode values, xfs_repair
>> does not have to restore them to the correct values after copying
>> the secondary into the primary.
>>
>> Attached is the mkfs.xfs patch and also the updated golden
>> outputs for QA 030 and 178.
>>
>> The next step after this is to enhance xfs_repair to be more
>> thorough in checking the secondaries during Phase 1.
> 
> One related thing I'd always wondered about was stamping a secondary at
> the very end of the device (and therefore shrinking the fs by just a
> bit) - repair could then do a quick check at the end of the device
> before resorting to scanning for the 2nd backup... would this make any
> sense?

I guess it might, Barry what do you think? Probably makes grow a bit
more complicated. What would repair do if it doesn't find the backup
SB at the end of the device? We'd need a new SB flag to indicate it's
supposed to be there, which seems a bit chicken-and-egg'ish ...

Cheers

> 
> -Eric
> 
> 

-- 

  Mark Goodwin                                  markgw@sgi.com
  Engineering Manager for XFS and PCP    Phone: +61-3-99631937
  SGI Australian Software Group           Cell: +61-4-18969583
-------------------------------------------------------------

Re: REVIEW: Write primary superblock info to ALL secondaries during mkfs

[Eric Sandeen]

Mark Goodwin wrote:
> 
> Eric Sandeen wrote:

>> One related thing I'd always wondered about was stamping a secondary at
>> the very end of the device (and therefore shrinking the fs by just a
>> bit) - repair could then do a quick check at the end of the device
>> before resorting to scanning for the 2nd backup... would this make any
>> sense?
> 
> I guess it might, Barry what do you think? Probably makes grow a bit
> more complicated. What would repair do if it doesn't find the backup
> SB at the end of the device? We'd need a new SB flag to indicate it's
> supposed to be there, which seems a bit chicken-and-egg'ish ...

If not found at the end, just go back to the original search scheme, I'd
say...

-Eric