xfs: fix up inode32/64 (re)mount handling

inode32/inode64 allocator behavior with respect to mount, remount and growfs is a little tricky.

The inode32 mount option should only enable the inode32 allocator heuristics if the filesystem is large enough for 64-bit inodes to exist. Today, it has this behavior on the initial mount, but a remount with inode32 unconditionally changes the allocation heuristics, even for a small fs.

Also, an inode32 mounted small filesystem should transition to the inode32 allocator if the filesystem is subsequently grown to a sufficient size. Today that does not happen.

This patch consolidates xfs_set_inode32 and xfs_set_inode64 into a single new function, and moves the "is the maximum inode number big enough to matter" test into that function, so it doesn't rely on the caller to get it right - which remount did not do, previously.

Signed-off-by Eric Sandeen <[email protected]>

---

Note, this goes after my token-parsing patch for mount.

fs/xfs/xfs_super.c +22 -8

index fe4c14e..044b416 100644
@@ -577,23 +577,35 @@ xfs_max_file_offset(
 }
 
 /*
- * xfs_set_inode32() and xfs_set_inode64() are passed an agcount
- * because in the growfs case, mp->m_sb.sb_agcount is not updated
- * yet to the potentially higher ag count.
+ * Set parameters for inode allocation heuristics, taking into account
+ * filesystem size and inode32/inode64 mount options; i.e. specifically
+ * whether or not XFS_MOUNT_SMALL_INUMS is set.
+ *
+ * Inode allocation patterns are altered only if inode32 is requested
+ * (XFS_MOUNT_SMALL_INUMS), and the filesystem is sufficiently large.
+ * If altered, XFS_MOUNT_32BITINODES is set as well.
+ *
+ * An agcount independent of that in the mount structure is provided
+ * because in the growfs case, mp->m_sb.sb_agcount is not yet updated
+ * to the potentially higher ag count.
+ *
+ * Returns the maximum AG index which may contain inodes.
  */
 xfs_agnumber_t
-xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
+xfs_set_inode_alloc(
+	struct xfs_mount *mp,
+	xfs_agnumber_t	agcount)
 {
-	xfs_agnumber_t	index = 0;
+	xfs_agnumber_t	index;
 	xfs_agnumber_t	maxagi = 0;
 	xfs_sb_t	*sbp = &mp->m_sb;
 	xfs_agnumber_t	max_metadata;
 	xfs_agino_t	agino;
 	xfs_ino_t	ino;
-	xfs_perag_t	*pag;
 
-	/* Calculate how much should be reserved for inodes to meet
-	 * the max inode percentage.
+	/*
+	 * Calculate how much should be reserved for inodes to meet
+	 * the max inode percentage.  Used only for inode32.
 	 */
 	if (mp->m_maxicount) {
 		__uint64_t	icount;

@@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
 		max_metadata = agcount;
 	}
 
+	/* Get the last possible inode in the filesystem */
 	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
+	ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);

fs/xfs/xfs_super.c +23 -39

 	for (index = 0; index < agcount; index++) {
-		ino = XFS_AGINO_TO_INO(mp, index, agino);
+		struct xfs_perag	*pag;
 
-		if (ino > XFS_MAXINUMBER_32) {
-			pag = xfs_perag_get(mp, index);
-			pag->pagi_inodeok = 0;
-			pag->pagf_metadata = 0;
-			xfs_perag_put(pag);
-			continue;
-		}
+		ino = XFS_AGINO_TO_INO(mp, index, agino);
 
 		pag = xfs_perag_get(mp, index);
-		pag->pagi_inodeok = 1;
-		maxagi++;
-		if (index < max_metadata)
-			pag->pagf_metadata = 1;
-		xfs_perag_put(pag);
-	}
-	mp->m_flags |= (XFS_MOUNT_32BITINODES |
-			XFS_MOUNT_SMALL_INUMS);
 
-	return maxagi;
-}
-
-xfs_agnumber_t
-xfs_set_inode64(struct xfs_mount *mp, xfs_agnumber_t agcount)
-{
-	xfs_agnumber_t index = 0;
-
-	for (index = 0; index < agcount; index++) {
-		struct xfs_perag	*pag;
+		if (mp->m_flags & XFS_MOUNT_32BITINODES) {
+			if (ino > XFS_MAXINUMBER_32) {
+				pag->pagi_inodeok = 0;
+				pag->pagf_metadata = 0;
+			} else {
+				pag->pagi_inodeok = 1;
+				maxagi++;
+				if (index < max_metadata)
+					pag->pagf_metadata = 1;
+				else
+					pag->pagf_metadata = 0;
+			}
+		} else {
+			pag->pagi_inodeok = 1;
+			pag->pagf_metadata = 0;
+		}
 
-		pag = xfs_perag_get(mp, index);
-		pag->pagi_inodeok = 1;
-		pag->pagf_metadata = 0;
 		xfs_perag_put(pag);
 	}
 
-	/* There is no need for lock protection on m_flags,
-	 * the rw_semaphore of the VFS superblock is locked
-	 * during mount/umount/remount operations, so this is
-	 * enough to avoid concurency on the m_flags field
-	 */
-	mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
-			 XFS_MOUNT_SMALL_INUMS);
-	return index;
+	return (mp->m_flags & XFS_MOUNT_32BITINODES) ? maxagi : agcount;
 }
 
 STATIC int

@@ -1224,10 +1230,12 @@ xfs_fs_remount(
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
 			break;
 		case Opt_inode64:
-			mp->m_maxagi = xfs_set_inode64(mp, sbp->sb_agcount);
+			mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
+			mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
 			break;
 		case Opt_inode32:
-			mp->m_maxagi = xfs_set_inode32(mp, sbp->sb_agcount);
+			mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
+			mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
 			break;
 		default:
 			/*

fs/xfs/xfs_super.h +2 -2

index 499058f..2dfb1ce 100644
@@ -65,8 +65,8 @@ extern __uint64_t xfs_max_file_offset(unsigned int);
 
 extern void xfs_flush_inodes(struct xfs_mount *mp);
 extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
-extern xfs_agnumber_t xfs_set_inode32(struct xfs_mount *, xfs_agnumber_t agcount);
-extern xfs_agnumber_t xfs_set_inode64(struct xfs_mount *, xfs_agnumber_t agcount);
+extern xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *,
+					   xfs_agnumber_t agcount);
 
 extern const struct export_operations xfs_export_operations;
 extern const struct xattr_handler *xfs_xattr_handlers[];

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 24 lines · Eric Sandeen

inode32/inode64 allocator behavior with respect to mount, remount and growfs is a little tricky.

The inode32 mount option should only enable the inode32 allocator heuristics if the filesystem is large enough for 64-bit inodes to exist. Today, it has this behavior on the initial mount, but a remount with inode32 unconditionally changes the allocation heuristics, even for a small fs.

Also, an inode32 mounted small filesystem should transition to the inode32 allocator if the filesystem is subsequently grown to a sufficient size. Today that does not happen.

This patch consolidates xfs_set_inode32 and xfs_set_inode64 into a single new function, and moves the "is the maximum inode number big enough to matter" test into that function, so it doesn't rely on the caller to get it right - which remount did not do, previously.

Signed-off-by Eric Sandeen <[email protected]>

---

Note, this goes after my token-parsing patch for mount.

Quoted 7 lines

fs/xfs/xfs_super.c +2 -0

@@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
 		max_metadata = agcount;
 	}
 
+	/* Get the last possible inode in the filesystem */
 	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
+	ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);

fs/xfs/xfs_super.c xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount) · line 634

Quoted 10 lines

fs/xfs/xfs_super.c +10 -0

+
+	/*
+	 * If user asked for no more than 32-bit inodes, and the fs is
+	 * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
+	 * the allocator to accommodate the request.
+	 */
+	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
+		mp->m_flags |= XFS_MOUNT_32BITINODES;
+	else
+		mp->m_flags &= ~XFS_MOUNT_32BITINODES;

In the current code, we call into xfs_set_inode64() if XFS_MOUNT_SMALL_INUMS is not set or it is, but the largest inode is within XFS_MAXINUMBER_32. In that latter case, xfs_set_inode64() does:

        mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
                         XFS_MOUNT_SMALL_INUMS);

... which I think means we want to clear XFS_MOUNT_SMALL_INUMS along with XFS_MOUNT_32BITINODES here, yes? The rest looks fine to me:

Quoted 3 lines

Also, an inode32 mounted small filesystem should transition to the inode32 allocator if the filesystem is subsequently grown to a sufficient size. Today that does not happen.

Reviewed-by Brian Foster <[email protected]>

Quoted 25 lines · Brian Foster

Quoted 24 lines · Eric Sandeen

inode32/inode64 allocator behavior with respect to mount, remount and growfs is a little tricky.

The inode32 mount option should only enable the inode32 allocator heuristics if the filesystem is large enough for 64-bit inodes to exist. Today, it has this behavior on the initial mount, but a remount with inode32 unconditionally changes the allocation heuristics, even for a small fs.

Also, an inode32 mounted small filesystem should transition to the inode32 allocator if the filesystem is subsequently grown to a sufficient size. Today that does not happen.

This patch consolidates xfs_set_inode32 and xfs_set_inode64 into a single new function, and moves the "is the maximum inode number big enough to matter" test into that function, so it doesn't rely on the caller to get it right - which remount did not do, previously.

Signed-off-by Eric Sandeen <[email protected]>

---

Note, this goes after my token-parsing patch for mount.

...

Quoted 25 lines

Quoted 7 lines

fs/xfs/xfs_super.c +2 -0

@@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
 		max_metadata = agcount;
 	}
 
+	/* Get the last possible inode in the filesystem */
 	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
+	ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);

fs/xfs/xfs_super.c xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount) · line 634

Quoted 10 lines

fs/xfs/xfs_super.c +10 -0

+
+	/*
+	 * If user asked for no more than 32-bit inodes, and the fs is
+	 * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
+	 * the allocator to accommodate the request.
+	 */
+	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
+		mp->m_flags |= XFS_MOUNT_32BITINODES;
+	else
+		mp->m_flags &= ~XFS_MOUNT_32BITINODES;

In the current code, we call into xfs_set_inode64() if XFS_MOUNT_SMALL_INUMS is not set or it is, but the largest inode is within XFS_MAXINUMBER_32. In that latter case, xfs_set_inode64() does:

        mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
                         XFS_MOUNT_SMALL_INUMS);

Quoted 2 lines

... which I think means we want to clear XFS_MOUNT_SMALL_INUMS along with XFS_MOUNT_32BITINODES here, yes? The rest looks fine to me:

I don't think so; that was a bug, AFAICT.

XFS_MOUNT_32BITINODES means that inode32 was specified at mount

time, i.e. the user wants no more than 32-bit inodes for the duration of this mount.

So this is actually a bugfix for the 2nd item mentioned above:

Quoted 5 lines

Quoted 3 lines

Also, an inode32 mounted small filesystem should transition to the inode32 allocator if the filesystem is subsequently grown to a sufficient size. Today that does not happen.

Reviewed-by Brian Foster <[email protected]>

Thanks,
-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 54 lines · Eric Sandeen

Quoted 25 lines · Brian Foster

Quoted 24 lines · Eric Sandeen

inode32/inode64 allocator behavior with respect to mount, remount and growfs is a little tricky.

The inode32 mount option should only enable the inode32 allocator heuristics if the filesystem is large enough for 64-bit inodes to exist. Today, it has this behavior on the initial mount, but a remount with inode32 unconditionally changes the allocation heuristics, even for a small fs.

Also, an inode32 mounted small filesystem should transition to the inode32 allocator if the filesystem is subsequently grown to a sufficient size. Today that does not happen.

This patch consolidates xfs_set_inode32 and xfs_set_inode64 into a single new function, and moves the "is the maximum inode number big enough to matter" test into that function, so it doesn't rely on the caller to get it right - which remount did not do, previously.

Signed-off-by Eric Sandeen <[email protected]>

---

Note, this goes after my token-parsing patch for mount.

...

Quoted 25 lines

Quoted 7 lines

fs/xfs/xfs_super.c +2 -0

@@ -607,54 +619,48 @@ xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount)
 		max_metadata = agcount;
 	}
 
+	/* Get the last possible inode in the filesystem */
 	agino =	XFS_OFFBNO_TO_AGINO(mp, sbp->sb_agblocks - 1, 0);
+	ino = XFS_AGINO_TO_INO(mp, agcount - 1, agino);

fs/xfs/xfs_super.c xfs_set_inode32(struct xfs_mount *mp, xfs_agnumber_t agcount) · line 634

Quoted 10 lines

fs/xfs/xfs_super.c +10 -0

+
+	/*
+	 * If user asked for no more than 32-bit inodes, and the fs is
+	 * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
+	 * the allocator to accommodate the request.
+	 */
+	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32)
+		mp->m_flags |= XFS_MOUNT_32BITINODES;
+	else
+		mp->m_flags &= ~XFS_MOUNT_32BITINODES;

In the current code, we call into xfs_set_inode64() if XFS_MOUNT_SMALL_INUMS is not set or it is, but the largest inode is within XFS_MAXINUMBER_32. In that latter case, xfs_set_inode64() does:

        mp->m_flags &= ~(XFS_MOUNT_32BITINODES |
                         XFS_MOUNT_SMALL_INUMS);

Quoted 6 lines

Quoted 2 lines

... which I think means we want to clear XFS_MOUNT_SMALL_INUMS along with XFS_MOUNT_32BITINODES here, yes? The rest looks fine to me:

I don't think so; that was a bug, AFAICT.

XFS_MOUNT_32BITINODES means that inode32 was specified at mount

Ugh; I had that backwards.

*XFS_MOUNT_SMALL_INUMS* means that inode32 was specified at mount time. For the reasons I stated, *that* flag should never be cleared. It signifies a specified mount option, which does not go away just because the filesystem is currently small.

Maybe we need clearer flag names :/

-Eric

Quoted 11 lines

time, i.e. the user wants no more than 32-bit inodes for the duration of this mount.

So this is actually a bugfix for the 2nd item mentioned above:

Quoted 5 lines

Quoted 3 lines

Also, an inode32 mounted small filesystem should transition to the inode32 allocator if the filesystem is subsequently grown to a sufficient size. Today that does not happen.

Reviewed-by Brian Foster <[email protected]>

Quoted 8 lines

Thanks,
-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs