On Wed, Feb 19, 2020 at 02:21:22PM -0500, Brian Foster wrote:
> On Wed, Feb 19, 2020 at 07:45:19PM +0100, Christoph Hellwig wrote:
> > On Wed, Feb 19, 2020 at 09:52:34AM -0500, Brian Foster wrote:
> > > FWIW, I don't really view this patch as a straightforward
> > > simplification. IMO, this slightly sacrifices readability for slightly
> > > less code and a smaller xfs_icdinode. That might be acceptable... I
> >
> > I actually find it easier to read. The per-inode versioning seems
> > to suggest inodes could actually be different on the same fs, while
> > the new one makes it clear that all inodes on the fs are the same.
> >
>
> It's subjective. I read it as that the logic assumes all inodes on the
> fs are the same version, but doesn't tell me anything about whether that
> assumption is (or will always be) true. I find that confusing,
> particularly since that's not always the case on older sb versions that
> we still support. IOW, so long as the codebase has to handle the common
> denominator of non-uniform inode formats (or might in the future), I
> don't see much value in using such mixed (feature level) logic when the
> per-inode versioning handles both regardless of the particular sb
> version policy. Just my .02.
>
> > > don't feel terribly strongly against it, but to me the explicit version
> > > checks are more clear in cases where the _hascrc() check is not used for
> > > something that is obviously CRC related (which is a pattern I'm
> > > generally not a fan of).
> >
> > xfs_sb_version_hascrc is rather misnamed unfortunately. In fact I think
> > just open coding it as 'XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5'
> > would improve things quite a bit.
> >
>
> Agreed. This would help mitigate my aesthetic gripe around the whole 'if
> (hascrc) { <do some non-crc related stuff> }' thing, at least.
That would work for me too. Maybe leave a comment somewhere that
XFS_SB_VERSION_5 is required for ondisk di_version == 3, if we haven't
already done so?
--D
> Brian
>
