[Top] [All Lists]

Re: [PATCH] xfstest: ensure small symlink is removed

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfstest: ensure small symlink is removed
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 14 Jun 2013 08:23:32 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130614021107.GQ29338@dastard>
References: <20130613151007.449190598@xxxxxxx> <20130613151037.271434256@xxxxxxx> <20130614021107.GQ29338@dastard>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 06/13/13 21:11, Dave Chinner wrote:
On Thu, Jun 13, 2013 at 10:10:08AM -0500, Mark Tinguely wrote:
Tests that XFS symlinks that are small enough to be in the
inode, but were move to a remote symlink due to an extended
attribute were correctly removed.

Signed-off-by: Mark Tinguely<tinguely@xxxxxxx>
  new/xfstests/tests/xfs/298     |   92 
  new/xfstests/tests/xfs/298.out |   33 ++++++++++++++
  new/xfstests/tests/xfs/group   |    1

I don't see anything that really needs to be XFS specific about this
test - can you make it generic?

+# test 32 to 512 byte symlink. This should make sure that 256 and
+# 512 byte inodes version 2 and 3 are covered.

Better to test all the way to MAXPATHLEN, rather than just an
arbitrary sub-range of the possible symlink ranges.

Nod. I was not covering the 1K inode.

+while [ $LOOP -gt 0 ];do

while [ $SIZE -le 512 ]; do

and eliminate one variable!

+       _scratch_mount>/dev/null 2>&1
+       cd $SCRATCH_MNT
+       echo "Testing symlink size $SIZE"
+       ln -s $SYMLINK $SYMLINK_FILE>  /dev/null 2>&1
+       inode=`ls -li $SYMLINK_FILE | awk '{print $1}'`
+# add the extended attributes
+       attr  -Rs 1234567890ab $SYMLINK_FILE<  /dev/null>  /dev/null 2>&1
+       attr  -Rs 1234567890ac $SYMLINK_FILE<  /dev/null>  /dev/null 2>&1
+       attr  -Rs 1234567890ad $SYMLINK_FILE<  /dev/null>  /dev/null 2>&1
+# remove the extended attributes
+       attr  -Rr 1234567890ab $SYMLINK_FILE>  /dev/null 2>&1
+       attr  -Rr 1234567890ac $SYMLINK_FILE>  /dev/null 2>&1
+       attr  -Rr 1234567890ad $SYMLINK_FILE>  /dev/null 2>&1
+# remove the symlink - make sure ifree removes the remote symlink.
+       rm $SYMLINK_FILE
+# umount and check the number of extents on the inode. Should be 0.
+       cd
+       _scratch_unmount>/dev/null 2>&1
+       $XFS_DB_PROG  -c "inode $inode" -c "p core.nextents" $SCRATCH_DEV

I'm not sure that's actually a valid thing to do - the inode has
been removed, so the underlying inode chunk may have been removed
and hence marked stale rather than been written back. Hence you
cannot rely on xfs_db being able to print the extent count after the
inode has been unlinked and the filesystem unmounted.

As it is, it's not really necessary for the test to detect the
problem that you found, right? i.e. that the kernel would assert
fail? If you drop the xfs_db stuff here, then the test is generic...

It will ASSERT on a debug kernel, but put this test so it would still fail in the debug case.

--- a/new/xfstests/tests/xfs/group
+++ b/new/xfstests/tests/xfs/group
@@ -177,3 +177,4 @@
  295 auto logprint quick
  296 dump auto quick
  297 auto freeze
+298 auto attr symlink

+ quick.

Okay, I had it in but removed it at the last minute.





<Prev in Thread] Current Thread [Next in Thread>