xfs: xfs_inactive fails to cleanup symlinks with attributes
cover xfs: xfs_inactive fails to cleanup symlinks with attributes
Found this bug testing extended attributes.
# make a big symbolic link that is in the inode core and mostly fills it. # CRC enabled filesystem will use a 68 byte smaller link in the test.
ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a
# the extended attribute will bump the symbolic link to a remote extent # I think only one of these attribute is needed, but they are so fun... attr -Rs 1234567890ad a < /dev/null attr -Rs 1234567890ae a < /dev/null attr -Rs 1234567890af a < /dev/null
# the following remove will assert a debug kernel: # XFS: Assertion failed: ip->i_d.di_nextents == 0, file: fs/xfs/xfs_inode.c, line: 2036
rm a
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/1 xfs: fix the symbolic link assert in xfs_ifree
Adding an extended attribute to a symbolic link can force that link to an remote extent. xfs_inactive() incorrectly assumes that any symbolic link small enough to be in the inode core is incore, the remote extent is not cleaned and xfs_ifree() asserts on presence the remote extent.
--- fs/xfs/xfs_vnodeops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_vnodeops.c +1 -1
@@ -325,7 +325,7 @@ xfs_inactive(/* * Zero length symlinks _can_ exist. */ -if (ip->i_d.di_size >XFS_IFORK_DSIZE (ip)) { +if (ip->i_d.di_nextents) { error =xfs_inactive_symlink_rmt (ip, &tp);if (error) goto out_cancel;
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree
Quoted 16 lines · Mark Tinguely
Adding an extended attribute to a symbolic link can force that link to an remote extent.
xfs_inactive()incorrectly assumes that any symbolic link small enough to be in the inode core is incore, the remote extent is not cleaned andxfs_ifree()asserts on presence the remote extent.Signed-off-by Mark Tinguely <[email protected]>--- fs/xfs/xfs_vnodeops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
b/fs/xfs/xfs_vnodeops.c
===================================================================
fs/xfs/xfs_vnodeops.c xfs_inactive( · line 331
Quoted 8 lines
fs/xfs/xfs_vnodeops.c +1
-1@@ -325,7 +325,7 @@ xfs_inactive(/* * Zero length symlinks _can_ exist. */ -if (ip->i_d.di_size >XFS_IFORK_DSIZE (ip)) { +if (ip->i_d.di_nextents) { error =xfs_inactive_symlink_rmt (ip, &tp);if (error) goto out_cancel;
This should match the check done in xfs_readlink. i.e. it should check against the fork format being in local or extent form, not check against the inode size.
Also, I think that this symlink specific code should be factored out of xfs_inactive() and moved to fs/xfs/xfs_symlink.c as xfs_symlink_truncate() so that all the intricacies of the symlink truncation are in the one place. This would make xfs_symlink_truncate() look very similar to xfs_readlink()...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree
Quoted 30 lines · Dave Chinner
Quoted 16 lines · Mark Tinguely
Adding an extended attribute to a symbolic link can force that link to an remote extent.
xfs_inactive()incorrectly assumes that any symbolic link small enough to be in the inode core is incore, the remote extent is not cleaned andxfs_ifree()asserts on presence the remote extent.Signed-off-by Mark Tinguely<[email protected]>--- fs/xfs/xfs_vnodeops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
b/fs/xfs/xfs_vnodeops.c
===================================================================fs/xfs/xfs_vnodeops.c xfs_inactive( · line 331
Quoted 8 lines
fs/xfs/xfs_vnodeops.c +1
-1@@ -325,7 +325,7 @@ xfs_inactive(/* * Zero length symlinks _can_ exist. */ -if (ip->i_d.di_size>XFS_IFORK_DSIZE (ip)) { +if (ip->i_d.di_nextents) { error =xfs_inactive_symlink_rmt (ip,&tp);if (error) goto out_cancel;This should match the check done in
xfs_readlink. i.e. it should check against the fork format being in local or extent form, not check against the inode size.
Quoted 6 lines
Also, I think that this symlink specific code should be factored out of
xfs_inactive()and moved tofs/xfs/xfs_symlink.casxfs_symlink_truncate()so that all the intricacies of the symlink truncation are in the one place. This would makexfs_symlink_truncate()look very similar toxfs_readlink()...
Sounds good.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 5 lines · Mark Tinguely
Found this bug testing extended attributes.
# make a big symbolic link that is in the inode core and mostly fills it. # CRC enabled filesystem will use a 68 byte smaller link in the test.
Quoted 8 lines
ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a
# the extended attribute will bump the symbolic link to a remote extent # I think only one of these attribute is needed, but they are so fun... attr -Rs 1234567890ad a< /dev/null attr -Rs 1234567890ae a< /dev/null attr -Rs 1234567890af a< /dev/null
oops. the following steps are also needed - I took them out because I thought they were unecessary:
# remove the attributes: attr -Rr 1234567890ad a attr -Rr 1234567890ae a attr -Rr 1234567890af a
now we will assert
Quoted 10 lines
# the following remove will assert a debug kernel: # XFS: Assertion failed: ip->i_d.di_nextents == 0, file: fs/xfs/xfs_inode.c, line: 2036rm a
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 18 lines · Mark Tinguely
Quoted 5 lines · Mark Tinguely
Found this bug testing extended attributes.
# make a big symbolic link that is in the inode core and mostly fills it. # CRC enabled filesystem will use a 68 byte smaller link in the test.Quoted 8 lines
ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a
# the extended attribute will bump the symbolic link to a remote extent # I think only one of these attribute is needed, but they are so fun... attr -Rs 1234567890ad a< /dev/null attr -Rs 1234567890ae a< /dev/null attr -Rs 1234567890af a< /dev/nulloops. the following steps are also needed - I took them out because I thought they were unecessary:
Quoted 6 lines
# remove the attributes: attr -Rr 1234567890ad a attr -Rr 1234567890ae a attr -Rr 1234567890af anow we will assert
I cannot reproduce this on a current TOT kernel with or without CRCs:
# mkfs.xfs -f /dev/vdb
meta-data=/dev/vdb isize=256 agcount=4, agsize=720896 blks
= sectsz=512 attr=2, projid32bit=1
= crc=0
data = bsize=4096 blocks=2883584, imaxpct=25
= sunit=0 swidth=0 blks
naming =version 2 bsize=4096 ascii-ci=0
log =internal log bsize=4096 blocks=2560, version=2
= sectsz=512 sunit=0 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
# mount /dev/vdb /mnt/scratch/
# cd /mnt/scratch/
# ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a
# attr -Rs 1234567890ad a< /dev/null
Attribute "1234567890ad" set to a 0 byte value for a:# attr -Rs 1234567890ae a< /dev/null Attribute "1234567890ae" set to a 0 byte value for a:
# attr -Rs 1234567890af a< /dev/null Attribute "1234567890af" set to a 0 byte value for a:
# attr -Rr 1234567890ad a # attr -Rr 1234567890ae a # attr -Rr 1234567890af a # sync # cd ~ # umount /mnt/scratch #
No assert. Can you write an xfstest that reproduces the problem and post the patch?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 59 lines · Dave Chinner
Quoted 18 lines · Mark Tinguely
Quoted 5 lines · Mark Tinguely
Found this bug testing extended attributes.
# make a big symbolic link that is in the inode core and mostly fills it. # CRC enabled filesystem will use a 68 byte smaller link in the test.Quoted 8 lines
ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a
# the extended attribute will bump the symbolic link to a remote extent # I think only one of these attribute is needed, but they are so fun... attr -Rs 1234567890ad a< /dev/null attr -Rs 1234567890ae a< /dev/null attr -Rs 1234567890af a< /dev/nulloops. the following steps are also needed - I took them out because I thought they were unecessary:
Quoted 6 lines
# remove the attributes: attr -Rr 1234567890ad a attr -Rr 1234567890ae a attr -Rr 1234567890af anow we will assert
I cannot reproduce this on a current TOT kernel with or without CRCs:
# mkfs.xfs -f /dev/vdb meta-data=/dev/vdb isize=256 agcount=4, agsize=720896 blks = sectsz=512 attr=2, projid32bit=1 = crc=0 data = bsize=4096 blocks=2883584, imaxpct=25 = sunit=0 swidth=0 blks naming =version 2 bsize=4096 ascii-ci=0 log =internal log bsize=4096 blocks=2560, version=2 = sectsz=512 sunit=0 blks, lazy-count=1 realtime =none extsz=4096 blocks=0, rtextents=0 # mount /dev/vdb /mnt/scratch/ # cd /mnt/scratch/ # ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a # attr -Rs 1234567890ad a< /dev/null Attribute "1234567890ad" set to a 0 byte value for a:# attr -Rs 1234567890ae a< /dev/null Attribute "1234567890ae" set to a 0 byte value for a:# attr -Rs 1234567890af a< /dev/null Attribute "1234567890af" set to a 0 byte value for a:# attr -Rr 1234567890ad a # attr -Rr 1234567890ae a # attr -Rr 1234567890af a # sync # cd ~ # umount /mnt/scratch #
Quoted 6 lines
No assert. Can you write an xfstest that reproduces the problem and post the patch?
Cheers,
Dave.
Yes, these instructions are for a *256* byte inode with top of tree code.
A 512 byte inode will need a bigger link. The link must nearly fill the literal area. Patch has been supplied:
http://oss.sgi.com/archives/xfs/2013-06/msg00110.html
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 65 lines · Mark Tinguely
Quoted 64 lines · Dave Chinner
Quoted 18 lines · Mark Tinguely
Quoted 5 lines · Mark Tinguely
Found this bug testing extended attributes.
# make a big symbolic link that is in the inode core and mostly fills it. # CRC enabled filesystem will use a 68 byte smaller link in the test.Quoted 8 lines
ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a
# the extended attribute will bump the symbolic link to a remote extent # I think only one of these attribute is needed, but they are so fun... attr -Rs 1234567890ad a< /dev/null attr -Rs 1234567890ae a< /dev/null attr -Rs 1234567890af a< /dev/nulloops. the following steps are also needed - I took them out because I thought they were unecessary:
Quoted 6 lines
# remove the attributes: attr -Rr 1234567890ad a attr -Rr 1234567890ae a attr -Rr 1234567890af anow we will assert
I cannot reproduce this on a current TOT kernel with or without CRCs:
# mkfs.xfs -f /dev/vdb meta-data=/dev/vdb isize=256 agcount=4, agsize=720896 blks = sectsz=512 attr=2, projid32bit=1 = crc=0 data = bsize=4096 blocks=2883584, imaxpct=25 = sunit=0 swidth=0 blks naming =version 2 bsize=4096 ascii-ci=0 log =internal log bsize=4096 blocks=2560, version=2 = sectsz=512 sunit=0 blks, lazy-count=1 realtime =none extsz=4096 blocks=0, rtextents=0 # mount /dev/vdb /mnt/scratch/ # cd /mnt/scratch/ # ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a # attr -Rs 1234567890ad a< /dev/null Attribute "1234567890ad" set to a 0 byte value for a:# attr -Rs 1234567890ae a< /dev/null Attribute "1234567890ae" set to a 0 byte value for a:# attr -Rs 1234567890af a< /dev/null Attribute "1234567890af" set to a 0 byte value for a:# attr -Rr 1234567890ad a # attr -Rr 1234567890ae a # attr -Rr 1234567890af a # sync # cd ~ # umount /mnt/scratch #No assert. Can you write an xfstest that reproduces the problem and post the patch?
Cheers,
Quoted 7 lines
Quoted 1 line
Dave.
Yes, these instructions are for a *256* byte inode with top of tree code.
A 512 byte inode will need a bigger link. The link must nearly fill the literal area. Patch has been supplied:
I think he meant a patch for xfstests ;)
-Eric
Quoted 7 lines
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 32 lines · Mark Tinguely
Quoted 31 lines · Dave Chinner
Quoted 18 lines · Mark Tinguely
Quoted 5 lines · Mark Tinguely
Found this bug testing extended attributes.
# make a big symbolic link that is in the inode core and mostly fills it. # CRC enabled filesystem will use a 68 byte smaller link in the test.Quoted 8 lines
ln -s 1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/1234567890/0123456/a a
# the extended attribute will bump the symbolic link to a remote extent # I think only one of these attribute is needed, but they are so fun... attr -Rs 1234567890ad a< /dev/null attr -Rs 1234567890ae a< /dev/null attr -Rs 1234567890af a< /dev/nulloops. the following steps are also needed - I took them out because I thought they were unecessary:
Quoted 6 lines
# remove the attributes: attr -Rr 1234567890ad a attr -Rr 1234567890ae a attr -Rr 1234567890af anow we will assert
I cannot reproduce this on a current TOT kernel with or without CRCs:
# mkfs.xfs -f /dev/vdb meta-data=/dev/vdb isize=256 agcount=4, agsize=720896 blks
.....
Quoted 2 lines
Yes, these instructions are for a *256* byte inode with top of tree code.
Yup, that's what I was testing with. It's right there in the mkfs output I posted. Hence I want your exact test script that reproduces it, in case there's still something missing from what you've sent as the test case....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs