--rwEMma7ioTxnRzrJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Sep 06, 2003 at 12:30:26AM +0200, Manuel Bouyer wrote:
> [...]
> 
> FS_FLAGS_UPDATED is used to tell the kernel that the superblock has been
> updated to the new world. It's used in 3 places:
> - ffs_mountfs():
>                 if ((fsblockloc == sblockloc ||
> 		     (fs->fs_old_flags & FS_FLAGS_UPDATED) == 0)
> 		      && sbsize <= MAXBSIZE && sbsize >= sizeof(struct fs))
>   If I read it properly, we always have fsblockloc == sblockloc for
>   FS_UFS1_MAGIC filesystems, so we could check FS_FLAGS_UPDATED for v2
>   filesystems only.

In fact, as fsblockloc == sblockloc in this case,
(fs->fs_old_flags & FS_FLAGS_UPDATED) == 0 is never checked for UFS1
filesystens. No need to change anything here.
FreeBSD has an explicit FS_UFS2_MAGIC check here, but it's not needed.

> - ffs_oldfscompat_read():
>   we could probably move it under the
>   if (fs->fs_maxbsize != fs->fs_bsize || fs->fs_time < fs->fs_old_time) 
>   check. We probably want to update the flags anyway if the filesystem
>   was mounted R/W by an old kernel (maybe fs->fs_flags |= fs->fs_old_flags
>   instead of =, and clear any new flags if fs->fs_maxbsize != fs->fs_bsize) ?
> - ffs_sbupdate(): update fs_sblockloc. Unless I missed something,
>   this should already have been done by ffs_oldfscompat_read() ?

In fact, FS_FLAGS_UPDATED is *never* set in fs->fs_flags, it's set in
fs->fs_old_flags. So ffs_sbupdate() doens't need to check FS_FLAGS_UPDATED
here.
I cheched the freebsd code, and from what I can see, it never sets
FS_FLAGS_UPDATED in fs_flags either.

> 
> So it looks like we don't need to set FS_FLAGS_UPDATED for v1 filesystem on
> NetBSD. But unfortunably, not doing this may cause compatibility problems with
> others BSD.
> 
> I think we could remove checks of FS_FLAGS_UPDATED in our kernel, and
> not set FS_FLAGS_UPDATED if we're not using any of the new features
> (basically, if fs->fs_flags contains only FS_UNCLEAN and FS_DOSOFTDEP)

The attached patch implement this:
- don't rely on FS_FLAGS_UPDATED to see if we need to update new fields from
  old fields.
- when writing back the superblock, copy back the flags to the old location
  if only old flags are set (FS_FLAGS_UPDATED won't be set in this case).
  If mounted on other OS, the lack of FS_FLAGS_UPDATED will trigger the
  compat code, but it shouldn't be a problem as the old fields are up to
  date. It will also help for filesystems shared between old and new
  kernels and fsck (one could think the filesystem is clean when it isn't).

I can't think of a pattern of use were we could loose with these changes.

Comments ? If nobody objects, I'll get this commited next week-end.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 24 ans d'experience feront toujours la difference
--

--rwEMma7ioTxnRzrJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="ffsei_current.diff"

Index: ffs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.119
diff -u -r1.119 ffs_vfsops.c
--- ffs_vfsops.c	2003/08/07 16:34:31	1.119
+++ ffs_vfsops.c	2003/09/06 16:26:52
@@ -964,36 +964,32 @@
 		return;
 
 	/*
-	 * If not yet done, update fs_flags location and value of fs_sblockloc.
+	 * If not yet done, update UFS1 superblock with new wider fields,
+	 * and  update fs_flags location and value of fs_sblockloc.
 	 */
-	if ((fs->fs_old_flags & FS_FLAGS_UPDATED) == 0) {
+	if (fs->fs_maxbsize != fs->fs_bsize) {
+		fs->fs_maxbsize = fs->fs_bsize;
+		fs->fs_time = fs->fs_old_time;
+		fs->fs_size = fs->fs_old_size;
+		fs->fs_dsize = fs->fs_old_dsize;
+		fs->fs_csaddr = fs->fs_old_csaddr;
+		fs->fs_sblockloc = sblockloc;
 		fs->fs_flags = fs->fs_old_flags;
 		fs->fs_old_flags |= FS_FLAGS_UPDATED;
-		fs->fs_sblockloc = sblockloc;
 	}
-
 	/*
 	 * If the new fields haven't been set yet, or if the filesystem
 	 * was mounted and modified by an old kernel, use the old csum
-	 * totals.
+	 * totals, and update the flags
 	 */
 	if (fs->fs_maxbsize != fs->fs_bsize || fs->fs_time < fs->fs_old_time) {
 		fs->fs_cstotal.cs_ndir = fs->fs_old_cstotal.cs_ndir;
 		fs->fs_cstotal.cs_nbfree = fs->fs_old_cstotal.cs_nbfree;
 		fs->fs_cstotal.cs_nifree = fs->fs_old_cstotal.cs_nifree;
 		fs->fs_cstotal.cs_nffree = fs->fs_old_cstotal.cs_nffree;
+		fs->fs_flags |= (fs->fs_old_flags & ~FS_FLAGS_UPDATED);
 	}
 
-	/*
-	 * If not yet done, update UFS1 superblock with new wider fields.
-	 */
-	if (fs->fs_maxbsize != fs->fs_bsize) {
-		fs->fs_maxbsize = fs->fs_bsize;
-		fs->fs_time = fs->fs_old_time;
-		fs->fs_size = fs->fs_old_size;
-		fs->fs_dsize = fs->fs_old_dsize;
-		fs->fs_csaddr = fs->fs_old_csaddr;
-	}
 
 	if (fs->fs_old_inodefmt < FS_44INODEFMT) {
 		fs->fs_maxfilesize = (u_quad_t) 1LL << 39;
@@ -1035,6 +1031,12 @@
 		return;
 
 	/*
+	 * if none of the newer flags are used, copy back fs_flags
+	 * to fs_old_flags
+	 */
+	if ((fs->fs_flags & ~(FS_UNCLEAN|FS_DOSOFTDEP)) == 0)
+		fs->fs_old_flags = fs->fs_flags & (FS_UNCLEAN|FS_DOSOFTDEP);
+	/*
 	 * OS X somehow still seems to use this field and panic.
 	 * Just set it to zero.
 	 */
@@ -1560,16 +1562,14 @@
 	    (int)fs->fs_sbsize, 0, 0);
 	saveflag = fs->fs_flags & FS_INTERNAL;
 	fs->fs_flags &= ~FS_INTERNAL;
-
-	if (fs->fs_magic == FS_UFS1_MAGIC && fs->fs_sblockloc != SBLOCK_UFS1 &&
-	    (fs->fs_flags & FS_FLAGS_UPDATED) == 0) {
+	
+	if (fs->fs_magic == FS_UFS1_MAGIC && fs->fs_sblockloc != SBLOCK_UFS1) {
 		printf("%s: correcting fs_sblockloc from %" PRId64 " to %d\n",
 		    fs->fs_fsmnt, fs->fs_sblockloc, SBLOCK_UFS1);
 		fs->fs_sblockloc = SBLOCK_UFS1;
 	}
 
-	if (fs->fs_magic == FS_UFS2_MAGIC && fs->fs_sblockloc != SBLOCK_UFS2 &&
-	    (fs->fs_flags & FS_FLAGS_UPDATED) == 0) {
+	if (fs->fs_magic == FS_UFS2_MAGIC && fs->fs_sblockloc != SBLOCK_UFS2) {
 		printf("%s: correcting fs_sblockloc from %" PRId64 " to %d\n",
 		    fs->fs_fsmnt, fs->fs_sblockloc, SBLOCK_UFS2);
 		fs->fs_sblockloc = SBLOCK_UFS2;

--rwEMma7ioTxnRzrJ--