On Thu, Mar 06, 2014 at 05:54:50PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> We only support filesystems that have v2 directory support, and than
> means all the checking and handling of superblock versions prior to
> this support being added is completely unnecessary overhead.
>
> Strip out all the version 1-3 support, sanitise the good version
> checking to reflect the supported versions, update all the feature
> supported functions and clean up all the support bit definitions to
> reflect the fact that we no longer care about Irix bootloader flag
> regions for v4 feature bits.
Good idea in general, I like it.
> Because the feature bit checking is all inline code, this relatively
> small cleanup has a noticable impact on code size:
I initially though moving it out of line might not be a bad idea,
but it seems after your diet it's lean enough to not bother.
> +/*
> + * We only support superblocks that have at least V2 Dir capability. Any feature
> + * bit added after v2 dir capability is also indicates a supported superblock
> + * format.
> + */
> +#define XFS_SB_NEEDED_FEATURES \
> + (XFS_SB_VERSION_DIRV2BIT | \
> + XFS_SB_VERSION_LOGV2BIT | \
> + XFS_SB_VERSION_SECTORBIT | \
> + XFS_SB_VERSION_BORGBIT | \
> XFS_SB_VERSION_MOREBITSBIT)
This seems a bit odd. Shouldn't we simply check for
XFS_SB_VERSION_DIRV2BIT as we actually rely on that? What if SGI had
backported any of those other features to some IRIX branch?
I'd vote to kill XFS_SB_NEEDED_FEATURES and just check the dirv2 bit
explicitly.
> +/*
> + * Supported feature bit list is just all bits in the versionnum field because
> + * we've used them all up and understand them all.
> + */
> +#define XFS_SB_VERSION_OKBITS \
> + (XFS_SB_VERSION_NUMBITS | \
> + XFS_SB_VERSION_ALLFBITS)
>
> +#define XFS_SB_VERSION2_OKBITS \
> (XFS_SB_VERSION2_LAZYSBCOUNTBIT | \
> XFS_SB_VERSION2_ATTR2BIT | \
> XFS_SB_VERSION2_PROJID32BIT | \
> XFS_SB_VERSION2_FTYPE)
> +/*
> + * The first XFS version we support is filesytsems with V2 directories.
> + */
is a v4 superblock with v2 directories?
Also filesystem is mis-spelled.
> static inline int xfs_sb_good_version(xfs_sb_t *sbp)
> {
> + /* We only support v4 and v5 */
> + if (XFS_SB_VERSION_NUM(sbp) < XFS_SB_VERSION_4 ||
> + XFS_SB_VERSION_NUM(sbp) > XFS_SB_VERSION_5)
> + return 0;
> +
> + /*
> + * Version 5 feature checks are done separately.
> + */
> + if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
> return 1;
How about doing this a little different?
static inline int xfs_sb_good_version(struct xfs_sb *sbp)
{
if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)
return 1;
if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4)
return xfs_sb_good_v4_features(sbp);
return 0;
}
then move the bulk of the function into xfs_sb_good_v4_features?
> + if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> + ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
> + (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
> return 0;
> + if (sbp->sb_shared_vn > XFS_SB_MAX_SHARED_VN)
> + return 0;
Given that XFS_SB_MAX_SHARED_VN we might as well make that and != 0
check and document that we don't support any shared superblocks.
> static inline int xfs_sb_version_hasattr(xfs_sb_t *sbp)
> {
> + return !!(sbp->sb_versionnum & XFS_SB_VERSION_ATTRBIT);
> }
Should this become a bool?
>
> static inline void xfs_sb_version_addattr(xfs_sb_t *sbp)
> {
> + sbp->sb_versionnum |= XFS_SB_VERSION_ATTRBIT;
> }
I'd rather not keep the wrappers for adding these flags - the callers
already know sb internals, might as well not keep a false abstraction
here.
> static inline int xfs_sb_version_hasnlink(xfs_sb_t *sbp)
> {
> - return sbp->sb_versionnum == XFS_SB_VERSION_3 ||
> - (XFS_SB_VERSION_NUM(sbp) >= XFS_SB_VERSION_4 &&
> - (sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT));
> + return !!(sbp->sb_versionnum & XFS_SB_VERSION_NLINKBIT);
> }
As we reject filesystems without the nlink bit we should just be able
to kill all code protected by xfs_sb_version_hasnlink checks, shouldn't
we?
> static inline void xfs_sb_version_addnlink(xfs_sb_t *sbp)
> {
> + sbp->sb_versionnum |= XFS_SB_VERSION_NLINKBIT;
> }
Same for addnlink.
> static inline int xfs_sb_version_hasdirv2(xfs_sb_t *sbp)
> {
> - return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) ||
> - (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4 &&
> - (sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT));
> + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 ||
> + (sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT);
> }
dirv2 is another candidate.
> @@ -536,11 +493,13 @@ static inline void xfs_sb_version_addattr2(xfs_sb_t *sbp)
> {
> sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
> sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT;
> + sbp->sb_bad_features2 |= XFS_SB_VERSION2_ATTR2BIT;
> }
>
> static inline void xfs_sb_version_removeattr2(xfs_sb_t *sbp)
> {
> sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
> + sbp->sb_bad_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
Where is this coming from? Seems unrelated to the other changes.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
