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

[Eric Sandeen <sandeen@sandeen.net>]

On 2/17/16 11:46 PM, Eric Sandeen wrote:
> 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

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

> 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
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs