On 4/16/25 13:05, Nirjhar Roy (IBM) wrote:
On 4/16/25 11:39, Christoph Hellwig wrote:
On Tue, Apr 15, 2025 at 12:48:39PM +0530, Nirjhar Roy (IBM) wrote:
condition(noattr2 on v5) is not caught in xfs_fs_validate_params()
because
the superblock isn't read yet and "struct xfs_mount *mp" is still
not
aware of whether the underlying filesystem is v5 or v4 (i.e, whether
crc=0
or crc=1). So, even if the underlying filesystem is v5,
xfs_has_attr2() will
return false, because the m_features isn't populated yet.
Yes.
However, once
xfs_readsb() is done, m_features is populated (mp->m_features |=
xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()).
After
that, when xfs_finish_flags() is called, the invalid mount option (i.e,
noattr2 with crc=1) is caught, and the mount fails correctly. So,
m_features
is partially populated while xfs_fs_validate_params() is getting
executed, I
am not sure if that is done intentionally.
As you pointed out above it can't be fully populated becaue we don't
have all the information. And that can't be fixed because some of the
options are needed to even get to reading the superblock.
So we do need a second pass of verification for everything that depends
Yes, we need a second pass and I think that is already being done by
xfs_finish_flags() in xfs_fs_fill_super(). However, in
xfs_fs_reconfigure(), we still need a check to make a transition from
/* attr2 -> noattr2 */ and /* noattr2 -> attr2 */ (only if it is
permitted to) and update mp->m_features accordingly, just like it is
being done for inode32 <-> inode64, right? Also, in your previous
reply[1], you did suggest moving the crc+noattr2 check to
xfs_fs_validate_params() - Are you suggesting to add another optional
(NULLable) parameter "new_mp" to xfs_fs_validate_params() and then
moving the check to xfs_fs_validate_params()?
[1] https://lore.kernel.org/all/Z_yhpwBQz7Xs4WLI@xxxxxxxxxxxxx/
--NR
Hi Christoph,
Any further feedback on the above and the overall patch? Can you please
suggest the changes you want me to do for the patch?
--NR
on informationtion from the superblock. The fact that m_features
mixes user options and on-disk feature bits is unfortunately not very
helpful for a clear structure here.
--
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore
