[PATCH] generic/449: make the test effective against xfs

[PATCH] generic/449: make the test effective against xfs

[Ernesto A. Fernández]

Setting acls on an xfs filesystem will succeed even after running out
of space for user attributes. Use trusted attributes instead.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 tests/generic/449 | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/tests/generic/449 b/tests/generic/449
index fb776b3..6fbc634 100755
--- a/tests/generic/449
+++ b/tests/generic/449
@@ -59,6 +59,15 @@ _require_attrs
 _scratch_mkfs_sized $((50 * 1024 * 1024)) >> $seqres.full 2>&1
 _scratch_mount || _fail "mount failed"
 
+if [ "$FSTYP" == "xfs" ]; then
+	# xfs takes too long to run out of space if setting only the names
+	# of the attributes. Create a 5k file filled with dots to be used
+	# as their value.
+	VFILE=$SCRATCH_MNT/valuefile
+	touch $VFILE
+	$XFS_IO_PROG -c "pwrite -S 0x2E 0 5k" $VFILE >>$seqres.full 2>&1
+fi
+
 TFILE=$SCRATCH_MNT/testfile.$seq
 
 # Create the test file and choose its permissions
@@ -70,6 +79,13 @@ chmod go-rwx $TFILE
 $XFS_IO_PROG -c "pwrite 0 50m" $TFILE >>$seqres.full 2>&1
 i=1
 j=1
+ATTR_TYPE=user
+if [ "$FSTYP" == "xfs" ]; then
+	ATTR_TYPE=trusted
+	while $SETFATTR_PROG -n $ATTR_TYPE.$i -v $(cat $VFILE) $TFILE &>/dev/null; do
+		((++i))
+	done
+fi
 ret=0
 while [ $ret -eq 0 ]; do
 	ret=1
@@ -77,7 +93,7 @@ while [ $ret -eq 0 ]; do
 		# On btrfs, setfattr will sometimes fail when free space is
 		# low, long before it's actually exhausted. Insist until it
 		# fails consistently.
-		$SETFATTR_PROG -n user.$i"x"$j $TFILE &>/dev/null
+		$SETFATTR_PROG -n $ATTR_TYPE.$i"x"$j $TFILE &>/dev/null
 		ret=$(( $ret && $? ))
 		((++j))
 	done
-- 
2.1.4

Re: [PATCH] generic/449: make the test effective against xfs

[Eryu Guan]

On Tue, Aug 01, 2017 at 01:55:31AM -0300, Ernesto A. Fernández wrote:
> Setting acls on an xfs filesystem will succeed even after running out
> of space for user attributes. Use trusted attributes instead.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

How about using the "XFS way" unconditionally in the test? I found that
test still failed for ext4 and btrfs when using the "XFS way", so I
wondered if we don't need to special-case XFS at all.

Thanks,
Eryu

Re: [PATCH] generic/449: make the test effective against xfs

[Ernesto A. Fernández]

On Wed, Aug 02, 2017 at 09:33:14AM +0800, Eryu Guan wrote:
> On Tue, Aug 01, 2017 at 01:55:31AM -0300, Ernesto A. Fernández wrote:
> > Setting acls on an xfs filesystem will succeed even after running out
> > of space for user attributes. Use trusted attributes instead.
> > 
> > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> 
> How about using the "XFS way" unconditionally in the test? I found that
> test still failed for ext4 and btrfs when using the "XFS way", so I
> wondered if we don't need to special-case XFS at all.

I tried some other filesystems and it is as you say. I don't know why I
expected otherwise. I'll send the new patch right away.

[PATCH v2] generic/449: make the test effective against xfs

[Ernesto A. Fernández]

Setting acls on an xfs filesystem will succeed even after running out
of space for user attributes. Use trusted attributes instead. Also speed
up the test by setting large values for the attributes.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
Changes in v2:
  - The check for xfs actually worked for all filesystems, as Eryu Guan
    noticed. So the test is now much more straightforward.

 tests/generic/449 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/generic/449 b/tests/generic/449
index fb776b3..f5aad22 100755
--- a/tests/generic/449
+++ b/tests/generic/449
@@ -66,9 +66,17 @@ touch $TFILE
 chmod u+rwx $TFILE
 chmod go-rwx $TFILE
 
+# The content of this file will be used as the value of the attributes
+VFILE=$SCRATCH_MNT/valuefile
+touch $VFILE
+$XFS_IO_PROG -c "pwrite -S 0x2E 0 1k" $VFILE >>$seqres.full 2>&1
+
 # Try to run out of space so setfacl will fail
 $XFS_IO_PROG -c "pwrite 0 50m" $TFILE >>$seqres.full 2>&1
 i=1
+while $SETFATTR_PROG -n trusted.$i -v $(cat $VFILE) $TFILE &>/dev/null; do
+	((++i))
+done
 j=1
 ret=0
 while [ $ret -eq 0 ]; do
@@ -77,7 +85,7 @@ while [ $ret -eq 0 ]; do
 		# On btrfs, setfattr will sometimes fail when free space is
 		# low, long before it's actually exhausted. Insist until it
 		# fails consistently.
-		$SETFATTR_PROG -n user.$i"x"$j $TFILE &>/dev/null
+		$SETFATTR_PROG -n trusted.$i"x"$j $TFILE &>/dev/null
 		ret=$(( $ret && $? ))
 		((++j))
 	done
-- 
2.1.4

Re: [PATCH v2] generic/449: make the test effective against xfs

[Eryu Guan]

On Wed, Aug 02, 2017 at 01:19:34AM -0300, Ernesto A. Fernández wrote:
> Setting acls on an xfs filesystem will succeed even after running out
> of space for user attributes. Use trusted attributes instead. Also speed
> up the test by setting large values for the attributes.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> ---
> Changes in v2:
>   - The check for xfs actually worked for all filesystems, as Eryu Guan
>     noticed. So the test is now much more straightforward.

Thanks for the update!

> 
>  tests/generic/449 | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/generic/449 b/tests/generic/449
> index fb776b3..f5aad22 100755
> --- a/tests/generic/449
> +++ b/tests/generic/449
> @@ -66,9 +66,17 @@ touch $TFILE
>  chmod u+rwx $TFILE
>  chmod go-rwx $TFILE
>  
> +# The content of this file will be used as the value of the attributes
> +VFILE=$SCRATCH_MNT/valuefile
> +touch $VFILE
> +$XFS_IO_PROG -c "pwrite -S 0x2E 0 1k" $VFILE >>$seqres.full 2>&1
> +

I think this VFILE is not needed, because ..

>  # Try to run out of space so setfacl will fail
>  $XFS_IO_PROG -c "pwrite 0 50m" $TFILE >>$seqres.full 2>&1
>  i=1
> +while $SETFATTR_PROG -n trusted.$i -v $(cat $VFILE) $TFILE &>/dev/null; do

we can use something like $(perl -e 'print "a"x1024') to generate 1k
attr value.

I can fix it at commit time if this looks OK to you.

Thanks,
Eryu

> +	((++i))
> +done
>  j=1
>  ret=0
>  while [ $ret -eq 0 ]; do
> @@ -77,7 +85,7 @@ while [ $ret -eq 0 ]; do
>  		# On btrfs, setfattr will sometimes fail when free space is
>  		# low, long before it's actually exhausted. Insist until it
>  		# fails consistently.
> -		$SETFATTR_PROG -n user.$i"x"$j $TFILE &>/dev/null
> +		$SETFATTR_PROG -n trusted.$i"x"$j $TFILE &>/dev/null
>  		ret=$(( $ret && $? ))
>  		((++j))
>  	done
> -- 
> 2.1.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] generic/449: make the test effective against xfs

[Ernesto A. Fernández]

On Wed, Aug 02, 2017 at 03:00:30PM +0800, Eryu Guan wrote:
> On Wed, Aug 02, 2017 at 01:19:34AM -0300, Ernesto A. Fernández wrote:
> >  
> > +# The content of this file will be used as the value of the attributes
> > +VFILE=$SCRATCH_MNT/valuefile
> > +touch $VFILE
> > +$XFS_IO_PROG -c "pwrite -S 0x2E 0 1k" $VFILE >>$seqres.full 2>&1
> > +
> 
> I think this VFILE is not needed, because ..
> 
> >  # Try to run out of space so setfacl will fail
> >  $XFS_IO_PROG -c "pwrite 0 50m" $TFILE >>$seqres.full 2>&1
> >  i=1
> > +while $SETFATTR_PROG -n trusted.$i -v $(cat $VFILE) $TFILE &>/dev/null; do
> 
> we can use something like $(perl -e 'print "a"x1024') to generate 1k
> attr value.

All right.

> I can fix it at commit time if this looks OK to you.

That's great, thanks.

Re: [PATCH v2] generic/449: make the test effective against xfs

[Darrick J. Wong]

On Wed, Aug 02, 2017 at 12:48:09PM -0300, Ernesto A. Fernández wrote:
> On Wed, Aug 02, 2017 at 03:00:30PM +0800, Eryu Guan wrote:
> > On Wed, Aug 02, 2017 at 01:19:34AM -0300, Ernesto A. Fernández wrote:
> > >  
> > > +# The content of this file will be used as the value of the attributes
> > > +VFILE=$SCRATCH_MNT/valuefile
> > > +touch $VFILE
> > > +$XFS_IO_PROG -c "pwrite -S 0x2E 0 1k" $VFILE >>$seqres.full 2>&1
> > > +
> > 
> > I think this VFILE is not needed, because ..
> > 
> > >  # Try to run out of space so setfacl will fail
> > >  $XFS_IO_PROG -c "pwrite 0 50m" $TFILE >>$seqres.full 2>&1
> > >  i=1
> > > +while $SETFATTR_PROG -n trusted.$i -v $(cat $VFILE) $TFILE &>/dev/null; do
> > 
> > we can use something like $(perl -e 'print "a"x1024') to generate 1k
> > attr value.

But we can do 60x better than that on xfs, which supports 64k attr values.
ext4 is adding support for large values too.

I guess the complicated part is having to experimentally determine the
maximum attr value size for a given fs and then plugging that in... the
gains for which aren't necessarily obvious aside from reducing test runtime.

--D

> 
> All right.
> 
> > I can fix it at commit time if this looks OK to you.
> 
> That's great, thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] generic/449: make the test effective against xfs

[Ernesto A. Fernández]

On Wed, Aug 02, 2017 at 10:33:34AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 02, 2017 at 12:48:09PM -0300, Ernesto A. Fernández wrote:
> > On Wed, Aug 02, 2017 at 03:00:30PM +0800, Eryu Guan wrote:
> > > On Wed, Aug 02, 2017 at 01:19:34AM -0300, Ernesto A. Fernández wrote:
> > > >  
> > > > +# The content of this file will be used as the value of the attributes
> > > > +VFILE=$SCRATCH_MNT/valuefile
> > > > +touch $VFILE
> > > > +$XFS_IO_PROG -c "pwrite -S 0x2E 0 1k" $VFILE >>$seqres.full 2>&1
> > > > +
> > > 
> > > I think this VFILE is not needed, because ..
> > > 
> > > >  # Try to run out of space so setfacl will fail
> > > >  $XFS_IO_PROG -c "pwrite 0 50m" $TFILE >>$seqres.full 2>&1
> > > >  i=1
> > > > +while $SETFATTR_PROG -n trusted.$i -v $(cat $VFILE) $TFILE &>/dev/null; do
> > > 
> > > we can use something like $(perl -e 'print "a"x1024') to generate 1k
> > > attr value.
> 
> But we can do 60x better than that on xfs, which supports 64k attr values.
> ext4 is adding support for large values too.

Thanks for bringing this up. I did know that larger attr values were
supported; I set it to 1k on purpose, as I thought that made it more
likely to work for all. There are several filesystems I have not tried
yet that will probably fail this test.

> I guess the complicated part is having to experimentally determine the
> maximum attr value size for a given fs and then plugging that in... the
> gains for which aren't necessarily obvious aside from reducing test runtime.

In this particular test the filesystem has already been mostly filled by
xfs_io, so I don't imagine we would see a big difference in runtime. I
wouldn't go through all that trouble unless you think other tests will
benefit from this.

Another thing: right after the filesystem runs out of space for large
attribute values, the test will start to set attrs without a value until
that fails as well. Otherwise there could still be some room left for the
acls. I'm guessing that, if we used 64k values, that could leave up to 64k
to be filled by those attributes with no value, which could actually worsen
the runtime, if only a bit.

Thanks,
Ernest