All of lore.kernel.org
 help / color / mirror / Atom feed
* Error setting extent size on a directory
@ 2014-07-14  7:09 Iustin Pop
  2014-07-17  9:04 ` Christoph Hellwig
  0 siblings, 1 reply; 19+ messages in thread
From: Iustin Pop @ 2014-07-14  7:09 UTC (permalink / raw)
  To: xfs

Hi,

During the restore of a file system, a few directories failed to have
their extent size set:

xfsrestore: WARNING: attempt to set extended attributes (xflags 0x1080,
extsize = 0x800000, projid = 0x0) of a failed: Invalid argument

Since there were just a few, I've set the nodump flag (0x80 in flags
above) manually, which worked, but trying to set the extent size still
fails:
xfs_io .
xfs_io> extsize
[0] .
xfs_io> extsize 8m
xfs_io: XFS_IOC_FSSETXATTR .: Invalid argument

The xfsctl man page says that an extent size should be settable any time
on a directory, so why would this fail? Looking at the kernel sources,
I see a number of possible cases where EINVAL is returned:

- ip->i_d.di_nextents non-zero
- requested extend size bigger than MAXEXTLEN; not the case here
- extsize_fsb > mp->m_sb.sb_agblocks / 2; not the case
- fa->fsx_extsize % size - extent size not a multiple of FS block size;
  again not the case

So to me this reads as if the di_nextents check can also fail for a
directory which has extents, contradicting the man page. Which one needs
to be updated?

The question arises to if the extent size also applies, then, to
allocating extents for a directory - instead of just being inherited for
files (the man page says no).

thanks,
iustin

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Error setting extent size on a directory
  2014-07-14  7:09 Error setting extent size on a directory Iustin Pop
@ 2014-07-17  9:04 ` Christoph Hellwig
  2014-07-18 19:13   ` Iustin Pop
  0 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2014-07-17  9:04 UTC (permalink / raw)
  To: xfs

On Mon, Jul 14, 2014 at 09:09:13AM +0200, Iustin Pop wrote:
> The xfsctl man page says that an extent size should be settable any time
> on a directory, so why would this fail? Looking at the kernel sources,
> I see a number of possible cases where EINVAL is returned:

And no special casing for directories at all..

> So to me this reads as if the di_nextents check can also fail for a
> directory which has extents, contradicting the man page. Which one needs
> to be updated?
> 
> The question arises to if the extent size also applies, then, to
> allocating extents for a directory - instead of just being inherited for
> files (the man page says no).

We're not using the extent size hint on the directory itself.  So to
me it seems we just not check for already allocated blocks if we're
setting the extent size on a directory, but instead maybe make sure
the directory.  What's also a little odd is that we allow setting
the extent size on a directory even if the extent size inherit bit is
not set, which doesn't make much sense to me.

Do you want to prepare a patch to remove the check for directories?
At testcase for xfstests that ensures this works also would be highly
useful..

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Error setting extent size on a directory
  2014-07-17  9:04 ` Christoph Hellwig
@ 2014-07-18 19:13   ` Iustin Pop
  2014-08-28  4:22     ` [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Iustin Pop
  2014-08-28  4:24     ` [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour Iustin Pop
  0 siblings, 2 replies; 19+ messages in thread
From: Iustin Pop @ 2014-07-18 19:13 UTC (permalink / raw)
  To: xfs

On Thu, Jul 17, 2014 at 02:04:50AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 14, 2014 at 09:09:13AM +0200, Iustin Pop wrote:
> > The xfsctl man page says that an extent size should be settable any time
> > on a directory, so why would this fail? Looking at the kernel sources,
> > I see a number of possible cases where EINVAL is returned:
> 
> And no special casing for directories at all..

I was not sure if di_nextents is valid (tracks the extents number, and
hence can be non-zero for a directory), I'll take this as a
confirmation.

> > So to me this reads as if the di_nextents check can also fail for a
> > directory which has extents, contradicting the man page. Which one needs
> > to be updated?
> > 
> > The question arises to if the extent size also applies, then, to
> > allocating extents for a directory - instead of just being inherited for
> > files (the man page says no).
> 
> We're not using the extent size hint on the directory itself.

Aha, this is good to know.

> So to
> me it seems we just not check for already allocated blocks if we're
> setting the extent size on a directory, but instead maybe make sure
> the directory.

Not sure I parse that - do you mean we should either check for a
directory, or for zero extent count?

> What's also a little odd is that we allow setting
> the extent size on a directory even if the extent size inherit bit is
> not set, which doesn't make much sense to me.

Since the hint it is never used for directories, agreed it doesn't make
sense. Should this be an error (since I don't think warnings can be
reported).

> Do you want to prepare a patch to remove the check for directories?
> At testcase for xfstests that ensures this works also would be highly
> useful..

I'll try to. Is the tree against which I should sent the kernel patch at
git://oss.sgi.com/xfs/xfs?

thanks,
iustin

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
  2014-07-18 19:13   ` Iustin Pop
@ 2014-08-28  4:22     ` Iustin Pop
  2014-08-28  9:31       ` Dave Chinner
  2014-08-28  4:24     ` [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour Iustin Pop
  1 sibling, 1 reply; 19+ messages in thread
From: Iustin Pop @ 2014-08-28  4:22 UTC (permalink / raw)
  To: xfs; +Cc: hch, Iustin Pop

Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
targets as regular files: it refuses to change the extent size if
extents are allocated. This is wrong for directories, as there the
extent size is only used as a default for children.

The patch fixes this by only checking for allocated extents for regular
files; it leaves undefined what it means to set an extent size on a
non-regular, non-directory inode. Additionally, when setting a non-zero
extent size, the appropriate flags (EXTSIZE, respectively EXTSZINHERIT)
are enforced for regular files and directories.

Signed-off-by: Iustin Pop <iusty@k1024.org>
---
 A patch against xfstests to test for the fixed behaviour will follow
 shortly.

 fs/xfs/xfs_ioctl.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 8bc1bbc..5b9acd2 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1116,14 +1116,32 @@ xfs_ioctl_setattr(
 	}
 
 	if (mask & FSX_EXTSIZE) {
-		/*
-		 * Can't change extent size if any extents are allocated.
-		 */
-		if (ip->i_d.di_nextents &&
-		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
-		     fa->fsx_extsize)) {
-			code = XFS_ERROR(EINVAL);	/* EFBIG? */
-			goto error_return;
+		if (S_ISDIR(ip->i_d.di_mode)) {
+			/*
+			 * Enforce setting the EXTSZINHERIT flag when
+			 * a non-zero extent size has been specified.
+			 */
+			if (fa->fsx_extsize) {
+				fa->fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
+			}
+		} else if (S_ISREG(ip->i_d.di_mode)) {
+			/*
+			 * For a regular file, we can't change extent
+			 * size if any extents are allocated.
+			 */
+			if (ip->i_d.di_nextents &&
+			    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
+			     fa->fsx_extsize)) {
+				code = XFS_ERROR(EINVAL);	/* EFBIG? */
+				goto error_return;
+			}
+			/*
+			 * Enforce setting the EXTSIZE flag when
+			 * a non-zero extent size has been specified.
+			 */
+			if (fa->fsx_extsize) {
+				fa->fsx_xflags |= XFS_XFLAG_EXTSIZE;
+			}
 		}
 
 		/*
-- 
2.1.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour
  2014-07-18 19:13   ` Iustin Pop
  2014-08-28  4:22     ` [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Iustin Pop
@ 2014-08-28  4:24     ` Iustin Pop
  2014-08-28 10:16         ` Dave Chinner
  1 sibling, 1 reply; 19+ messages in thread
From: Iustin Pop @ 2014-08-28  4:24 UTC (permalink / raw)
  To: xfs; +Cc: hch, Iustin Pop

Add two tests that check for correct behaviour of XFS_IOC_FSSETXATTR:

- 307: check that extent size can always be set on a directory
- 308: check that setting a non-zero extent size directly via the ioctl
  sets the expected flags (EXTSIZE and EXTSZINHERIT)

Signed-off-by: Iustin Pop <iusty@k1024.org>
---
 tests/xfs/307     | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/307.out |  3 ++
 tests/xfs/308     | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/308.out |  3 ++
 tests/xfs/group   |  2 ++
 5 files changed, 193 insertions(+)
 create mode 100755 tests/xfs/307
 create mode 100644 tests/xfs/307.out
 create mode 100755 tests/xfs/308
 create mode 100644 tests/xfs/308.out

diff --git a/tests/xfs/307 b/tests/xfs/307
new file mode 100755
index 0000000..e8f3576
--- /dev/null
+++ b/tests/xfs/307
@@ -0,0 +1,87 @@
+#! /bin/bash
+# FS QA Test No. 307
+#
+# Test that setting extent size on directories work even for large
+# directories.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Google Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+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/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_supported_os Linux
+_require_test
+_require_scratch
+
+_scratch_mkfs_xfs >/dev/null 2>&1
+_scratch_mount
+
+small=$SCRATCH_MNT/small
+big=$SCRATCH_MNT/big
+
+# sanity check on a small directory
+mkdir $small
+# expect that an empty directory has no extents
+xfs_bmap $small | grep -q "no extents"
+# and that we can set an extent size on it
+xfs_io -c 'extsize 8m' $small
+# and finally check that the extent size update has taken place
+(cd $SCRATCH_MNT; xfs_io -c 'extsize' small)
+
+# now create a 'big' (with extents) directory
+mkdir $big
+idx=1
+while xfs_bmap $big | grep -q "no extents"; do
+	touch $big/$idx
+	idx=$((idx+1))
+	if [ "$idx" -gt 1048576 ]; then
+		# still no extents? giving up
+		echo "Can't make a directory to have extents even after 1M files" 1>&2
+		exit
+	fi
+done
+
+# expect that we can set the extent size on $big as well
+xfs_io -c 'extsize 8m' $big
+# and that it took effect
+(cd $SCRATCH_MNT; xfs_io -c 'extsize' big)
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/307.out b/tests/xfs/307.out
new file mode 100644
index 0000000..f825525
--- /dev/null
+++ b/tests/xfs/307.out
@@ -0,0 +1,3 @@
+QA output created by 307
+[8388608] small
+[8388608] big
diff --git a/tests/xfs/308 b/tests/xfs/308
new file mode 100755
index 0000000..7b43836
--- /dev/null
+++ b/tests/xfs/308
@@ -0,0 +1,98 @@
+#! /bin/bash
+# FS QA Test No. 308
+#
+# Test that setting extent size on files and directories (directly via
+# ioctl and not via xfs_io) sets the correct flags.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Google Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+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/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_supported_os Linux
+_require_test
+_require_scratch
+
+_scratch_mkfs_xfs >/dev/null 2>&1
+_scratch_mount
+
+file=$SCRATCH_MNT/file
+dir=$SCRATCH_MNT/dir
+cprog=$tmp.get_structs.c
+oprog=$tmp.get_structs
+
+touch $file
+mkdir $dir
+
+cat > $cprog << EOF
+#include <stdio.h>
+#include <xfs/xfs.h>
+
+int main(int argc, char **argv) {
+	struct fsxattr fa;
+	int fd = open(argv[1], O_RDONLY);
+	if (fd < 0) {
+		perror("open");
+		return 1;
+	}
+	fa.fsx_xflags = 0;
+	fa.fsx_extsize = 1048576 * 8;
+	int r = xfsctl(argv[1], fd, XFS_IOC_FSSETXATTR, &fa);
+	if (r < 0) {
+		perror("xfsctl");
+		return 1;
+	}
+	return 0;
+}
+EOF
+cc -o $oprog $cprog >> $seqres.full 2>&1 || \
+  _notrun "Could not compile test program (see end of $seqres.full)"
+
+$oprog $file
+$oprog $dir
+(cd $SCRATCH_MNT;
+ xfs_io -c 'lsattr' file;
+ xfs_io -c 'lsattr' dir)
+
+rm $file
+rmdir $dir
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/308.out b/tests/xfs/308.out
new file mode 100644
index 0000000..ef2d5fd
--- /dev/null
+++ b/tests/xfs/308.out
@@ -0,0 +1,3 @@
+QA output created by 308
+----------e--- file 
+-----------E-- dir 
diff --git a/tests/xfs/group b/tests/xfs/group
index 4d35df5..35f1e6a 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -197,3 +197,5 @@
 304 auto quick quota
 305 auto quota
 306 auto stress log metadata repair
+307 ioctl quick
+308 ioctl quick
-- 
2.1.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
  2014-08-28  4:22     ` [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Iustin Pop
@ 2014-08-28  9:31       ` Dave Chinner
  2014-08-28 22:34         ` Iustin Pop
  0 siblings, 1 reply; 19+ messages in thread
From: Dave Chinner @ 2014-08-28  9:31 UTC (permalink / raw)
  To: Iustin Pop; +Cc: hch, xfs

On Wed, Aug 27, 2014 at 09:22:53PM -0700, Iustin Pop wrote:
> Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
> targets as regular files: it refuses to change the extent size if
> extents are allocated. This is wrong for directories, as there the
> extent size is only used as a default for children.
> 
> The patch fixes this by only checking for allocated extents for regular
> files; it leaves undefined what it means to set an extent size on a
> non-regular, non-directory inode. Additionally, when setting a non-zero
> extent size, the appropriate flags (EXTSIZE, respectively EXTSZINHERIT)
> are enforced for regular files and directories.
> 
> Signed-off-by: Iustin Pop <iusty@k1024.org>
> ---
>  A patch against xfstests to test for the fixed behaviour will follow
>  shortly.
> 
>  fs/xfs/xfs_ioctl.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 8bc1bbc..5b9acd2 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1116,14 +1116,32 @@ xfs_ioctl_setattr(
>  	}
>  
>  	if (mask & FSX_EXTSIZE) {
> -		/*
> -		 * Can't change extent size if any extents are allocated.
> -		 */
> -		if (ip->i_d.di_nextents &&
> -		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> -		     fa->fsx_extsize)) {
> -			code = XFS_ERROR(EINVAL);	/* EFBIG? */
> -			goto error_return;

Doesn't apply to a current 3.17-rc1 tree. Can you update the patch?

> +		if (S_ISDIR(ip->i_d.di_mode)) {
> +			/*
> +			 * Enforce setting the EXTSZINHERIT flag when
> +			 * a non-zero extent size has been specified.
> +			 */
> +			if (fa->fsx_extsize) {
> +				fa->fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
> +			}
> +		} else if (S_ISREG(ip->i_d.di_mode)) {
> +			/*
> +			 * For a regular file, we can't change extent
> +			 * size if any extents are allocated.
> +			 */
> +			if (ip->i_d.di_nextents &&
> +			    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> +			     fa->fsx_extsize)) {
> +				code = XFS_ERROR(EINVAL);	/* EFBIG? */
> +				goto error_return;
> +			}
> +			/*
> +			 * Enforce setting the EXTSIZE flag when
> +			 * a non-zero extent size has been specified.
> +			 */
> +			if (fa->fsx_extsize) {
> +				fa->fsx_xflags |= XFS_XFLAG_EXTSIZE;
> +			}
>  		}

Hmmmm. That's not validating/enforcing the correct use of
XFS_XFLAG_EXTSIZE or XFS_XFLAG_EXTSZINHERIT, that's setting it
implicitly based on the type of inode.

If we are going to enforce this properly, then XFS_XFLAG_EXTSIZE is
only valid for a regular file, and XFS_XFLAG_EXTSZINHERIT is only
valid on a directory, and the flags on th einode should only be set
if the hint is not zero. i.e:

	if (mask & FSX_EXTSIZE) {
		error = -EINVAL;

		/* validate the flags are set appropriately */
		if ((fa->fsx_xflags & XFS_XFLAG_EXTSIZE) &&
		    !S_ISREG(ip->i_d.di_mode))
			goto error_return;
		if ((fa->fsx_xflags & XFS_XFLAG_EXTSZINHERIT) &&
		    !S_ISDIR(ip->i_d.di_mode))
			goto error_return;

		/* Can't change extent size on regular files with allocated extents */
		if (S_ISREG(ip->i_d.di_mode) && ip->i_d.di_nextents &&
		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
							fa->fsx_extsize))
			goto error_return;

		/* if the extent size is zero, clear the inode flags */
		if (fs->fsx_extsize == 0)
			fa->fsx_xflags &= ~(XFS_XFLAG_EXTSIZE | XFS_XFLAG_EXTSZINHERIT);
		} else {
			/* existing size check code */
			....
		}
	}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour
  2014-08-28  4:24     ` [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour Iustin Pop
@ 2014-08-28 10:16         ` Dave Chinner
  0 siblings, 0 replies; 19+ messages in thread
From: Dave Chinner @ 2014-08-28 10:16 UTC (permalink / raw)
  To: Iustin Pop; +Cc: xfs, hch, fstests

[cc fstests@vger.kernel.org]

On Wed, Aug 27, 2014 at 09:24:00PM -0700, Iustin Pop wrote:
> Add two tests that check for correct behaviour of XFS_IOC_FSSETXATTR:
> 
> - 307: check that extent size can always be set on a directory
> - 308: check that setting a non-zero extent size directly via the ioctl
>   sets the expected flags (EXTSIZE and EXTSZINHERIT)
> 
> Signed-off-by: Iustin Pop <iusty@k1024.org>

Minor stuff first:

- xfstests patches should be sent to fstests@vger.kernel.org now.
- can you pick the first unused numbers in the sequence for new
  tests (xfs/032, xfs/051) so I don't have to renumber them before
  applying them?
- a patch per new test - it makes it easier to review and apply as i
  don't have to split patches into multiple commits...

> diff --git a/tests/xfs/307 b/tests/xfs/307
> new file mode 100755
> index 0000000..e8f3576
> --- /dev/null
> +++ b/tests/xfs/307
> @@ -0,0 +1,87 @@
> +#! /bin/bash
> +# FS QA Test No. 307
> +#
> +# Test that setting extent size on directories work even for large
> +# directories.

What is a "large directory"? Wouldn't it be better to describe the
test as "Determine whether the extent size hint can be set on
directories with allocated extents correctly"?

> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2014 Google Inc.  All Rights Reserved.

Is that correct? It doesn't match the email address you sent this
from and I've never seen you post from a @google.com address.  I
always like to check that the copyright assignment is correct in
situations like this...

> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs
> +_supported_os Linux
> +_require_test

Not needed, doesn't use the test device.

> +_require_scratch
> +
> +_scratch_mkfs_xfs >/dev/null 2>&1
> +_scratch_mount
> +
> +small=$SCRATCH_MNT/small
> +big=$SCRATCH_MNT/big
> +
> +# sanity check on a small directory
> +mkdir $small
> +# expect that an empty directory has no extents
> +xfs_bmap $small | grep -q "no extents"

Better to use xfs_io directly and filter out $SCRATCH_MNT into the
golden output file like so:

$XFS_IO_PROG -c "bmap" $small | _filter_scratch

which will give:

SCRATCH_MNT/small: no extents

> +# and that we can set an extent size on it
> +xfs_io -c 'extsize 8m' $small

$XFS_IO_PROG

> +# and finally check that the extent size update has taken place
> +(cd $SCRATCH_MNT; xfs_io -c 'extsize' small)

$XFS_IO_PROG -c 'extsize' $small | _filter_scratch

> +# now create a 'big' (with extents) directory
> +mkdir $big
> +idx=1
> +while xfs_bmap $big | grep -q "no extents"; do
> +	touch $big/$idx
> +	idx=$((idx+1))
> +	if [ "$idx" -gt 1048576 ]; then
> +		# still no extents? giving up
> +		echo "Can't make a directory to have extents even after 1M files" 1>&2
> +		exit
> +	fi
> +done

urk. largest inode size is 2kb, which means at most it can fit less
than 100 dirents in the inode before spilling to extent form. So
just do a loop that creates 1000 files - there's no need to
overengineer the test code.

> +# expect that we can set the extent size on $big as well
> +xfs_io -c 'extsize 8m' $big

$XFS_IO_PROG

> +# and that it took effect
> +(cd $SCRATCH_MNT; xfs_io -c 'extsize' big)

$XFS_IO_PROG -c 'extsize' $big | _filter_scratch

> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/307.out b/tests/xfs/307.out
> new file mode 100644
> index 0000000..f825525
> --- /dev/null
> +++ b/tests/xfs/307.out
> @@ -0,0 +1,3 @@
> +QA output created by 307
> +[8388608] small
> +[8388608] big
> diff --git a/tests/xfs/308 b/tests/xfs/308
> new file mode 100755
> index 0000000..7b43836
> --- /dev/null
> +++ b/tests/xfs/308
> @@ -0,0 +1,98 @@
> +#! /bin/bash
> +# FS QA Test No. 308
> +#
> +# Test that setting extent size on files and directories (directly via
> +# ioctl and not via xfs_io) sets the correct flags.

xfs_io uses the ioctl directly - there's no need to write a special
c program to do this, especially as....

> +touch $file
> +mkdir $dir
> +
> +cat > $cprog << EOF
> +#include <stdio.h>
> +#include <xfs/xfs.h>
> +
> +int main(int argc, char **argv) {
> +	struct fsxattr fa;
> +	int fd = open(argv[1], O_RDONLY);
> +	if (fd < 0) {
> +		perror("open");
> +		return 1;
> +	}
> +	fa.fsx_xflags = 0;
> +	fa.fsx_extsize = 1048576 * 8;
> +	int r = xfsctl(argv[1], fd, XFS_IOC_FSSETXATTR, &fa);

.... that code is quite broken. Yes, it would work to set the
appropriate extent size flags with the kernel
changes you made, but that's not how this ioctl works.

i.e. it will cause any flag bits that are set on the inode to be
cleared and it's likely to fail on old kernels beacuse they have
very different behaviour to what your patch does.

IOWs, setting fsx_extsize without setting either XFS_XFLAG_EXTSIZE
or XFS_XFLAG_EXTSZINHERIT is bad practice. The kernel is left to
guess what you actually wanted to be done - the flags are supposed
to tell the kernel that the fsx_extsize value is meaningful, not the
other way around.

FWIW, the xfs_io code is *exactly* what applications should be
doing to set the extent size or any other inode flag. i.e:

1. stat the fd to determine the type.
2. populate the fsxattr structure with the existing inode flags
3. change the flags/fields of the fsxattr structure appropriately
4. set the new values to the inode.

i.e, from io/open.c:

static int
set_extsize(const char *path, int fd, long extsz)
{
        struct fsxattr  fsx;
        struct stat64   stat;

        if (fstat64(fd, &stat) < 0) {
                perror("fstat64");
                return 0;
        }
        if ((xfsctl(path, fd, XFS_IOC_FSGETXATTR, &fsx)) < 0) {
                printf("%s: XFS_IOC_FSGETXATTR %s: %s\n",
                        progname, path, strerror(errno));
                return 0;
        }

        if (S_ISREG(stat.st_mode)) {
                fsx.fsx_xflags |= XFS_XFLAG_EXTSIZE;
        } else if (S_ISDIR(stat.st_mode)) {
                fsx.fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
        } else {
                printf(_("invalid target file type - file %s\n"), path);
                return 0;
        }
        fsx.fsx_extsize = extsz;

        if ((xfsctl(path, fd, XFS_IOC_FSSETXATTR, &fsx)) < 0) {
                printf("%s: XFS_IOC_FSSETXATTR %s: %s\n",
                        progname, path, strerror(errno));
                return 0;
        }

        return 0;
}

We have xfs_io precisely so that we don't have to maintain random
test code like this throughout xfstests - do it once, do it right,
use it everywhere.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour
@ 2014-08-28 10:16         ` Dave Chinner
  0 siblings, 0 replies; 19+ messages in thread
From: Dave Chinner @ 2014-08-28 10:16 UTC (permalink / raw)
  To: Iustin Pop; +Cc: hch, fstests, xfs

[cc fstests@vger.kernel.org]

On Wed, Aug 27, 2014 at 09:24:00PM -0700, Iustin Pop wrote:
> Add two tests that check for correct behaviour of XFS_IOC_FSSETXATTR:
> 
> - 307: check that extent size can always be set on a directory
> - 308: check that setting a non-zero extent size directly via the ioctl
>   sets the expected flags (EXTSIZE and EXTSZINHERIT)
> 
> Signed-off-by: Iustin Pop <iusty@k1024.org>

Minor stuff first:

- xfstests patches should be sent to fstests@vger.kernel.org now.
- can you pick the first unused numbers in the sequence for new
  tests (xfs/032, xfs/051) so I don't have to renumber them before
  applying them?
- a patch per new test - it makes it easier to review and apply as i
  don't have to split patches into multiple commits...

> diff --git a/tests/xfs/307 b/tests/xfs/307
> new file mode 100755
> index 0000000..e8f3576
> --- /dev/null
> +++ b/tests/xfs/307
> @@ -0,0 +1,87 @@
> +#! /bin/bash
> +# FS QA Test No. 307
> +#
> +# Test that setting extent size on directories work even for large
> +# directories.

What is a "large directory"? Wouldn't it be better to describe the
test as "Determine whether the extent size hint can be set on
directories with allocated extents correctly"?

> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2014 Google Inc.  All Rights Reserved.

Is that correct? It doesn't match the email address you sent this
from and I've never seen you post from a @google.com address.  I
always like to check that the copyright assignment is correct in
situations like this...

> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs
> +_supported_os Linux
> +_require_test

Not needed, doesn't use the test device.

> +_require_scratch
> +
> +_scratch_mkfs_xfs >/dev/null 2>&1
> +_scratch_mount
> +
> +small=$SCRATCH_MNT/small
> +big=$SCRATCH_MNT/big
> +
> +# sanity check on a small directory
> +mkdir $small
> +# expect that an empty directory has no extents
> +xfs_bmap $small | grep -q "no extents"

Better to use xfs_io directly and filter out $SCRATCH_MNT into the
golden output file like so:

$XFS_IO_PROG -c "bmap" $small | _filter_scratch

which will give:

SCRATCH_MNT/small: no extents

> +# and that we can set an extent size on it
> +xfs_io -c 'extsize 8m' $small

$XFS_IO_PROG

> +# and finally check that the extent size update has taken place
> +(cd $SCRATCH_MNT; xfs_io -c 'extsize' small)

$XFS_IO_PROG -c 'extsize' $small | _filter_scratch

> +# now create a 'big' (with extents) directory
> +mkdir $big
> +idx=1
> +while xfs_bmap $big | grep -q "no extents"; do
> +	touch $big/$idx
> +	idx=$((idx+1))
> +	if [ "$idx" -gt 1048576 ]; then
> +		# still no extents? giving up
> +		echo "Can't make a directory to have extents even after 1M files" 1>&2
> +		exit
> +	fi
> +done

urk. largest inode size is 2kb, which means at most it can fit less
than 100 dirents in the inode before spilling to extent form. So
just do a loop that creates 1000 files - there's no need to
overengineer the test code.

> +# expect that we can set the extent size on $big as well
> +xfs_io -c 'extsize 8m' $big

$XFS_IO_PROG

> +# and that it took effect
> +(cd $SCRATCH_MNT; xfs_io -c 'extsize' big)

$XFS_IO_PROG -c 'extsize' $big | _filter_scratch

> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/307.out b/tests/xfs/307.out
> new file mode 100644
> index 0000000..f825525
> --- /dev/null
> +++ b/tests/xfs/307.out
> @@ -0,0 +1,3 @@
> +QA output created by 307
> +[8388608] small
> +[8388608] big
> diff --git a/tests/xfs/308 b/tests/xfs/308
> new file mode 100755
> index 0000000..7b43836
> --- /dev/null
> +++ b/tests/xfs/308
> @@ -0,0 +1,98 @@
> +#! /bin/bash
> +# FS QA Test No. 308
> +#
> +# Test that setting extent size on files and directories (directly via
> +# ioctl and not via xfs_io) sets the correct flags.

xfs_io uses the ioctl directly - there's no need to write a special
c program to do this, especially as....

> +touch $file
> +mkdir $dir
> +
> +cat > $cprog << EOF
> +#include <stdio.h>
> +#include <xfs/xfs.h>
> +
> +int main(int argc, char **argv) {
> +	struct fsxattr fa;
> +	int fd = open(argv[1], O_RDONLY);
> +	if (fd < 0) {
> +		perror("open");
> +		return 1;
> +	}
> +	fa.fsx_xflags = 0;
> +	fa.fsx_extsize = 1048576 * 8;
> +	int r = xfsctl(argv[1], fd, XFS_IOC_FSSETXATTR, &fa);

.... that code is quite broken. Yes, it would work to set the
appropriate extent size flags with the kernel
changes you made, but that's not how this ioctl works.

i.e. it will cause any flag bits that are set on the inode to be
cleared and it's likely to fail on old kernels beacuse they have
very different behaviour to what your patch does.

IOWs, setting fsx_extsize without setting either XFS_XFLAG_EXTSIZE
or XFS_XFLAG_EXTSZINHERIT is bad practice. The kernel is left to
guess what you actually wanted to be done - the flags are supposed
to tell the kernel that the fsx_extsize value is meaningful, not the
other way around.

FWIW, the xfs_io code is *exactly* what applications should be
doing to set the extent size or any other inode flag. i.e:

1. stat the fd to determine the type.
2. populate the fsxattr structure with the existing inode flags
3. change the flags/fields of the fsxattr structure appropriately
4. set the new values to the inode.

i.e, from io/open.c:

static int
set_extsize(const char *path, int fd, long extsz)
{
        struct fsxattr  fsx;
        struct stat64   stat;

        if (fstat64(fd, &stat) < 0) {
                perror("fstat64");
                return 0;
        }
        if ((xfsctl(path, fd, XFS_IOC_FSGETXATTR, &fsx)) < 0) {
                printf("%s: XFS_IOC_FSGETXATTR %s: %s\n",
                        progname, path, strerror(errno));
                return 0;
        }

        if (S_ISREG(stat.st_mode)) {
                fsx.fsx_xflags |= XFS_XFLAG_EXTSIZE;
        } else if (S_ISDIR(stat.st_mode)) {
                fsx.fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
        } else {
                printf(_("invalid target file type - file %s\n"), path);
                return 0;
        }
        fsx.fsx_extsize = extsz;

        if ((xfsctl(path, fd, XFS_IOC_FSSETXATTR, &fsx)) < 0) {
                printf("%s: XFS_IOC_FSSETXATTR %s: %s\n",
                        progname, path, strerror(errno));
                return 0;
        }

        return 0;
}

We have xfs_io precisely so that we don't have to maintain random
test code like this throughout xfstests - do it once, do it right,
use it everywhere.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour
  2014-08-28 10:16         ` Dave Chinner
@ 2014-08-28 22:28           ` Iustin Pop
  -1 siblings, 0 replies; 19+ messages in thread
From: Iustin Pop @ 2014-08-28 22:28 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Iustin Pop, xfs, hch, fstests

On Don, Aug 28, 2014 at 08:16:28 +1000, Dave Chinner wrote:
> [cc fstests@vger.kernel.org]
> 
> On Wed, Aug 27, 2014 at 09:24:00PM -0700, Iustin Pop wrote:
> > Add two tests that check for correct behaviour of XFS_IOC_FSSETXATTR:
> > 
> > - 307: check that extent size can always be set on a directory
> > - 308: check that setting a non-zero extent size directly via the ioctl
> >   sets the expected flags (EXTSIZE and EXTSZINHERIT)
> > 
> > Signed-off-by: Iustin Pop <iusty@k1024.org>
> 
> Minor stuff first:
> 
> - xfstests patches should be sent to fstests@vger.kernel.org now.

OK, will do. There was nothing written in the git repository's README,
hence I chose what I thought best.

> - can you pick the first unused numbers in the sequence for new
>   tests (xfs/032, xfs/051) so I don't have to renumber them before
>   applying them?

Will do - this is what the 'new' script did (or tried to do, as it
doesn't seem to work reliably).

> - a patch per new test - it makes it easier to review and apply as i
>   don't have to split patches into multiple commits...

Will do so when resending.

> > diff --git a/tests/xfs/307 b/tests/xfs/307
> > new file mode 100755
> > index 0000000..e8f3576
> > --- /dev/null
> > +++ b/tests/xfs/307
> > @@ -0,0 +1,87 @@
> > +#! /bin/bash
> > +# FS QA Test No. 307
> > +#
> > +# Test that setting extent size on directories work even for large
> > +# directories.
> 
> What is a "large directory"? Wouldn't it be better to describe the
> test as "Determine whether the extent size hint can be set on
> directories with allocated extents correctly"?

Indeed that's better, I'll update.

> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2014 Google Inc.  All Rights Reserved.
> 
> Is that correct? It doesn't match the email address you sent this
> from and I've never seen you post from a @google.com address.  I
> always like to check that the copyright assignment is correct in
> situations like this...

It is correct indeed (and thanks for double-checking). I prefer to send
my interactions/contributions done not as part of my job using my
personal address (hence I always wrote to xfs@ from the same address),
but even in that case, the copyright remains with my employer.

Just as a confirmation, sending this email from my @google.com address.

> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs xfs
> > +_supported_os Linux
> > +_require_test
> 
> Not needed, doesn't use the test device.

Ack.

> > +_require_scratch
> > +
> > +_scratch_mkfs_xfs >/dev/null 2>&1
> > +_scratch_mount
> > +
> > +small=$SCRATCH_MNT/small
> > +big=$SCRATCH_MNT/big
> > +
> > +# sanity check on a small directory
> > +mkdir $small
> > +# expect that an empty directory has no extents
> > +xfs_bmap $small | grep -q "no extents"
> 
> Better to use xfs_io directly and filter out $SCRATCH_MNT into the
> golden output file like so:
> 
> $XFS_IO_PROG -c "bmap" $small | _filter_scratch
> 
> which will give:
> 
> SCRATCH_MNT/small: no extents

Oh, nice, thanks!

> > +# and that we can set an extent size on it
> > +xfs_io -c 'extsize 8m' $small
> 
> $XFS_IO_PROG

Ack.

> > +# and finally check that the extent size update has taken place
> > +(cd $SCRATCH_MNT; xfs_io -c 'extsize' small)
> 
> $XFS_IO_PROG -c 'extsize' $small | _filter_scratch

Ack.

> > +# now create a 'big' (with extents) directory
> > +mkdir $big
> > +idx=1
> > +while xfs_bmap $big | grep -q "no extents"; do
> > +	touch $big/$idx
> > +	idx=$((idx+1))
> > +	if [ "$idx" -gt 1048576 ]; then
> > +		# still no extents? giving up
> > +		echo "Can't make a directory to have extents even after 1M files" 1>&2
> > +		exit
> > +	fi
> > +done
> 
> urk. largest inode size is 2kb, which means at most it can fit less
> than 100 dirents in the inode before spilling to extent form. So
> just do a loop that creates 1000 files - there's no need to
> overengineer the test code.

Will do.  It's fine to still check that the directory does have extents,
I hope?

> > +# expect that we can set the extent size on $big as well
> > +xfs_io -c 'extsize 8m' $big
> 
> $XFS_IO_PROG

Ack.

> > +# and that it took effect
> > +(cd $SCRATCH_MNT; xfs_io -c 'extsize' big)
> 
> $XFS_IO_PROG -c 'extsize' $big | _filter_scratch

Ack.

> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/307.out b/tests/xfs/307.out
> > new file mode 100644
> > index 0000000..f825525
> > --- /dev/null
> > +++ b/tests/xfs/307.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 307
> > +[8388608] small
> > +[8388608] big
> > diff --git a/tests/xfs/308 b/tests/xfs/308
> > new file mode 100755
> > index 0000000..7b43836
> > --- /dev/null
> > +++ b/tests/xfs/308
> > @@ -0,0 +1,98 @@
> > +#! /bin/bash
> > +# FS QA Test No. 308
> > +#
> > +# Test that setting extent size on files and directories (directly via
> > +# ioctl and not via xfs_io) sets the correct flags.
> 
> xfs_io uses the ioctl directly - there's no need to write a special
> c program to do this, especially as....
> 
> > +touch $file
> > +mkdir $dir
> > +
> > +cat > $cprog << EOF
> > +#include <stdio.h>
> > +#include <xfs/xfs.h>
> > +
> > +int main(int argc, char **argv) {
> > +	struct fsxattr fa;
> > +	int fd = open(argv[1], O_RDONLY);
> > +	if (fd < 0) {
> > +		perror("open");
> > +		return 1;
> > +	}
> > +	fa.fsx_xflags = 0;
> > +	fa.fsx_extsize = 1048576 * 8;
> > +	int r = xfsctl(argv[1], fd, XFS_IOC_FSSETXATTR, &fa);
> 
> .... that code is quite broken. Yes, it would work to set the
> appropriate extent size flags with the kernel
> changes you made, but that's not how this ioctl works.
> 
> i.e. it will cause any flag bits that are set on the inode to be
> cleared

Good point…

> and it's likely to fail on old kernels beacuse they have
> very different behaviour to what your patch does.

OK, that I didn't know. (Would you mind quickly explaining?)

> IOWs, setting fsx_extsize without setting either XFS_XFLAG_EXTSIZE
> or XFS_XFLAG_EXTSZINHERIT is bad practice. The kernel is left to
> guess what you actually wanted to be done - the flags are supposed
> to tell the kernel that the fsx_extsize value is meaningful, not the
> other way around.

See below.

> FWIW, the xfs_io code is *exactly* what applications should be
> doing to set the extent size or any other inode flag. i.e:
> 
> 1. stat the fd to determine the type.
> 2. populate the fsxattr structure with the existing inode flags
> 3. change the flags/fields of the fsxattr structure appropriately
> 4. set the new values to the inode.
> 
> i.e, from io/open.c:
> 
> static int
> set_extsize(const char *path, int fd, long extsz)
> {
>         struct fsxattr  fsx;
>         struct stat64   stat;
> 
>         if (fstat64(fd, &stat) < 0) {
>                 perror("fstat64");
>                 return 0;
>         }
>         if ((xfsctl(path, fd, XFS_IOC_FSGETXATTR, &fsx)) < 0) {
>                 printf("%s: XFS_IOC_FSGETXATTR %s: %s\n",
>                         progname, path, strerror(errno));
>                 return 0;
>         }
> 
>         if (S_ISREG(stat.st_mode)) {
>                 fsx.fsx_xflags |= XFS_XFLAG_EXTSIZE;
>         } else if (S_ISDIR(stat.st_mode)) {
>                 fsx.fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
>         } else {
>                 printf(_("invalid target file type - file %s\n"), path);
>                 return 0;
>         }
>         fsx.fsx_extsize = extsz;
> 
>         if ((xfsctl(path, fd, XFS_IOC_FSSETXATTR, &fsx)) < 0) {
>                 printf("%s: XFS_IOC_FSSETXATTR %s: %s\n",
>                         progname, path, strerror(errno));
>                 return 0;
>         }
> 
>         return 0;
> }
> 
> We have xfs_io precisely so that we don't have to maintain random
> test code like this throughout xfstests - do it once, do it right,
> use it everywhere.

I totally agree that xfs_io is what people should use, but I disagree on
the use of xfs_io in this particular test, let me explain why.

With 3.16-rc1 at least, it is possible to set fsx_extsize to a non-zero
value, without setting the flags (if you call the ioctl directly). Such
an inode  will be (unless I'm mistaken) flagged with a warning by
xfs_repair, which means that it's an invalid inode state.

So in my view, there's a kernel bug, in that it allows a user land
application to put an inode into a "wrong" state. This particular test
is designed to reproduce this kernel bug, so that the kernel fix can be
verified that is indeed a fix.

I can't use xfs_io here, because it will do the "right" thing - set the
EXTSIZE/EXTSZINHERIT flags correctly; but this is a test that the kernel
protects the inode invariants, not that xfs_io does so.

Alternatively, you could say that it's perfectly fine to have a non-zero
fsx_extsize, and that only when the flag is set it should be taken into
account; but I don't think that is what the rest of the code expects
today.

So, I'm fine either way, but I would to fix this so that all the code
agrees what the correct states for an inode are, and that the kernel
prevents user space from violating this assumption via a (documented)
ioctl. Just let me know which are the correct states.

Thanks for the feedback, and for such a quick reply.

iustin

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour
@ 2014-08-28 22:28           ` Iustin Pop
  0 siblings, 0 replies; 19+ messages in thread
From: Iustin Pop @ 2014-08-28 22:28 UTC (permalink / raw)
  To: Dave Chinner; +Cc: hch, Iustin Pop, fstests, xfs

On Don, Aug 28, 2014 at 08:16:28 +1000, Dave Chinner wrote:
> [cc fstests@vger.kernel.org]
> 
> On Wed, Aug 27, 2014 at 09:24:00PM -0700, Iustin Pop wrote:
> > Add two tests that check for correct behaviour of XFS_IOC_FSSETXATTR:
> > 
> > - 307: check that extent size can always be set on a directory
> > - 308: check that setting a non-zero extent size directly via the ioctl
> >   sets the expected flags (EXTSIZE and EXTSZINHERIT)
> > 
> > Signed-off-by: Iustin Pop <iusty@k1024.org>
> 
> Minor stuff first:
> 
> - xfstests patches should be sent to fstests@vger.kernel.org now.

OK, will do. There was nothing written in the git repository's README,
hence I chose what I thought best.

> - can you pick the first unused numbers in the sequence for new
>   tests (xfs/032, xfs/051) so I don't have to renumber them before
>   applying them?

Will do - this is what the 'new' script did (or tried to do, as it
doesn't seem to work reliably).

> - a patch per new test - it makes it easier to review and apply as i
>   don't have to split patches into multiple commits...

Will do so when resending.

> > diff --git a/tests/xfs/307 b/tests/xfs/307
> > new file mode 100755
> > index 0000000..e8f3576
> > --- /dev/null
> > +++ b/tests/xfs/307
> > @@ -0,0 +1,87 @@
> > +#! /bin/bash
> > +# FS QA Test No. 307
> > +#
> > +# Test that setting extent size on directories work even for large
> > +# directories.
> 
> What is a "large directory"? Wouldn't it be better to describe the
> test as "Determine whether the extent size hint can be set on
> directories with allocated extents correctly"?

Indeed that's better, I'll update.

> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2014 Google Inc.  All Rights Reserved.
> 
> Is that correct? It doesn't match the email address you sent this
> from and I've never seen you post from a @google.com address.  I
> always like to check that the copyright assignment is correct in
> situations like this...

It is correct indeed (and thanks for double-checking). I prefer to send
my interactions/contributions done not as part of my job using my
personal address (hence I always wrote to xfs@ from the same address),
but even in that case, the copyright remains with my employer.

Just as a confirmation, sending this email from my @google.com address.

> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs xfs
> > +_supported_os Linux
> > +_require_test
> 
> Not needed, doesn't use the test device.

Ack.

> > +_require_scratch
> > +
> > +_scratch_mkfs_xfs >/dev/null 2>&1
> > +_scratch_mount
> > +
> > +small=$SCRATCH_MNT/small
> > +big=$SCRATCH_MNT/big
> > +
> > +# sanity check on a small directory
> > +mkdir $small
> > +# expect that an empty directory has no extents
> > +xfs_bmap $small | grep -q "no extents"
> 
> Better to use xfs_io directly and filter out $SCRATCH_MNT into the
> golden output file like so:
> 
> $XFS_IO_PROG -c "bmap" $small | _filter_scratch
> 
> which will give:
> 
> SCRATCH_MNT/small: no extents

Oh, nice, thanks!

> > +# and that we can set an extent size on it
> > +xfs_io -c 'extsize 8m' $small
> 
> $XFS_IO_PROG

Ack.

> > +# and finally check that the extent size update has taken place
> > +(cd $SCRATCH_MNT; xfs_io -c 'extsize' small)
> 
> $XFS_IO_PROG -c 'extsize' $small | _filter_scratch

Ack.

> > +# now create a 'big' (with extents) directory
> > +mkdir $big
> > +idx=1
> > +while xfs_bmap $big | grep -q "no extents"; do
> > +	touch $big/$idx
> > +	idx=$((idx+1))
> > +	if [ "$idx" -gt 1048576 ]; then
> > +		# still no extents? giving up
> > +		echo "Can't make a directory to have extents even after 1M files" 1>&2
> > +		exit
> > +	fi
> > +done
> 
> urk. largest inode size is 2kb, which means at most it can fit less
> than 100 dirents in the inode before spilling to extent form. So
> just do a loop that creates 1000 files - there's no need to
> overengineer the test code.

Will do.  It's fine to still check that the directory does have extents,
I hope?

> > +# expect that we can set the extent size on $big as well
> > +xfs_io -c 'extsize 8m' $big
> 
> $XFS_IO_PROG

Ack.

> > +# and that it took effect
> > +(cd $SCRATCH_MNT; xfs_io -c 'extsize' big)
> 
> $XFS_IO_PROG -c 'extsize' $big | _filter_scratch

Ack.

> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/307.out b/tests/xfs/307.out
> > new file mode 100644
> > index 0000000..f825525
> > --- /dev/null
> > +++ b/tests/xfs/307.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 307
> > +[8388608] small
> > +[8388608] big
> > diff --git a/tests/xfs/308 b/tests/xfs/308
> > new file mode 100755
> > index 0000000..7b43836
> > --- /dev/null
> > +++ b/tests/xfs/308
> > @@ -0,0 +1,98 @@
> > +#! /bin/bash
> > +# FS QA Test No. 308
> > +#
> > +# Test that setting extent size on files and directories (directly via
> > +# ioctl and not via xfs_io) sets the correct flags.
> 
> xfs_io uses the ioctl directly - there's no need to write a special
> c program to do this, especially as....
> 
> > +touch $file
> > +mkdir $dir
> > +
> > +cat > $cprog << EOF
> > +#include <stdio.h>
> > +#include <xfs/xfs.h>
> > +
> > +int main(int argc, char **argv) {
> > +	struct fsxattr fa;
> > +	int fd = open(argv[1], O_RDONLY);
> > +	if (fd < 0) {
> > +		perror("open");
> > +		return 1;
> > +	}
> > +	fa.fsx_xflags = 0;
> > +	fa.fsx_extsize = 1048576 * 8;
> > +	int r = xfsctl(argv[1], fd, XFS_IOC_FSSETXATTR, &fa);
> 
> .... that code is quite broken. Yes, it would work to set the
> appropriate extent size flags with the kernel
> changes you made, but that's not how this ioctl works.
> 
> i.e. it will cause any flag bits that are set on the inode to be
> cleared

Good point…

> and it's likely to fail on old kernels beacuse they have
> very different behaviour to what your patch does.

OK, that I didn't know. (Would you mind quickly explaining?)

> IOWs, setting fsx_extsize without setting either XFS_XFLAG_EXTSIZE
> or XFS_XFLAG_EXTSZINHERIT is bad practice. The kernel is left to
> guess what you actually wanted to be done - the flags are supposed
> to tell the kernel that the fsx_extsize value is meaningful, not the
> other way around.

See below.

> FWIW, the xfs_io code is *exactly* what applications should be
> doing to set the extent size or any other inode flag. i.e:
> 
> 1. stat the fd to determine the type.
> 2. populate the fsxattr structure with the existing inode flags
> 3. change the flags/fields of the fsxattr structure appropriately
> 4. set the new values to the inode.
> 
> i.e, from io/open.c:
> 
> static int
> set_extsize(const char *path, int fd, long extsz)
> {
>         struct fsxattr  fsx;
>         struct stat64   stat;
> 
>         if (fstat64(fd, &stat) < 0) {
>                 perror("fstat64");
>                 return 0;
>         }
>         if ((xfsctl(path, fd, XFS_IOC_FSGETXATTR, &fsx)) < 0) {
>                 printf("%s: XFS_IOC_FSGETXATTR %s: %s\n",
>                         progname, path, strerror(errno));
>                 return 0;
>         }
> 
>         if (S_ISREG(stat.st_mode)) {
>                 fsx.fsx_xflags |= XFS_XFLAG_EXTSIZE;
>         } else if (S_ISDIR(stat.st_mode)) {
>                 fsx.fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
>         } else {
>                 printf(_("invalid target file type - file %s\n"), path);
>                 return 0;
>         }
>         fsx.fsx_extsize = extsz;
> 
>         if ((xfsctl(path, fd, XFS_IOC_FSSETXATTR, &fsx)) < 0) {
>                 printf("%s: XFS_IOC_FSSETXATTR %s: %s\n",
>                         progname, path, strerror(errno));
>                 return 0;
>         }
> 
>         return 0;
> }
> 
> We have xfs_io precisely so that we don't have to maintain random
> test code like this throughout xfstests - do it once, do it right,
> use it everywhere.

I totally agree that xfs_io is what people should use, but I disagree on
the use of xfs_io in this particular test, let me explain why.

With 3.16-rc1 at least, it is possible to set fsx_extsize to a non-zero
value, without setting the flags (if you call the ioctl directly). Such
an inode  will be (unless I'm mistaken) flagged with a warning by
xfs_repair, which means that it's an invalid inode state.

So in my view, there's a kernel bug, in that it allows a user land
application to put an inode into a "wrong" state. This particular test
is designed to reproduce this kernel bug, so that the kernel fix can be
verified that is indeed a fix.

I can't use xfs_io here, because it will do the "right" thing - set the
EXTSIZE/EXTSZINHERIT flags correctly; but this is a test that the kernel
protects the inode invariants, not that xfs_io does so.

Alternatively, you could say that it's perfectly fine to have a non-zero
fsx_extsize, and that only when the flag is set it should be taken into
account; but I don't think that is what the rest of the code expects
today.

So, I'm fine either way, but I would to fix this so that all the code
agrees what the correct states for an inode are, and that the kernel
prevents user space from violating this assumption via a (documented)
ioctl. Just let me know which are the correct states.

Thanks for the feedback, and for such a quick reply.

iustin

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
  2014-08-28  9:31       ` Dave Chinner
@ 2014-08-28 22:34         ` Iustin Pop
  2014-08-29  0:46           ` Dave Chinner
  0 siblings, 1 reply; 19+ messages in thread
From: Iustin Pop @ 2014-08-28 22:34 UTC (permalink / raw)
  To: Dave Chinner; +Cc: hch, Iustin Pop, xfs

On Don, Aug 28, 2014 at 07:31:58 +1000, Dave Chinner wrote:
> On Wed, Aug 27, 2014 at 09:22:53PM -0700, Iustin Pop wrote:
> > Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
> > targets as regular files: it refuses to change the extent size if
> > extents are allocated. This is wrong for directories, as there the
> > extent size is only used as a default for children.
> > 
> > The patch fixes this by only checking for allocated extents for regular
> > files; it leaves undefined what it means to set an extent size on a
> > non-regular, non-directory inode. Additionally, when setting a non-zero
> > extent size, the appropriate flags (EXTSIZE, respectively EXTSZINHERIT)
> > are enforced for regular files and directories.
> > 
> > Signed-off-by: Iustin Pop <iusty@k1024.org>
> > ---
> >  A patch against xfstests to test for the fixed behaviour will follow
> >  shortly.
> > 
> >  fs/xfs/xfs_ioctl.c | 34 ++++++++++++++++++++++++++--------
> >  1 file changed, 26 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 8bc1bbc..5b9acd2 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1116,14 +1116,32 @@ xfs_ioctl_setattr(
> >  	}
> >  
> >  	if (mask & FSX_EXTSIZE) {
> > -		/*
> > -		 * Can't change extent size if any extents are allocated.
> > -		 */
> > -		if (ip->i_d.di_nextents &&
> > -		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> > -		     fa->fsx_extsize)) {
> > -			code = XFS_ERROR(EINVAL);	/* EFBIG? */
> > -			goto error_return;
> 
> Doesn't apply to a current 3.17-rc1 tree. Can you update the patch?

Will do, thanks. Is it correct to use git://oss.sgi.com/xfs/xfs, master
branch as what I should rebase it on top of?

> > +		if (S_ISDIR(ip->i_d.di_mode)) {
> > +			/*
> > +			 * Enforce setting the EXTSZINHERIT flag when
> > +			 * a non-zero extent size has been specified.
> > +			 */
> > +			if (fa->fsx_extsize) {
> > +				fa->fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
> > +			}
> > +		} else if (S_ISREG(ip->i_d.di_mode)) {
> > +			/*
> > +			 * For a regular file, we can't change extent
> > +			 * size if any extents are allocated.
> > +			 */
> > +			if (ip->i_d.di_nextents &&
> > +			    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> > +			     fa->fsx_extsize)) {
> > +				code = XFS_ERROR(EINVAL);	/* EFBIG? */
> > +				goto error_return;
> > +			}
> > +			/*
> > +			 * Enforce setting the EXTSIZE flag when
> > +			 * a non-zero extent size has been specified.
> > +			 */
> > +			if (fa->fsx_extsize) {
> > +				fa->fsx_xflags |= XFS_XFLAG_EXTSIZE;
> > +			}
> >  		}
> 
> Hmmmm. That's not validating/enforcing the correct use of
> XFS_XFLAG_EXTSIZE or XFS_XFLAG_EXTSZINHERIT, that's setting it
> implicitly based on the type of inode.
> 
> If we are going to enforce this properly, then XFS_XFLAG_EXTSIZE is
> only valid for a regular file, and XFS_XFLAG_EXTSZINHERIT is only
> valid on a directory, and the flags on th einode should only be set
> if the hint is not zero. i.e:
> 
> 	if (mask & FSX_EXTSIZE) {
> 		error = -EINVAL;
> 
> 		/* validate the flags are set appropriately */
> 		if ((fa->fsx_xflags & XFS_XFLAG_EXTSIZE) &&
> 		    !S_ISREG(ip->i_d.di_mode))
> 			goto error_return;
> 		if ((fa->fsx_xflags & XFS_XFLAG_EXTSZINHERIT) &&
> 		    !S_ISDIR(ip->i_d.di_mode))
> 			goto error_return;

This only prevents invalid flags, but doesn't prevent setting a non-zero
extent_size without the appropriate flag. We already are having this
discussion in the other patch, so I'll defer this to that.

But aside from that, the current code that actually applies the flag
changes (xfs_set_diflags) implicitly drops invalid flags - that's why I
didn't bother to validate the flags here. If we want to do validation,
maybe we should validate (with error, not silently) in that function,
instead of here?

thanks,
iustin

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
  2014-08-28 22:34         ` Iustin Pop
@ 2014-08-29  0:46           ` Dave Chinner
  2014-12-04  4:14             ` Iustin Pop
  0 siblings, 1 reply; 19+ messages in thread
From: Dave Chinner @ 2014-08-29  0:46 UTC (permalink / raw)
  To: Iustin Pop; +Cc: hch, Iustin Pop, xfs

On Thu, Aug 28, 2014 at 03:34:14PM -0700, Iustin Pop wrote:
> On Don, Aug 28, 2014 at 07:31:58 +1000, Dave Chinner wrote:
> > On Wed, Aug 27, 2014 at 09:22:53PM -0700, Iustin Pop wrote:
> > > Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
> > > targets as regular files: it refuses to change the extent size if
> > > extents are allocated. This is wrong for directories, as there the
> > > extent size is only used as a default for children.
> > > 
> > > The patch fixes this by only checking for allocated extents for regular
> > > files; it leaves undefined what it means to set an extent size on a
> > > non-regular, non-directory inode. Additionally, when setting a non-zero
> > > extent size, the appropriate flags (EXTSIZE, respectively EXTSZINHERIT)
> > > are enforced for regular files and directories.
> > > 
> > > Signed-off-by: Iustin Pop <iusty@k1024.org>
> > > ---
> > >  A patch against xfstests to test for the fixed behaviour will follow
> > >  shortly.
> > > 
> > >  fs/xfs/xfs_ioctl.c | 34 ++++++++++++++++++++++++++--------
> > >  1 file changed, 26 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > index 8bc1bbc..5b9acd2 100644
> > > --- a/fs/xfs/xfs_ioctl.c
> > > +++ b/fs/xfs/xfs_ioctl.c
> > > @@ -1116,14 +1116,32 @@ xfs_ioctl_setattr(
> > >  	}
> > >  
> > >  	if (mask & FSX_EXTSIZE) {
> > > -		/*
> > > -		 * Can't change extent size if any extents are allocated.
> > > -		 */
> > > -		if (ip->i_d.di_nextents &&
> > > -		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> > > -		     fa->fsx_extsize)) {
> > > -			code = XFS_ERROR(EINVAL);	/* EFBIG? */
> > > -			goto error_return;
> > 
> > Doesn't apply to a current 3.17-rc1 tree. Can you update the patch?
> 
> Will do, thanks. Is it correct to use git://oss.sgi.com/xfs/xfs, master
> branch as what I should rebase it on top of?

I'm moving off oss.sgi.com. I'll keep the trees up to date for a
while, but the master kernel tree I'm using now is:

git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git master

> > > +		if (S_ISDIR(ip->i_d.di_mode)) {
> > > +			/*
> > > +			 * Enforce setting the EXTSZINHERIT flag when
> > > +			 * a non-zero extent size has been specified.
> > > +			 */
> > > +			if (fa->fsx_extsize) {
> > > +				fa->fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
> > > +			}
> > > +		} else if (S_ISREG(ip->i_d.di_mode)) {
> > > +			/*
> > > +			 * For a regular file, we can't change extent
> > > +			 * size if any extents are allocated.
> > > +			 */
> > > +			if (ip->i_d.di_nextents &&
> > > +			    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
> > > +			     fa->fsx_extsize)) {
> > > +				code = XFS_ERROR(EINVAL);	/* EFBIG? */
> > > +				goto error_return;
> > > +			}
> > > +			/*
> > > +			 * Enforce setting the EXTSIZE flag when
> > > +			 * a non-zero extent size has been specified.
> > > +			 */
> > > +			if (fa->fsx_extsize) {
> > > +				fa->fsx_xflags |= XFS_XFLAG_EXTSIZE;
> > > +			}
> > >  		}
> > 
> > Hmmmm. That's not validating/enforcing the correct use of
> > XFS_XFLAG_EXTSIZE or XFS_XFLAG_EXTSZINHERIT, that's setting it
> > implicitly based on the type of inode.
> > 
> > If we are going to enforce this properly, then XFS_XFLAG_EXTSIZE is
> > only valid for a regular file, and XFS_XFLAG_EXTSZINHERIT is only
> > valid on a directory, and the flags on th einode should only be set
> > if the hint is not zero. i.e:
> > 
> > 	if (mask & FSX_EXTSIZE) {
> > 		error = -EINVAL;
> > 
> > 		/* validate the flags are set appropriately */
> > 		if ((fa->fsx_xflags & XFS_XFLAG_EXTSIZE) &&
> > 		    !S_ISREG(ip->i_d.di_mode))
> > 			goto error_return;
> > 		if ((fa->fsx_xflags & XFS_XFLAG_EXTSZINHERIT) &&
> > 		    !S_ISDIR(ip->i_d.di_mode))
> > 			goto error_return;
> 
> This only prevents invalid flags, but doesn't prevent setting a non-zero
> extent_size without the appropriate flag. We already are having this
> discussion in the other patch, so I'll defer this to that.

So take the previous code I cleared the flags on extsize == 0, and
do this:

-	if (mask & FSX_EXTSIZE)
+	if (fa->fsx_flags & (XFS_XFLAG_EXTSIZE | XFS_XFLAG_EXTSZINHERIT)) {
		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
+		if (ip->i_d.di_extsize == 0)
+			fa->fsx_flags &= ~(XFS_XFLAG_EXTSIZE | XFS_XFLAG_EXTSZINHERIT);
+	}

The way that mask gets used is just nasty, IMO. We could clean that
code up a lot with a bit of factoring.

> But aside from that, the current code that actually applies the flag
> changes (xfs_set_diflags) implicitly drops invalid flags - that's why I
> didn't bother to validate the flags here.

Which points out that the flag validation code is broken, yes?

> If we want to do validation,
> maybe we should validate (with error, not silently) in that function,
> instead of here?

No. We need to have a clear separation of user input validation from
the modification of the inode state, because if we abort
modification during a transaction due to a validation failure after
modifying the inode, then we have to abort the dirty transaction and
that will cause a filesystem shutdown. Which would then be called a
"user level DOS" and hence we must maintain the separation of
validation from modification that we already have...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour
  2014-08-28 22:28           ` Iustin Pop
@ 2014-08-29  2:52             ` Dave Chinner
  -1 siblings, 0 replies; 19+ messages in thread
From: Dave Chinner @ 2014-08-29  2:52 UTC (permalink / raw)
  To: Iustin Pop; +Cc: Iustin Pop, xfs, hch, fstests

On Thu, Aug 28, 2014 at 03:28:54PM -0700, Iustin Pop wrote:
> On Don, Aug 28, 2014 at 08:16:28 +1000, Dave Chinner wrote:
> > [cc fstests@vger.kernel.org]
> > 
> > On Wed, Aug 27, 2014 at 09:24:00PM -0700, Iustin Pop wrote:
> > > Add two tests that check for correct behaviour of XFS_IOC_FSSETXATTR:
> > > 
> > > - 307: check that extent size can always be set on a directory
> > > - 308: check that setting a non-zero extent size directly via the ioctl
> > >   sets the expected flags (EXTSIZE and EXTSZINHERIT)
> > > 
> > > Signed-off-by: Iustin Pop <iusty@k1024.org>
> > 
> > Minor stuff first:
> > 
> > - xfstests patches should be sent to fstests@vger.kernel.org now.
> 
> OK, will do. There was nothing written in the git repository's README,
> hence I chose what I thought best.

Documentation always needs updating, and stuff is in the process of
being moved around.

> > > +# Copyright (c) 2014 Google Inc.  All Rights Reserved.
> > 
> > Is that correct? It doesn't match the email address you sent this
> > from and I've never seen you post from a @google.com address.  I
> > always like to check that the copyright assignment is correct in
> > situations like this...
> 
> It is correct indeed (and thanks for double-checking). I prefer to send
> my interactions/contributions done not as part of my job using my
> personal address (hence I always wrote to xfs@ from the same address),
> but even in that case, the copyright remains with my employer.
> 
> Just as a confirmation, sending this email from my @google.com address.

Thanks, I'll know in future ;)

> > > +# now create a 'big' (with extents) directory
> > > +mkdir $big
> > > +idx=1
> > > +while xfs_bmap $big | grep -q "no extents"; do
> > > +	touch $big/$idx
> > > +	idx=$((idx+1))
> > > +	if [ "$idx" -gt 1048576 ]; then
> > > +		# still no extents? giving up
> > > +		echo "Can't make a directory to have extents even after 1M files" 1>&2
> > > +		exit
> > > +	fi
> > > +done
> > 
> > urk. largest inode size is 2kb, which means at most it can fit less
> > than 100 dirents in the inode before spilling to extent form. So
> > just do a loop that creates 1000 files - there's no need to
> > overengineer the test code.
> 
> Will do.  It's fine to still check that the directory does have extents,
> I hope?

$XFS_IO_PROG -c 'bmap -vp' $big | _filter_bmap

> > > +int main(int argc, char **argv) {
> > > +	struct fsxattr fa;
> > > +	int fd = open(argv[1], O_RDONLY);
> > > +	if (fd < 0) {
> > > +		perror("open");
> > > +		return 1;
> > > +	}
> > > +	fa.fsx_xflags = 0;
> > > +	fa.fsx_extsize = 1048576 * 8;
> > > +	int r = xfsctl(argv[1], fd, XFS_IOC_FSSETXATTR, &fa);
> > 
> > .... that code is quite broken. Yes, it would work to set the
> > appropriate extent size flags with the kernel
> > changes you made, but that's not how this ioctl works.
> > 
> > i.e. it will cause any flag bits that are set on the inode to be
> > cleared
> 
> Good point…
> 
> > and it's likely to fail on old kernels beacuse they have
> > very different behaviour to what your patch does.
> 
> OK, that I didn't know. (Would you mind quickly explaining?)

The extsize hint on the inode is not used by the kernel code unless
the XFS_XFLAG_EXTSIZE is also set. So existing kernels may set the
inode extszhint field, but the code will ignore it because the flags
didn't get set on the inode. See xfs_get_extsz_hint().

With your kernel changes, the above *invalid* code will result in
the flags being set on the inode, and so there's a change of
behaviour from "old kernel, does not trigger extsz behaviour" to
"new kernel, extsz behaviour is invoked".

> > We have xfs_io precisely so that we don't have to maintain random
> > test code like this throughout xfstests - do it once, do it right,
> > use it everywhere.
> 
> I totally agree that xfs_io is what people should use, but I disagree on
> the use of xfs_io in this particular test, let me explain why.
> 
> With 3.16-rc1 at least, it is possible to set fsx_extsize to a non-zero
> value, without setting the flags (if you call the ioctl directly). Such
> an inode  will be (unless I'm mistaken) flagged with a warning by
> xfs_repair, which means that it's an invalid inode state.

Yes, since this commit bd5683f ("xfs_repair: validate inode di_flags
field") in 2011 repair will flag that the hint is non-zero but the
correct flags are not set. Likewise it will warn if the wrong flags
are set. You can get wrong flags set on the inode in many ways, but
historically the kernel and repair utilities haven't cared.

> So in my view, there's a kernel bug, in that it allows a user land
> application to put an inode into a "wrong" state. This particular test
> is designed to reproduce this kernel bug, so that the kernel fix can be
> verified that is indeed a fix.

Yes this is not what xfstests is for. If we fix an API bug, there's
little value to testing forever that the API is fixed. What we are
trying to cover is that when the parameters are set correctly that
the behaviour is correct.

If you want to do "is the API validating user input correctly"
testing then that's what trinity is for. Dave Jones has long wanted
to get better ioctl coverage for filesystem specific operations, so
I'd suggest that this is the avenue for testing whether an API
behaves correctly and catches all invalid input.

Then we get to fix all the problems that trinity causes, just like
the mm folk have been doing for the past couple of years...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour
@ 2014-08-29  2:52             ` Dave Chinner
  0 siblings, 0 replies; 19+ messages in thread
From: Dave Chinner @ 2014-08-29  2:52 UTC (permalink / raw)
  To: Iustin Pop; +Cc: hch, Iustin Pop, fstests, xfs

On Thu, Aug 28, 2014 at 03:28:54PM -0700, Iustin Pop wrote:
> On Don, Aug 28, 2014 at 08:16:28 +1000, Dave Chinner wrote:
> > [cc fstests@vger.kernel.org]
> > 
> > On Wed, Aug 27, 2014 at 09:24:00PM -0700, Iustin Pop wrote:
> > > Add two tests that check for correct behaviour of XFS_IOC_FSSETXATTR:
> > > 
> > > - 307: check that extent size can always be set on a directory
> > > - 308: check that setting a non-zero extent size directly via the ioctl
> > >   sets the expected flags (EXTSIZE and EXTSZINHERIT)
> > > 
> > > Signed-off-by: Iustin Pop <iusty@k1024.org>
> > 
> > Minor stuff first:
> > 
> > - xfstests patches should be sent to fstests@vger.kernel.org now.
> 
> OK, will do. There was nothing written in the git repository's README,
> hence I chose what I thought best.

Documentation always needs updating, and stuff is in the process of
being moved around.

> > > +# Copyright (c) 2014 Google Inc.  All Rights Reserved.
> > 
> > Is that correct? It doesn't match the email address you sent this
> > from and I've never seen you post from a @google.com address.  I
> > always like to check that the copyright assignment is correct in
> > situations like this...
> 
> It is correct indeed (and thanks for double-checking). I prefer to send
> my interactions/contributions done not as part of my job using my
> personal address (hence I always wrote to xfs@ from the same address),
> but even in that case, the copyright remains with my employer.
> 
> Just as a confirmation, sending this email from my @google.com address.

Thanks, I'll know in future ;)

> > > +# now create a 'big' (with extents) directory
> > > +mkdir $big
> > > +idx=1
> > > +while xfs_bmap $big | grep -q "no extents"; do
> > > +	touch $big/$idx
> > > +	idx=$((idx+1))
> > > +	if [ "$idx" -gt 1048576 ]; then
> > > +		# still no extents? giving up
> > > +		echo "Can't make a directory to have extents even after 1M files" 1>&2
> > > +		exit
> > > +	fi
> > > +done
> > 
> > urk. largest inode size is 2kb, which means at most it can fit less
> > than 100 dirents in the inode before spilling to extent form. So
> > just do a loop that creates 1000 files - there's no need to
> > overengineer the test code.
> 
> Will do.  It's fine to still check that the directory does have extents,
> I hope?

$XFS_IO_PROG -c 'bmap -vp' $big | _filter_bmap

> > > +int main(int argc, char **argv) {
> > > +	struct fsxattr fa;
> > > +	int fd = open(argv[1], O_RDONLY);
> > > +	if (fd < 0) {
> > > +		perror("open");
> > > +		return 1;
> > > +	}
> > > +	fa.fsx_xflags = 0;
> > > +	fa.fsx_extsize = 1048576 * 8;
> > > +	int r = xfsctl(argv[1], fd, XFS_IOC_FSSETXATTR, &fa);
> > 
> > .... that code is quite broken. Yes, it would work to set the
> > appropriate extent size flags with the kernel
> > changes you made, but that's not how this ioctl works.
> > 
> > i.e. it will cause any flag bits that are set on the inode to be
> > cleared
> 
> Good point…
> 
> > and it's likely to fail on old kernels beacuse they have
> > very different behaviour to what your patch does.
> 
> OK, that I didn't know. (Would you mind quickly explaining?)

The extsize hint on the inode is not used by the kernel code unless
the XFS_XFLAG_EXTSIZE is also set. So existing kernels may set the
inode extszhint field, but the code will ignore it because the flags
didn't get set on the inode. See xfs_get_extsz_hint().

With your kernel changes, the above *invalid* code will result in
the flags being set on the inode, and so there's a change of
behaviour from "old kernel, does not trigger extsz behaviour" to
"new kernel, extsz behaviour is invoked".

> > We have xfs_io precisely so that we don't have to maintain random
> > test code like this throughout xfstests - do it once, do it right,
> > use it everywhere.
> 
> I totally agree that xfs_io is what people should use, but I disagree on
> the use of xfs_io in this particular test, let me explain why.
> 
> With 3.16-rc1 at least, it is possible to set fsx_extsize to a non-zero
> value, without setting the flags (if you call the ioctl directly). Such
> an inode  will be (unless I'm mistaken) flagged with a warning by
> xfs_repair, which means that it's an invalid inode state.

Yes, since this commit bd5683f ("xfs_repair: validate inode di_flags
field") in 2011 repair will flag that the hint is non-zero but the
correct flags are not set. Likewise it will warn if the wrong flags
are set. You can get wrong flags set on the inode in many ways, but
historically the kernel and repair utilities haven't cared.

> So in my view, there's a kernel bug, in that it allows a user land
> application to put an inode into a "wrong" state. This particular test
> is designed to reproduce this kernel bug, so that the kernel fix can be
> verified that is indeed a fix.

Yes this is not what xfstests is for. If we fix an API bug, there's
little value to testing forever that the API is fixed. What we are
trying to cover is that when the parameters are set correctly that
the behaviour is correct.

If you want to do "is the API validating user input correctly"
testing then that's what trinity is for. Dave Jones has long wanted
to get better ioctl coverage for filesystem specific operations, so
I'd suggest that this is the avenue for testing whether an API
behaves correctly and catches all invalid input.

Then we get to fix all the problems that trinity causes, just like
the mm folk have been doing for the past couple of years...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
  2014-08-29  0:46           ` Dave Chinner
@ 2014-12-04  4:14             ` Iustin Pop
  2014-12-05  0:11               ` Dave Chinner
  0 siblings, 1 reply; 19+ messages in thread
From: Iustin Pop @ 2014-12-04  4:14 UTC (permalink / raw)
  To: david; +Cc: Iustin Pop, xfs

Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
targets as regular files: it refuses to change the extent size if
extents are allocated. This is wrong for directories, as there the
extent size is only used as a default for children.

The patch fixes this issue and improves validation of flag
combinations:

- only disallow extent size changes after extents have been allocated
  for regular files
- only allow XFS_XFLAG_EXTSIZE for regular files
- only allow XFS_XFLAG_EXTSZINHERIT for directories
- automatically clear the flags if the extent size is zero

Thanks to Dave Chinner for guidance on the proper fix for this issue.

Signed-off-by: Iustin Pop <iustin@k1024.org>
---
 Trying to revive this fix. Note that this patch is on top of
 git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git master, which
 seems to have no commits since Oct 26; let me know if I should rebase it on
 top of something else.

 fs/xfs/xfs_ioctl.c | 59 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 24c926b..67e3ab1 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1113,34 +1113,49 @@ xfs_ioctl_setattr(
 	}
 
 	if (mask & FSX_EXTSIZE) {
-		/*
-		 * Can't change extent size if any extents are allocated.
+		code = -EINVAL;
+
+		/* Validate the flags are set appropriately per the
+		 * inode type.
 		 */
-		if (ip->i_d.di_nextents &&
-		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
-		     fa->fsx_extsize)) {
-			code = -EINVAL;	/* EFBIG? */
+		if ((fa->fsx_xflags & XFS_XFLAG_EXTSIZE) &&
+		    !S_ISREG(ip->i_d.di_mode))
+			goto error_return;
+		if ((fa->fsx_xflags & XFS_XFLAG_EXTSZINHERIT) &&
+		    !S_ISDIR(ip->i_d.di_mode))
 			goto error_return;
-		}
 
 		/*
-		 * Extent size must be a multiple of the appropriate block
-		 * size, if set at all. It must also be smaller than the
-		 * maximum extent size supported by the filesystem.
-		 *
-		 * Also, for non-realtime files, limit the extent size hint to
-		 * half the size of the AGs in the filesystem so alignment
-		 * doesn't result in extents larger than an AG.
+		 * Dissalow changing extent size on regular files with
+		 * allocated extents.
 		 */
-		if (fa->fsx_extsize != 0) {
+		if (S_ISREG(ip->i_d.di_mode) && ip->i_d.di_nextents &&
+		    ((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) !=
+		     fa->fsx_extsize))
+			goto error_return;
+
+		/* If the extent size is zero, clear the inode flags. */
+		if (fa->fsx_extsize == 0) {
+			fa->fsx_xflags &= ~(XFS_XFLAG_EXTSIZE |
+					    XFS_XFLAG_EXTSZINHERIT);
+		} else {
+			/*
+			 * Extent size must be a multiple of the
+			 * appropriate block size, if set at all. It
+			 * must also be smaller than the maximum
+			 * extent size supported by the filesystem.
+			 *
+			 * Also, for non-realtime files, limit the
+			 * extent size hint to half the size of the
+			 * AGs in the filesystem so alignment doesn't
+			 * result in extents larger than an AG.
+			 */
 			xfs_extlen_t    size;
 			xfs_fsblock_t   extsize_fsb;
 
 			extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
-			if (extsize_fsb > MAXEXTLEN) {
-				code = -EINVAL;
+			if (extsize_fsb > MAXEXTLEN)
 				goto error_return;
-			}
 
 			if (XFS_IS_REALTIME_INODE(ip) ||
 			    ((mask & FSX_XFLAGS) &&
@@ -1149,16 +1164,12 @@ xfs_ioctl_setattr(
 				       mp->m_sb.sb_blocklog;
 			} else {
 				size = mp->m_sb.sb_blocksize;
-				if (extsize_fsb > mp->m_sb.sb_agblocks / 2) {
-					code = -EINVAL;
+				if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
 					goto error_return;
-				}
 			}
 
-			if (fa->fsx_extsize % size) {
-				code = -EINVAL;
+			if (fa->fsx_extsize % size)
 				goto error_return;
-			}
 		}
 	}
 
-- 
2.1.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH] xfs: add test for XFS_IOC_FSSETXATTR behaviour
  2014-08-29  2:52             ` Dave Chinner
@ 2014-12-04  4:20               ` Iustin Pop
  -1 siblings, 0 replies; 19+ messages in thread
From: Iustin Pop @ 2014-12-04  4:20 UTC (permalink / raw)
  To: david; +Cc: xfs, fstests, Iustin Pop

Adds a new test (xfs/032) that checks for correct behaviour of
XFS_IOC_FSSETXATTR for directories: extent sizes should always be
settable on a directory, even if the directory already has allocated
extents.

Signed-off-by: Iustin Pop <iustin@k1024.org>
---
 Note that I've dropped the other test, since - thanks to your explanations - I
 understood that it was meaningless (and why).

 tests/xfs/032     | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/032.out |  4 +++
 tests/xfs/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/xfs/032
 create mode 100644 tests/xfs/032.out

diff --git a/tests/xfs/032 b/tests/xfs/032
new file mode 100755
index 0000000..3afce26
--- /dev/null
+++ b/tests/xfs/032
@@ -0,0 +1,83 @@
+#! /bin/bash
+# FS QA Test No. 032
+#
+# Determine whether the extent size hint can be set on directories
+# with allocated extents correctly.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Google Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+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/filter
+. ./common/punch
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_supported_os Linux
+_require_scratch
+
+_scratch_mkfs_xfs >/dev/null 2>&1
+_scratch_mount
+
+small=$SCRATCH_MNT/small
+big=$SCRATCH_MNT/big
+
+# sanity check on a small directory
+mkdir $small
+# expect that an empty directory has no extents
+$XFS_IO_PROG -c "bmap" $small | _filter_scratch
+# and that we can set an extent size on it
+$XFS_IO_PROG -c 'extsize 8m' $small
+# and finally check that the extent size update has taken place
+$XFS_IO_PROG -c "extsize" $small | _filter_scratch
+
+# now create a 'big' (with extents) directory
+mkdir $big
+for idx in {0..1000}; do
+    touch $big/$idx
+done
+$XFS_IO_PROG -c 'bmap -vp' $big | _filter_bmap | \
+    grep -q '^0: .*data'
+[ $? -eq 0 ] || echo "Can't force allocating extents!" 1>&2
+
+# expect that we can set the extent size on $big as well
+$XFS_IO_PROG -c 'extsize 8m' $big | _filter_scratch
+# and that it took effect
+$XFS_IO_PROG -c 'extsize' $big | _filter_scratch
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/032.out b/tests/xfs/032.out
new file mode 100644
index 0000000..4b7bd92
--- /dev/null
+++ b/tests/xfs/032.out
@@ -0,0 +1,4 @@
+QA output created by 032
+SCRATCH_MNT/small: no extents
+[8388608] SCRATCH_MNT/small
+[8388608] SCRATCH_MNT/big
diff --git a/tests/xfs/group b/tests/xfs/group
index 4b8e51a..dc132b5 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -29,6 +29,7 @@
 029 mkfs logprint log auto quick
 030 repair auto quick
 031 repair mkfs auto quick
+032 ioctl quick
 033 repair auto quick
 034 other auto quick
 035 dump ioctl tape auto
-- 
2.1.3


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH] xfs: add test for XFS_IOC_FSSETXATTR behaviour
@ 2014-12-04  4:20               ` Iustin Pop
  0 siblings, 0 replies; 19+ messages in thread
From: Iustin Pop @ 2014-12-04  4:20 UTC (permalink / raw)
  To: david; +Cc: Iustin Pop, fstests, xfs

Adds a new test (xfs/032) that checks for correct behaviour of
XFS_IOC_FSSETXATTR for directories: extent sizes should always be
settable on a directory, even if the directory already has allocated
extents.

Signed-off-by: Iustin Pop <iustin@k1024.org>
---
 Note that I've dropped the other test, since - thanks to your explanations - I
 understood that it was meaningless (and why).

 tests/xfs/032     | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/032.out |  4 +++
 tests/xfs/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/xfs/032
 create mode 100644 tests/xfs/032.out

diff --git a/tests/xfs/032 b/tests/xfs/032
new file mode 100755
index 0000000..3afce26
--- /dev/null
+++ b/tests/xfs/032
@@ -0,0 +1,83 @@
+#! /bin/bash
+# FS QA Test No. 032
+#
+# Determine whether the extent size hint can be set on directories
+# with allocated extents correctly.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2014 Google Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+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/filter
+. ./common/punch
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_supported_os Linux
+_require_scratch
+
+_scratch_mkfs_xfs >/dev/null 2>&1
+_scratch_mount
+
+small=$SCRATCH_MNT/small
+big=$SCRATCH_MNT/big
+
+# sanity check on a small directory
+mkdir $small
+# expect that an empty directory has no extents
+$XFS_IO_PROG -c "bmap" $small | _filter_scratch
+# and that we can set an extent size on it
+$XFS_IO_PROG -c 'extsize 8m' $small
+# and finally check that the extent size update has taken place
+$XFS_IO_PROG -c "extsize" $small | _filter_scratch
+
+# now create a 'big' (with extents) directory
+mkdir $big
+for idx in {0..1000}; do
+    touch $big/$idx
+done
+$XFS_IO_PROG -c 'bmap -vp' $big | _filter_bmap | \
+    grep -q '^0: .*data'
+[ $? -eq 0 ] || echo "Can't force allocating extents!" 1>&2
+
+# expect that we can set the extent size on $big as well
+$XFS_IO_PROG -c 'extsize 8m' $big | _filter_scratch
+# and that it took effect
+$XFS_IO_PROG -c 'extsize' $big | _filter_scratch
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/032.out b/tests/xfs/032.out
new file mode 100644
index 0000000..4b7bd92
--- /dev/null
+++ b/tests/xfs/032.out
@@ -0,0 +1,4 @@
+QA output created by 032
+SCRATCH_MNT/small: no extents
+[8388608] SCRATCH_MNT/small
+[8388608] SCRATCH_MNT/big
diff --git a/tests/xfs/group b/tests/xfs/group
index 4b8e51a..dc132b5 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -29,6 +29,7 @@
 029 mkfs logprint log auto quick
 030 repair auto quick
 031 repair mkfs auto quick
+032 ioctl quick
 033 repair auto quick
 034 other auto quick
 035 dump ioctl tape auto
-- 
2.1.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
  2014-12-04  4:14             ` Iustin Pop
@ 2014-12-05  0:11               ` Dave Chinner
  2014-12-05  5:49                 ` Iustin Pop
  0 siblings, 1 reply; 19+ messages in thread
From: Dave Chinner @ 2014-12-05  0:11 UTC (permalink / raw)
  To: Iustin Pop; +Cc: xfs

On Thu, Dec 04, 2014 at 05:14:26AM +0100, Iustin Pop wrote:
> Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
> targets as regular files: it refuses to change the extent size if
> extents are allocated. This is wrong for directories, as there the
> extent size is only used as a default for children.
> 
> The patch fixes this issue and improves validation of flag
> combinations:
> 
> - only disallow extent size changes after extents have been allocated
>   for regular files
> - only allow XFS_XFLAG_EXTSIZE for regular files
> - only allow XFS_XFLAG_EXTSZINHERIT for directories
> - automatically clear the flags if the extent size is zero
> 
> Thanks to Dave Chinner for guidance on the proper fix for this issue.

I'll have to remind myself of the context again - I think I have
some patches in my local queue that I never finished that kill a lot
of the mess around this code. I put that at the head of next week's
queue...

> Signed-off-by: Iustin Pop <iustin@k1024.org>
> ---
>  Trying to revive this fix. Note that this patch is on top of
>  git://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git master, which
>  seems to have no commits since Oct 26; let me know if I should rebase it on
>  top of something else.

That's the right tree+branch to base dev patches on. I'll commit it
to a topic branch based on linux-xfs/master unless it has
dependencies on patches in other topic branches. Mostly you do not
need to worry about that. If you want to see the latest development
snapshot, then use the for-next branch (note: for-next gets rebased
if necessary). I normally develop against linux-xfs/master, then
merge into linux-xfs/for-next for integration testing....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories
  2014-12-05  0:11               ` Dave Chinner
@ 2014-12-05  5:49                 ` Iustin Pop
  0 siblings, 0 replies; 19+ messages in thread
From: Iustin Pop @ 2014-12-05  5:49 UTC (permalink / raw)
  To: Dave Chinner; +Cc: xfs

On Fri, Dec 05, 2014 at 11:11:48AM +1100, Dave Chinner wrote:
> On Thu, Dec 04, 2014 at 05:14:26AM +0100, Iustin Pop wrote:
> > Currently, the ioctl handling code for XFS_IOC_FSSETXATTR treats all
> > targets as regular files: it refuses to change the extent size if
> > extents are allocated. This is wrong for directories, as there the
> > extent size is only used as a default for children.
> > 
> > The patch fixes this issue and improves validation of flag
> > combinations:
> > 
> > - only disallow extent size changes after extents have been allocated
> >   for regular files
> > - only allow XFS_XFLAG_EXTSIZE for regular files
> > - only allow XFS_XFLAG_EXTSZINHERIT for directories
> > - automatically clear the flags if the extent size is zero
> > 
> > Thanks to Dave Chinner for guidance on the proper fix for this issue.
> 
> I'll have to remind myself of the context again - I think I have
> some patches in my local queue that I never finished that kill a lot
> of the mess around this code. I put that at the head of next week's
> queue...

Yep, I had forgotten myself what was the status, and had to reread
through the old email thread. That's what happens when you leave a patch
"rot" for so long :)

And yes, the code in this function is a bit hard to follow, since the
various checks are spread around. Cleaning this up would be good; I only
care that when next time I run an xfsrestore, it won't fail to set the
extent sizes correctly on directories :)

thanks!
iustin

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2014-12-05  5:49 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-14  7:09 Error setting extent size on a directory Iustin Pop
2014-07-17  9:04 ` Christoph Hellwig
2014-07-18 19:13   ` Iustin Pop
2014-08-28  4:22     ` [PATCH] xfs: fix behaviour of XFS_IOC_FSSETXATTR on directories Iustin Pop
2014-08-28  9:31       ` Dave Chinner
2014-08-28 22:34         ` Iustin Pop
2014-08-29  0:46           ` Dave Chinner
2014-12-04  4:14             ` Iustin Pop
2014-12-05  0:11               ` Dave Chinner
2014-12-05  5:49                 ` Iustin Pop
2014-08-28  4:24     ` [PATCH xfstests] xfs: add tests for XFS_IOC_FSSETXATTR behaviour Iustin Pop
2014-08-28 10:16       ` Dave Chinner
2014-08-28 10:16         ` Dave Chinner
2014-08-28 22:28         ` Iustin Pop
2014-08-28 22:28           ` Iustin Pop
2014-08-29  2:52           ` Dave Chinner
2014-08-29  2:52             ` Dave Chinner
2014-12-04  4:20             ` [PATCH] xfs: add test " Iustin Pop
2014-12-04  4:20               ` Iustin Pop

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.