* [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes
@ 2013-06-06 16:10 Mark Tinguely
2013-06-06 16:10 ` [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree Mark Tinguely
2013-06-06 19:17 ` [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes Mark Tinguely
0 siblings, 2 replies; 9+ messages in thread
From: Mark Tinguely @ 2013-06-06 16:10 UTC (permalink / raw)
To: xfs
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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree
2013-06-06 16:10 [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes Mark Tinguely
@ 2013-06-06 16:10 ` Mark Tinguely
2013-06-07 1:06 ` Dave Chinner
2013-06-06 19:17 ` [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes Mark Tinguely
1 sibling, 1 reply; 9+ messages in thread
From: Mark Tinguely @ 2013-06-06 16:10 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-fix-assert-in-xfs_ifree.patch --]
[-- Type: text/plain, Size: 944 bytes --]
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.
Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
fs/xfs/xfs_vnodeops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: b/fs/xfs/xfs_vnodeops.c
===================================================================
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes
2013-06-06 16:10 [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes Mark Tinguely
2013-06-06 16:10 ` [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree Mark Tinguely
@ 2013-06-06 19:17 ` Mark Tinguely
2013-06-06 21:13 ` Dave Chinner
1 sibling, 1 reply; 9+ messages in thread
From: Mark Tinguely @ 2013-06-06 19:17 UTC (permalink / raw)
To: xfs
On 06/06/13 11:10, Mark Tinguely wrote:
> 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
>
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
> # 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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes
2013-06-06 19:17 ` [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes Mark Tinguely
@ 2013-06-06 21:13 ` Dave Chinner
2013-06-06 21:29 ` Mark Tinguely
0 siblings, 1 reply; 9+ messages in thread
From: Dave Chinner @ 2013-06-06 21:13 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On Thu, Jun 06, 2013 at 02:17:15PM -0500, Mark Tinguely wrote:
> On 06/06/13 11:10, Mark Tinguely wrote:
> >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
> >
>
> 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
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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes
2013-06-06 21:13 ` Dave Chinner
@ 2013-06-06 21:29 ` Mark Tinguely
2013-06-06 21:44 ` Eric Sandeen
2013-06-06 21:51 ` Dave Chinner
0 siblings, 2 replies; 9+ messages in thread
From: Mark Tinguely @ 2013-06-06 21:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 06/06/13 16:13, Dave Chinner wrote:
> On Thu, Jun 06, 2013 at 02:17:15PM -0500, Mark Tinguely wrote:
>> On 06/06/13 11:10, Mark Tinguely wrote:
>>> 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
>>>
>>
>> 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
>
> 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.
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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes
2013-06-06 21:29 ` Mark Tinguely
@ 2013-06-06 21:44 ` Eric Sandeen
2013-06-06 21:51 ` Dave Chinner
1 sibling, 0 replies; 9+ messages in thread
From: Eric Sandeen @ 2013-06-06 21:44 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On 6/6/13 4:29 PM, Mark Tinguely wrote:
> On 06/06/13 16:13, Dave Chinner wrote:
>> On Thu, Jun 06, 2013 at 02:17:15PM -0500, Mark Tinguely wrote:
>>> On 06/06/13 11:10, Mark Tinguely wrote:
>>>> 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
>>>>
>>>
>>> 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
>>
>> 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.
>
> 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
I think he meant a patch for xfstests ;)
-Eric
> --Mark.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes
2013-06-06 21:29 ` Mark Tinguely
2013-06-06 21:44 ` Eric Sandeen
@ 2013-06-06 21:51 ` Dave Chinner
1 sibling, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2013-06-06 21:51 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On Thu, Jun 06, 2013 at 04:29:01PM -0500, Mark Tinguely wrote:
> On 06/06/13 16:13, Dave Chinner wrote:
> >On Thu, Jun 06, 2013 at 02:17:15PM -0500, Mark Tinguely wrote:
> >>On 06/06/13 11:10, Mark Tinguely wrote:
> >>>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
> >>>
> >>
> >>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
> >
> >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
.....
>
> 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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree
2013-06-06 16:10 ` [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree Mark Tinguely
@ 2013-06-07 1:06 ` Dave Chinner
2013-06-07 14:05 ` Mark Tinguely
0 siblings, 1 reply; 9+ messages in thread
From: Dave Chinner @ 2013-06-07 1:06 UTC (permalink / raw)
To: Mark Tinguely; +Cc: xfs
On Thu, Jun 06, 2013 at 11:10:33AM -0500, Mark Tinguely wrote:
> 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.
>
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> fs/xfs/xfs_vnodeops.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: b/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree
2013-06-07 1:06 ` Dave Chinner
@ 2013-06-07 14:05 ` Mark Tinguely
0 siblings, 0 replies; 9+ messages in thread
From: Mark Tinguely @ 2013-06-07 14:05 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 06/06/13 20:06, Dave Chinner wrote:
> On Thu, Jun 06, 2013 at 11:10:33AM -0500, Mark Tinguely wrote:
>> 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.
>>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---
>> fs/xfs/xfs_vnodeops.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: b/fs/xfs/xfs_vnodeops.c
>> ===================================================================
>> --- a/fs/xfs/xfs_vnodeops.c
>> +++ b/fs/xfs/xfs_vnodeops.c
>> @@ -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()...
>
Sounds good.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-06-07 14:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-06 16:10 [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes Mark Tinguely
2013-06-06 16:10 ` [PATCH 1/1] xfs: fix the symbolic link assert in xfs_ifree Mark Tinguely
2013-06-07 1:06 ` Dave Chinner
2013-06-07 14:05 ` Mark Tinguely
2013-06-06 19:17 ` [PATCH 0/1] xfs: xfs_inactive fails to cleanup symlinks with attributes Mark Tinguely
2013-06-06 21:13 ` Dave Chinner
2013-06-06 21:29 ` Mark Tinguely
2013-06-06 21:44 ` Eric Sandeen
2013-06-06 21:51 ` Dave Chinner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox