Re: [PATCH v3] xfs: deprecate the V4 format

[Eric Sandeen <sandeen@redhat.com>]

On 9/14/20 4:54 PM, Dave Chinner wrote:
> On Mon, Sep 14, 2020 at 02:12:41PM -0700, Darrick J. Wong wrote:
>> On Mon, Sep 14, 2020 at 08:29:09AM +0100, Christoph Hellwig wrote:
>>> On Fri, Sep 11, 2020 at 09:43:11AM -0700, Darrick J. Wong wrote:
>>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>>
>>>> The V4 filesystem format contains known weaknesses in the on-disk format
>>>> that make metadata verification diffiult.  In addition, the format will
>>>> does not support dates past 2038 and will not be upgraded to do so.
>>>> Therefore, we should start the process of retiring the old format to
>>>> close off attack surfaces and to encourage users to migrate onto V5.
>>>>
>>>> Therefore, make XFS V4 support a configurable option.  For the first
>>>> period it will be default Y in case some distributors want to withdraw
>>>> support early; for the second period it will be default N so that anyone
>>>> who wishes to continue support can do so; and after that, support will
>>>> be removed from the kernel.
>>>>
>>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>>> ---
>>>> v3: be a little more helpful about old xfsprogs and warn more loudly
>>>> about deprecation
>>>> v2: define what is a V4 filesystem, update the administrator guide
>>>
>>> Whie this patch itself looks good, I think the ifdef as is is rather
>>> silly as it just prevents mounting v4 file systems without reaping any
>>> benefits from that.
>>>
>>> So at very least we should add a little helper like this:
>>>
>>> static inline bool xfs_sb_is_v4(truct xfs_sb *sbp)
>>> {
>>> 	if (IS_ENABLED(CONFIG_XFS_SUPPORT_V4))
>>> 		return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4;
>>> 	return false;
>>> }
>>>
>>> and use it in all the feature test macros to let the compile eliminate
>>> all the dead code.
>>
>> Oh, wait, you meant as a means for future patches to make various bits
>> of code disappear, not just as a weird one-off thing for this particular
>> patch?
>>
>> I mean... maybe we should just stuff that into the hascrc predicate,
>> like Eric sort of implied on irc.  Hmm, I'll look into that.
> 
> Killing dead code is not the goal of this patch, getting the policy
> in place and documenting it sufficiently is the goal of this patch.
> 
> Optimise the implementation in follow-on patches, don't obfuscate
> this one by commingling it with wide-spread code changes...

Agreed - 

To be clear, the (messy) patch I sent was supposed to be a follow on
patch, not something to merge with the original.

-Eric