Re: [PATCH 4/7] xfs: zap broken inode forks

[Christoph Hellwig <hch@infradead.org>]

> +/* Verify the consistency of an inline attribute fork. */
> +xfs_failaddr_t
> +xfs_attr_shortform_verify(
> +	struct xfs_inode		*ip)
> +{
> +	struct xfs_attr_shortform	*sfp;
> +	struct xfs_ifork		*ifp;
> +	int64_t				size;
> +
> +	ASSERT(ip->i_af.if_format == XFS_DINODE_FMT_LOCAL);
> +	ifp = xfs_ifork_ptr(ip, XFS_ATTR_FORK);
> +	sfp = (struct xfs_attr_shortform *)ifp->if_u1.if_data;
> +	size = ifp->if_bytes;
> +
> +	return xfs_attr_shortform_verify_struct(sfp, size);

Given that xfs_attr_shortform_verify only has a single caller in the
kernel and no extra n in xfsprogs I'd just change the calling
convention to pass the xfs_attr_shortform structure and size there
and not bother with the wrapper.

> +/* Check that an inode's extent does not have invalid flags or bad ranges. */
> +xfs_failaddr_t
> +xfs_bmap_validate_extent(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	struct xfs_bmbt_irec	*irec)
> +{
> +	return xfs_bmap_validate_extent_raw(ip->i_mount,
> +			XFS_IS_REALTIME_INODE(ip), whichfork, irec);
> +}

.. while this one has a bunch of caller so I expect it's actually
somewhat useful.

> +extern xfs_failaddr_t xfs_dir2_sf_verify_struct(struct xfs_mount *mp,
> +		struct xfs_dir2_sf_hdr *sfp, int64_t size);

It would be nice if we didn't add more pointless externs to function
declarations in heders.

> +xfs_failaddr_t
> +xfs_dir2_sf_verify(
> +	struct xfs_inode		*ip)
> +{
> +	struct xfs_mount		*mp = ip->i_mount;
> +	struct xfs_ifork		*ifp = xfs_ifork_ptr(ip, XFS_DATA_FORK);
> +	struct xfs_dir2_sf_hdr		*sfp;
> +
> +	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
> +
> +	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> +	return xfs_dir2_sf_verify_struct(mp, sfp, ifp->if_bytes);
> +}

This one also only has a single caller in the kernel and user space
combined, so I wou;dn't bother with the wrapper.

> +xfs_failaddr_t
> +xfs_symlink_shortform_verify(
> +	struct xfs_inode	*ip)
> +{
> +	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, XFS_DATA_FORK);
> +
> +	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
> +
> +	return xfs_symlink_sf_verify_struct(ifp->if_u1.if_data, ifp->if_bytes);
> +}

Same here.

Once past thes nitpicks the zapping functionality looks fine to me,
but leaves me with a very high level question:

As far as I can tell the inodes with the zapped fork(s) remains in it's
normal place, and normaly accessible, and I think any read will return
zeroes because i_size isn't reset.  Which would change the data seen
by an application using it.  Don't we need to block access to it until
it is fully repaired?