[REVIEW] User-space support for bad_features2 patch

[REVIEW] User-space support for bad_features2 patch

[Barry Naujok]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

The attached patch fixes mkfs.xfs writing the bad features2 in the first  
place (the change to xfs_sb.h does this).

Next xfs_db support printing of this superblock field and xfs_check can  
report the bad_features2 field is set.

xfs_repair can correct the error in the same fashion that David Chinner's  
mount code does it.

This patch applies to the lazy-count xfs_repair conversion code that I  
posted a short time before this patch.

Barry.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bad_features2.patch --]
[-- Type: text/x-patch; name=bad_features2.patch, Size: 3335 bytes --]

Index: ci/xfsprogs/db/check.c
===================================================================
--- ci.orig/xfsprogs/db/check.c	2008-02-22 14:15:42.000000000 +1100
+++ ci/xfsprogs/db/check.c	2008-02-22 18:43:56.698852255 +1100
@@ -869,6 +869,12 @@
 				mp->m_sb.sb_frextents, frextents);
 		error++;
 	}
+	if (mp->m_sb.sb_bad_features2 != 0) {
+		if (!sflag)
+			dbprintf("sb_bad_features2 is non-zero (%x)\n",
+				mp->m_sb.sb_bad_features2);
+		error++;
+	}
 	if ((sbversion & XFS_SB_VERSION_ATTRBIT) &&
 	    !XFS_SB_VERSION_HASATTR(&mp->m_sb)) {
 		if (!sflag)
Index: ci/xfsprogs/db/sb.c
===================================================================
--- ci.orig/xfsprogs/db/sb.c	2008-02-22 14:15:42.000000000 +1100
+++ ci/xfsprogs/db/sb.c	2008-02-22 18:30:10.742626136 +1100
@@ -108,6 +108,7 @@
 	{ "logsectsize", FLDT_UINT16D, OI(OFF(logsectsize)), C1, 0, TYP_NONE },
 	{ "logsunit", FLDT_UINT32D, OI(OFF(logsunit)), C1, 0, TYP_NONE },
 	{ "features2", FLDT_UINT32X, OI(OFF(features2)), C1, 0, TYP_NONE },
+	{ "bad_features2", FLDT_UINT32X, OI(OFF(bad_features2)), C1, 0, TYP_NONE },
 	{ NULL }
 };
 
Index: ci/xfsprogs/include/xfs_sb.h
===================================================================
--- ci.orig/xfsprogs/include/xfs_sb.h	2008-02-22 14:15:42.000000000 +1100
+++ ci/xfsprogs/include/xfs_sb.h	2008-02-22 18:11:58.816553959 +1100
@@ -151,6 +151,7 @@
 	__uint16_t	sb_logsectsize;	/* sector size for the log, bytes */
 	__uint32_t	sb_logsunit;	/* stripe unit size for the log */
 	__uint32_t	sb_features2;	/* additional feature bits */
+	__uint32_t	sb_bad_features2; /* unusable space */
 } xfs_sb_t;
 
 /*
@@ -169,7 +170,7 @@
 	XFS_SBS_GQUOTINO, XFS_SBS_QFLAGS, XFS_SBS_FLAGS, XFS_SBS_SHARED_VN,
 	XFS_SBS_INOALIGNMT, XFS_SBS_UNIT, XFS_SBS_WIDTH, XFS_SBS_DIRBLKLOG,
 	XFS_SBS_LOGSECTLOG, XFS_SBS_LOGSECTSIZE, XFS_SBS_LOGSUNIT,
-	XFS_SBS_FEATURES2,
+	XFS_SBS_FEATURES2, XFS_SBS_BAD_FEATURES2,
 	XFS_SBS_FIELDCOUNT
 } xfs_sb_field_t;
 
Index: ci/xfsprogs/libxfs/xfs_mount.c
===================================================================
--- ci.orig/xfsprogs/libxfs/xfs_mount.c	2008-02-22 14:15:42.000000000 +1100
+++ ci/xfsprogs/libxfs/xfs_mount.c	2008-02-22 18:22:00.330489434 +1100
@@ -140,6 +140,7 @@
     { offsetof(xfs_sb_t, sb_logsectsize),0 },
     { offsetof(xfs_sb_t, sb_logsunit),	 0 },
     { offsetof(xfs_sb_t, sb_features2),	 0 },
+    { offsetof(xfs_sb_t, sb_bad_features2), 0 },
     { sizeof(xfs_sb_t),			 0 }
 };
 
Index: ci/xfsprogs/repair/phase1.c
===================================================================
--- ci.orig/xfsprogs/repair/phase1.c	2008-02-22 17:31:01.000000000 +1100
+++ ci/xfsprogs/repair/phase1.c	2008-02-22 18:44:23.347411378 +1100
@@ -92,6 +92,19 @@
 	}
 
 	/*
+	 * Check bad_features2, if set and features2 is zero, copy
+	 * bad_features2 to features2 and zero bad_features2.
+	 */
+	if (sb->sb_bad_features2 != 0) {
+		if (sb->sb_features2 == 0)
+			sb->sb_features2 = sb->sb_bad_features2;
+		sb->sb_bad_features2 = 0;
+		primary_sb_modified = 1;
+		do_warn(_("superblock's features2 field is in the wrong "
+			"location, correcting\n"));
+	}
+
+	/*
 	 * apply any version changes or conversions after the primary
 	 * superblock has been verified or repaired
 	 */

Re: [REVIEW] User-space support for bad_features2 patch

[Eric Sandeen]

Barry Naujok wrote:
> The attached patch fixes mkfs.xfs writing the bad features2 in the first  
> place (the change to xfs_sb.h does this).
> 
> Next xfs_db support printing of this superblock field and xfs_check can  
> report the bad_features2 field is set.
> 
> xfs_repair can correct the error in the same fashion that David Chinner's  
> mount code does it.
> 
> This patch applies to the lazy-count xfs_repair conversion code that I  
> posted a short time before this patch.
> 
> Barry.
> 
+	 * Check bad_features2, if set and features2 is zero, copy
+	 * bad_features2 to features2 and zero bad_features2.
+	 */
+	if (sb->sb_bad_features2 != 0) {
+		if (sb->sb_features2 == 0)
+			sb->sb_features2 = sb->sb_bad_features2;
+		sb->sb_bad_features2 = 0;
+		primary_sb_modified = 1;
+		do_warn(_("superblock's features2 field is in the wrong "
+			"location, correcting\n"));
+	}

My only thought here is that if you repair it, then use an older kernel
w/o the fix, suddenly your fs behavior changes, whereas before you often
got lucky, and both userspace & kernelspace swapped the same way, and
you found the bits you were looking for out of luck :)  (same goes for
the recent kernel fix too, I guess)

Should we flag the bad_features2 as "already copied to features2" but
otherwise leave it?  Hmm I guess that would look like an unsupported
feature flag to old kernels, wouldn't it.  Urgh.

-Eric

Re: [REVIEW] User-space support for bad_features2 patch

[Barry Naujok]

On Sat, 23 Feb 2008 04:41:55 +1100, Eric Sandeen <sandeen@sandeen.net>  
wrote:

> Barry Naujok wrote:
>> The attached patch fixes mkfs.xfs writing the bad features2 in the first
>> place (the change to xfs_sb.h does this).
>>
>> Next xfs_db support printing of this superblock field and xfs_check can
>> report the bad_features2 field is set.
>>
>> xfs_repair can correct the error in the same fashion that David  
>> Chinner's
>> mount code does it.
>>
>> This patch applies to the lazy-count xfs_repair conversion code that I
>> posted a short time before this patch.
>>
>> Barry.
>>
> +	 * Check bad_features2, if set and features2 is zero, copy
> +	 * bad_features2 to features2 and zero bad_features2.
> +	 */
> +	if (sb->sb_bad_features2 != 0) {
> +		if (sb->sb_features2 == 0)
> +			sb->sb_features2 = sb->sb_bad_features2;
> +		sb->sb_bad_features2 = 0;
> +		primary_sb_modified = 1;
> +		do_warn(_("superblock's features2 field is in the wrong "
> +			"location, correcting\n"));
> +	}
>
> My only thought here is that if you repair it, then use an older kernel
> w/o the fix, suddenly your fs behavior changes, whereas before you often
> got lucky, and both userspace & kernelspace swapped the same way, and
> you found the bits you were looking for out of luck :)  (same goes for
> the recent kernel fix too, I guess)

I believe the kernel code never tried to access "bad_features2" part
of the superblock, it always did the correct thing (correct me if I'm
wrong of course :).

But, as Dave pointed out, I should be OR'ing the two together before
zeroing the bad one.

> Should we flag the bad_features2 as "already copied to features2" but
> otherwise leave it?  Hmm I guess that would look like an unsupported
> feature flag to old kernels, wouldn't it.  Urgh.
>
> -Eric
>
>

Re: [REVIEW] User-space support for bad_features2 patch

[Eric Sandeen]

Barry Naujok wrote:
> On Sat, 23 Feb 2008 04:41:55 +1100, Eric Sandeen <sandeen@sandeen.net>  
> wrote:

...

>> My only thought here is that if you repair it, then use an older kernel
>> w/o the fix, suddenly your fs behavior changes, whereas before you often
>> got lucky, and both userspace & kernelspace swapped the same way, and
>> you found the bits you were looking for out of luck :)  (same goes for
>> the recent kernel fix too, I guess)
> 
> I believe the kernel code never tried to access "bad_features2" part
> of the superblock, it always did the correct thing (correct me if I'm
> wrong of course :).

I'm fairly sure that it did; both userspace & kernelspace were doing the
same thing, and endian-flipping "too much" ... but I'd have to test
again to be sure.

-Eric

Re: [REVIEW] User-space support for bad_features2 patch

[Jan Derfinak]

On Mon, 25 Feb 2008, Eric Sandeen wrote:

> Barry Naujok wrote:
> > On Sat, 23 Feb 2008 04:41:55 +1100, Eric Sandeen <sandeen@sandeen.net>  
> > wrote:
> 
> ...
> 
> >> My only thought here is that if you repair it, then use an older kernel
> >> w/o the fix, suddenly your fs behavior changes, whereas before you often
> >> got lucky, and both userspace & kernelspace swapped the same way, and
> >> you found the bits you were looking for out of luck :)  (same goes for
> >> the recent kernel fix too, I guess)
> > 
> > I believe the kernel code never tried to access "bad_features2" part
> > of the superblock, it always did the correct thing (correct me if I'm
> > wrong of course :).
> 
> I'm fairly sure that it did; both userspace & kernelspace were doing the
> same thing, and endian-flipping "too much" ... but I'd have to test
> again to be sure.

In my case (x86_64, both kernel and userspace 64-bit), kernel accessed the
right place (features2), but userspace (mkfs.xfs) accessed wrong place
(bad_features2).

After patching mkfs.xfs with your patch, kernel began to use lazy-count
without additional bad_features2 changes.

There seems to be problem with sb_fdblocks count but kernel really look for
features2 and behaves differently if lazy-count is sets on right place.


jan


-- 

Re: [REVIEW] User-space support for bad_features2 patch

[David Chinner]

[ please inline patches so they are easy to quote for review. ]

On Fri, Feb 22, 2008 at 06:52:43PM +1100, Barry Naujok wrote:
> The attached patch fixes mkfs.xfs writing the bad features2 in the first  
> place (the change to xfs_sb.h does this).
> 
> Next xfs_db support printing of this superblock field and xfs_check can  
> report the bad_features2 field is set.
> 
> xfs_repair can correct the error in the same fashion that David Chinner's  
> mount code does it.

Actually, it doesn't:

        /*
+        * Check bad_features2, if set and features2 is zero, copy
+        * bad_features2 to features2 and zero bad_features2.
+        */
+       if (sb->sb_bad_features2 != 0) {
+               if (sb->sb_features2 == 0)
+                       sb->sb_features2 = sb->sb_bad_features2;

This simply copies the bad features over the features field if
the sb_features2 field is zero. This ignores the fact that we may
have set something into the sb_features2 field before detecting the
problem (e.g. attr2 can be turned on dynamically).  The patch I posted
OR'd the two fields together to ensure no feature bits were lost.
This needs to be done here as well.

Cheers,

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