Re: [PATCH] xfs: fix up inode32/64 (re)mount handling

Eric Sandeen
2016-02-18T05:46:44Z

On 2/17/16 12:30 PM, Brian Foster wrote:
> On Tue, Feb 16, 2016 at 10:47:49PM -0600, Eric Sandeen wrote:
>> 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 <sandeen@redhat.com>
>> ---
>>
>> Note, this goes after my token-parsing patch for mount.

...

>> @@ -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);
>> +
>> + /*
>> + * 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:

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:

>> 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 <bfoster@redhat.com>

Thanks,
-Eric