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.
Brian
