On 09 Dec 2021 at 04:45, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> As part of multiple customer escalations due to file data corruption
> after copy on write operations, I wrote some fstests that use fsstress
> to hammer on COW to shake things loose. Regrettably, I caught some
> filesystem shutdowns due to incorrect rmap operations with the following
> loop:
>
> mount <filesystem> # (0)
> fsstress <run only readonly ops> & # (1)
> while true; do
> fsstress <run all ops>
> mount -o remount,ro # (2)
> fsstress <run only readonly ops>
> mount -o remount,rw # (3)
> done
>
> When (2) happens, notice that (1) is still running. xfs_remount_ro will
> call xfs_blockgc_stop to walk the inode cache to free all the COW
> extents, but the blockgc mechanism races with (1)'s reader threads to
> take IOLOCKs and loses, which means that it doesn't clean them all out.
> Call such a file (A).
>
> When (3) happens, xfs_remount_rw calls xfs_reflink_recover_cow, which
> walks the ondisk refcount btree and frees any COW extent that it finds.
> This function does not check the inode cache, which means that incore
> COW forks of inode (A) is now inconsistent with the ondisk metadata. If
> one of those former COW extents are allocated and mapped into another
> file (B) and someone triggers a COW to the stale reservation in (A), A's
> dirty data will be written into (B) and once that's done, those blocks
> will be transferred to (A)'s data fork without bumping the refcount.
>
> The results are catastrophic -- file (B) and the refcount btree are now
> corrupt. Solve this race by forcing the xfs_blockgc_free_space to run
> synchronously, which causes xfs_icwalk to return to inodes that were
> skipped because the blockgc code couldn't take the IOLOCK. This is safe
> to do here because the VFS has already prohibited new writer threads.
>
Looks good.
Reviewed-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
> Fixes: 10ddf64e420f ("xfs: remove leftover CoW reservations when remounting ro")
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_super.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e21459f9923a..778b57b1f020 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1765,7 +1765,10 @@ static int
> xfs_remount_ro(
> struct xfs_mount *mp)
> {
> - int error;
> + struct xfs_icwalk icw = {
> + .icw_flags = XFS_ICWALK_FLAG_SYNC,
> + };
> + int error;
>
> /*
> * Cancel background eofb scanning so it cannot race with the final
> @@ -1773,8 +1776,13 @@ xfs_remount_ro(
> */
> xfs_blockgc_stop(mp);
>
> - /* Get rid of any leftover CoW reservations... */
> - error = xfs_blockgc_free_space(mp, NULL);
> + /*
> + * Clear out all remaining COW staging extents and speculative post-EOF
> + * preallocations so that we don't leave inodes requiring inactivation
> + * cleanups during reclaim on a read-only mount. We must process every
> + * cached inode, so this requires a synchronous cache scan.
> + */
> + error = xfs_blockgc_free_space(mp, &icw);
> if (error) {
> xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> return error;
--
chandan
