xfs
[Top] [All Lists]

Re: [PATCH] xfs_file_last_byte() needs to acquire ilock

To: Lachlan McIlroy <lmcilroy@xxxxxxxxxx>
Subject: Re: [PATCH] xfs_file_last_byte() needs to acquire ilock
From: Felix Blyakher <felixb@xxxxxxx>
Date: Fri, 24 Apr 2009 16:42:41 -0500
Cc: Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <773418105.4861240544809694.JavaMail.root@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <773418105.4861240544809694.JavaMail.root@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>

On Apr 23, 2009, at 10:46 PM, Lachlan McIlroy wrote:


----- "Eric Sandeen" <sandeen@xxxxxxxxxxx> wrote:

Lachlan McIlroy wrote:
We had some systems crash with this stack:

[<a00000010000cb20>] ia64_leave_kernel+0x0/0x280
[<a00000021291ca00>] xfs_bmbt_get_startoff+0x0/0x20 [xfs]
[<a0000002129080b0>] xfs_bmap_last_offset+0x210/0x280 [xfs]
[<a00000021295b010>] xfs_file_last_byte+0x70/0x1a0 [xfs]
[<a00000021295b200>] xfs_itruncate_start+0xc0/0x1a0 [xfs]
[<a0000002129935f0>] xfs_inactive_free_eofblocks+0x290/0x460 [xfs]
[<a000000212998fb0>] xfs_release+0x1b0/0x240 [xfs]
[<a0000002129ad930>] xfs_file_release+0x70/0xa0 [xfs]
[<a000000100162ea0>] __fput+0x1a0/0x420
[<a000000100163160>] fput+0x40/0x60

The problem here is that xfs_file_last_byte() does not acquire the
inode lock and can therefore race with another thread that is
modifying
the extext list.  While xfs_bmap_last_offset() is trying to lookup
what was the last extent some extents were merged and the extent
list
shrunk so the index we lookup is now beyond the end of the extent
list
and potentially in a freed buffer.

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e7ae08d..cf62d9d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1258,8 +1258,10 @@ xfs_file_last_byte(

       /*
        * Only check for blocks beyond the EOF if the extents have
        * been read in.  This eliminates the need for the inode
lock,
        * and it also saves us from looking when it really isn't
         * necessary.
         */

I suppose that comment should be modified too, and maybe the commit
log
should say why, exactly, it was wrong? :)
Ha, I didn't even read the comment!  It's still kind of correct in
that we wont have to get the inode lock if the extents have not been

read in.

I'd still think the comments could be made less confusing
if we're adding the inode lock here.

Felix




-Eric

        if (ip->i_df.if_flags & XFS_IFEXTENTS) {
+               xfs_ilock(ip, XFS_ILOCK_SHARED);
                error = xfs_bmap_last_offset(NULL, ip, &last_block,
                        XFS_DATA_FORK);
+               xfs_iunlock(ip, XFS_ILOCK_SHARED);
                if (error) {
                        last_block = 0;
                }

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


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

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

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