Message-ID: <20170614071133.GD31671@ZenIV.linux.org.uk>
Date:   Wed, 14 Jun 2017 08:11:33 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Richard Narron <comet.berkeley@...il.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] first batch of ufs fixes

On Tue, Jun 13, 2017 at 02:56:23PM -0700, Richard Narron wrote:
> On Tue, 13 Jun 2017, Al Viro wrote:
> 
> > On Mon, Jun 12, 2017 at 05:54:06PM -0700, Richard Narron wrote:
> > 
> > > Earlier today I could not reproduce the OpenBSD 6.1 ufs1 fsck error after
> > > Linux 4.12-rc5 copy of my >2GB file using "cp".
> > > 
> > > But later today I get the error when I copy using your "dd" method...
> > > 
> > > In any case I always get a ufs1 fsck error after the Linux rm and rmdir.
> > 
> > Interesting...  Could you put together an image (starting with zeroing the
> > device before newfs, and ideally with dd from /dev/zero to create files)
> > that would
> > 	a) pass fsck on OpenBSD
> > 	b) after rm on Linux fail the same
> > then convert it to qcow2 and publish?  Or just compress it - all free and
> > data blocks would contain only zeroes, so any kind of compression (gzip,
> > bzip2, whatever) would reduce the size to something more managable...
> 
> I created a gzip and sent you an email with the link to a UFS1 OpenBSD
> filesytem image.
> 
> I finished simple testing of UFS1 with FreeBSD and NetBSD and found no
> problems except for the differences between "available" blocks in df
> commands.

AFAICS, what happens is a combination of OpenBSD and FreeBSD acting differently
on when reading UFS1 and Linux "[PATCH] ufs: make fsck -f happy" getting the
logics wrong.  First of all, on UFS1 writing a superblock always duplicates the
values into old locations, UFS_FLAGS_UPDATED or not.  Linux implementation
writes either only to new or only to old locations.  What's more, on the read
side the rules are different between FreeBSD and OpenBSD.  The former does
	if we hadn't set fs_un.fs_u2.fs_maxbsize to block size
		set it so
		read from old locations (and copy them to new ones)
The latter *always* reads from old locations.  It also sets FS_FLAGS_UPDATED
at the same spot (FreeBSD does it a bit upstream) and has an ifdefed out "if
flag is already set, bugger off" logics.

Hell knows...  Using FS_FLAGS_UPDATED as a predicate is wrong, due to OpenBSD
fsck clearing it when it modifies a superblock for any reason.  FWIW, using
fs_maxbsize as an indicator looks like a good idea.  The thing is, it lives
in place where the first two elements of ->opostbl used to be.  In filesystems
with ->s_postblformat equal to UFS_42POSTBLFMT.  Which excludes everything
created by 4.4 newfs; in fact, 4.3-Reno is already too recent for that.
All of those will have zeroes in the entire ->opostbl area.

AFAICS, a conservative approach would be
	* reject UFS_42POSTBLFMT for 44bsd ones - it's almost certainly
*not* one.
	* check if fs_maxbsize is equal to frag size; treat that as
"counts are read from new location and stored both to old and new".
44bsd fs_maxbsize != block size => not converted, just use old locations
for everything.  UFS2 => use new locations for everything, don't bother
with old ones.  IOW, something like this (WARNING: completely untested,
might screw your filesystem) might do.

NOTE: all I have is your image *after* it had counters buggered; I don't
know the exact sequence of operations that fucked it in your case.  One
way to trigger it is to mount/umount on OpenBSD, then mount/modify/umount
on Linux, then mount/umount on OpenBSD, then fsck on OpenBSD.  This patch
apparently fixes that, but your reproducer might be something different.

Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
---
diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index d9aa2627c9df..eca838a8b43e 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -480,7 +480,7 @@ static void ufs_setup_cstotal(struct super_block *sb)
 	usb3 = ubh_get_usb_third(uspi);
 
 	if ((mtype == UFS_MOUNT_UFSTYPE_44BSD &&
-	     (usb1->fs_flags & UFS_FLAGS_UPDATED)) ||
+	     (usb2->fs_un.fs_u2.fs_maxbsize == usb1->fs_bsize)) ||
 	    mtype == UFS_MOUNT_UFSTYPE_UFS2) {
 		/*we have statistic in different place, then usual*/
 		uspi->cs_total.cs_ndir = fs64_to_cpu(sb, usb2->fs_un.fs_u2.cs_ndir);
@@ -596,9 +596,7 @@ static void ufs_put_cstotal(struct super_block *sb)
 	usb2 = ubh_get_usb_second(uspi);
 	usb3 = ubh_get_usb_third(uspi);
 
-	if ((mtype == UFS_MOUNT_UFSTYPE_44BSD &&
-	     (usb1->fs_flags & UFS_FLAGS_UPDATED)) ||
-	    mtype == UFS_MOUNT_UFSTYPE_UFS2) {
+	if (mtype == UFS_MOUNT_UFSTYPE_UFS2) {
 		/*we have statistic in different place, then usual*/
 		usb2->fs_un.fs_u2.cs_ndir =
 			cpu_to_fs64(sb, uspi->cs_total.cs_ndir);
@@ -608,16 +606,26 @@ static void ufs_put_cstotal(struct super_block *sb)
 			cpu_to_fs64(sb, uspi->cs_total.cs_nifree);
 		usb3->fs_un1.fs_u2.cs_nffree =
 			cpu_to_fs64(sb, uspi->cs_total.cs_nffree);
-	} else {
-		usb1->fs_cstotal.cs_ndir =
-			cpu_to_fs32(sb, uspi->cs_total.cs_ndir);
-		usb1->fs_cstotal.cs_nbfree =
-			cpu_to_fs32(sb, uspi->cs_total.cs_nbfree);
-		usb1->fs_cstotal.cs_nifree =
-			cpu_to_fs32(sb, uspi->cs_total.cs_nifree);
-		usb1->fs_cstotal.cs_nffree =
-			cpu_to_fs32(sb, uspi->cs_total.cs_nffree);
+		goto out;
 	}
+
+	if (mtype == UFS_MOUNT_UFSTYPE_44BSD &&
+	     (usb2->fs_un.fs_u2.fs_maxbsize == usb1->fs_bsize)) {
+		/* store stats in both old and new places */
+		usb2->fs_un.fs_u2.cs_ndir =
+			cpu_to_fs64(sb, uspi->cs_total.cs_ndir);
+		usb2->fs_un.fs_u2.cs_nbfree =
+			cpu_to_fs64(sb, uspi->cs_total.cs_nbfree);
+		usb3->fs_un1.fs_u2.cs_nifree =
+			cpu_to_fs64(sb, uspi->cs_total.cs_nifree);
+		usb3->fs_un1.fs_u2.cs_nffree =
+			cpu_to_fs64(sb, uspi->cs_total.cs_nffree);
+	}
+	usb1->fs_cstotal.cs_ndir = cpu_to_fs32(sb, uspi->cs_total.cs_ndir);
+	usb1->fs_cstotal.cs_nbfree = cpu_to_fs32(sb, uspi->cs_total.cs_nbfree);
+	usb1->fs_cstotal.cs_nifree = cpu_to_fs32(sb, uspi->cs_total.cs_nifree);
+	usb1->fs_cstotal.cs_nffree = cpu_to_fs32(sb, uspi->cs_total.cs_nffree);
+out:
 	ubh_mark_buffer_dirty(USPI_UBH(uspi));
 	ufs_print_super_stuff(sb, usb1, usb2, usb3);
 	UFSD("EXIT\n");
@@ -997,6 +1005,13 @@ static int ufs_fill_super(struct super_block *sb, void *data, int silent)
 		flags |=  UFS_ST_SUN;
 	}
 
+	if ((flags & UFS_ST_MASK) == UFS_ST_44BSD &&
+	    uspi->s_postblformat == UFS_42POSTBLFMT) {
+		if (!silent)
+			pr_err("this is not a 44bsd filesystem");
+		goto failed;
+	}
+
 	/*
 	 * Check ufs magic number
 	 */

Powered by blists - more mailing lists