On Tue, Feb 02, 2021 at 11:41:01AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> This is a regression test for a bug in the XFS implementation of
> FSSETXATTR. When we try to change a file's project id, the quota
> reservation code will update the incore quota reservations for delayed
> allocation blocks. Unfortunately, it does this before we finish
> validating all the FSSETXATTR parameters, which means that if we decide
> to bail out, we also fail to undo the incore changes.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> .gitignore | 1 +
> src/Makefile | 3 +-
> src/chprojid_fail.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/xfs/765 | 63 +++++++++++++++++++++++++++++++++++++
> tests/xfs/765.out | 3 ++
> tests/xfs/group | 1 +
> 6 files changed, 156 insertions(+), 1 deletion(-)
> create mode 100644 src/chprojid_fail.c
> create mode 100755 tests/xfs/765
> create mode 100644 tests/xfs/765.out
>
...
> diff --git a/src/chprojid_fail.c b/src/chprojid_fail.c
> new file mode 100644
> index 00000000..e7467372
> --- /dev/null
> +++ b/src/chprojid_fail.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Oracle. All Rights Reserved.
> + * Author: Darrick J. Wong <djwong@xxxxxxxxxx>
> + *
> + * Regression test for failing to undo delalloc quota reservations when
> + * changing project id and we fail some other FSSETXATTR validation.
> + */
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <linux/fs.h>
> +
> +static char zerobuf[65536];
> +
> +int
> +main(
> + int argc,
> + char *argv[])
> +{
> + struct fsxattr fa;
> + ssize_t sz;
> + int fd, ret;
> +
> + if (argc < 2) {
> + printf("Usage: %s filename\n", argv[0]);
> + return 1;
> + }
> +
> + fd = open(argv[1], O_CREAT | O_TRUNC | O_RDWR, 0600);
> + if (fd < 0) {
> + perror(argv[1]);
> + return 2;
> + }
> +
> + /* Zero the project id and the extent size hint. */
> + ret = ioctl(fd, FS_IOC_FSGETXATTR, &fa);
> + if (ret) {
> + perror("FSGETXATTR check file");
> + return 2;
> + }
> +
> + if (fa.fsx_projid != 0 || fa.fsx_extsize != 0) {
> + fa.fsx_projid = 0;
> + fa.fsx_extsize = 0;
> + ret = ioctl(fd, FS_IOC_FSSETXATTR, &fa);
> + if (ret) {
> + perror("FSSETXATTR zeroing");
> + return 2;
> + }
> + }
> +
> + /* Dirty a few kb of a file to create delalloc extents. */
> + sz = write(fd, zerobuf, sizeof(zerobuf));
> + if (sz != sizeof(zerobuf)) {
> + perror("delalloc write");
> + return 2;
> + }
> +
> + /*
> + * Fail to chprojid and set an extent size hint after we wrote the file.
> + */
Might be helpful to document exactly why this command should fail.
> + ret = ioctl(fd, FS_IOC_FSGETXATTR, &fa);
> + if (ret) {
> + perror("FSGETXATTR");
> + return 2;
> + }
> +
> + fa.fsx_projid = 23652;
> + fa.fsx_extsize = 2;
> + fa.fsx_xflags |= FS_XFLAG_EXTSIZE;
> +
> + ret = ioctl(fd, FS_IOC_FSSETXATTR, &fa);
> + if (ret) {
> + printf("FSSETXATTRR should fail: %s\n", strerror(errno));
> + return 0;
> + }
> +
> + /* Uhoh, that FSSETXATTR call should have failed! */
> + return 3;
> +}
> diff --git a/tests/xfs/765 b/tests/xfs/765
> new file mode 100755
> index 00000000..769b545b
> --- /dev/null
> +++ b/tests/xfs/765
> @@ -0,0 +1,63 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2021 Oracle. All Rights Reserved.
> +#
> +# FS QA Test No. 765
> +#
> +# Regression test for failing to undo delalloc quota reservations when changing
> +# project id but we fail some other part of FSSETXATTR validation. If we fail
> +# the test, we trip debugging assertions in dmesg.
> +#
> +# The appropriate XFS patch is:
> +# xfs: fix chown leaking delalloc quota blocks when fssetxattr fails
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> + cd /
> + rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/quota
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_require_xfs_debug
I agree with Zorro that we might as well just let this run with or
without debug mode..
> +_require_command "$FILEFRAG_PROG" filefrag
> +_require_test_program "chprojid_fail"
> +_require_quota
> +_require_scratch
> +
> +rm -f $seqres.full
> +
> +echo "Format filesystem" | tee -a $seqres.full
> +_scratch_mkfs > $seqres.full
> +_qmount_option 'prjquota'
> +_qmount
> +_require_prjquota $SCRATCH_DEV
> +
> +echo "Run test program"
> +$XFS_QUOTA_PROG -f -x -c 'report -ap' $SCRATCH_MNT >> $seqres.full
> +$here/src/chprojid_fail $SCRATCH_MNT/blah >> $seqres.full
> +res=$?
> +if [ $res -ne 0 ]; then
> + echo "chprojid_fail returned $res, expected 0"
> +fi
Can we just use the output from the test program as the golden test
output?
> +$XFS_QUOTA_PROG -f -x -c 'report -ap' $SCRATCH_MNT >> $seqres.full
> +$FILEFRAG_PROG -v $SCRATCH_MNT/blah >> $seqres.full
> +$FILEFRAG_PROG -v $SCRATCH_MNT/blah 2>&1 | grep -q delalloc || \
> + echo "file didn't get delalloc extents?"
This could technically cause a failure even if the test did the right
thing if writeback happens to occur, right? I suppose that's exceedingly
unlikely, but might at least be worth documenting in the event that
somebody has to deal with that scenario sometime in the future.
Brian
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/765.out b/tests/xfs/765.out
> new file mode 100644
> index 00000000..f44ba43e
> --- /dev/null
> +++ b/tests/xfs/765.out
> @@ -0,0 +1,3 @@
> +QA output created by 765
> +Format filesystem
> +Run test program
> diff --git a/tests/xfs/group b/tests/xfs/group
> index f406a9b9..fb78b0d7 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -544,6 +544,7 @@
> 762 auto quick rw scrub realtime
> 763 auto quick rw realtime
> 764 auto quick repair
> +765 auto quick quota
> 908 auto quick bigtime
> 909 auto quick bigtime quota
> 910 auto quick inobtcount
>
