From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cuda.sgi.com (cuda1.sgi.com [192.48.157.11]) by oss.sgi.com (8.14.3/8.14.3/SuSE Linux 0.8) with ESMTP id n3O3ksqv056320 for ; Thu, 23 Apr 2009 22:46:54 -0500 Received: from mx1.redhat.com (localhost [127.0.0.1]) by cuda.sgi.com (Spam Firewall) with ESMTP id 474641440FBF for ; Thu, 23 Apr 2009 20:49:46 -0700 (PDT) Received: from mx1.redhat.com (mx1.redhat.com [66.187.233.31]) by cuda.sgi.com with ESMTP id BSDrGsSt6XJwCuba for ; Thu, 23 Apr 2009 20:49:46 -0700 (PDT) Date: Thu, 23 Apr 2009 23:46:49 -0400 (EDT) From: Lachlan McIlroy Message-ID: <773418105.4861240544809694.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> In-Reply-To: <344266684.4811240544710893.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> Subject: Re: [PATCH] xfs_file_last_byte() needs to acquire ilock MIME-Version: 1.0 Reply-To: Lachlan McIlroy List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: xfs-bounces@oss.sgi.com Errors-To: xfs-bounces@oss.sgi.com To: Eric Sandeen Cc: xfs@oss.sgi.com ----- "Eric Sandeen" wrote: > Lachlan McIlroy wrote: > > We had some systems crash with this stack: > > > > [] ia64_leave_kernel+0x0/0x280 > > [] xfs_bmbt_get_startoff+0x0/0x20 [xfs] > > [] xfs_bmap_last_offset+0x210/0x280 [xfs] > > [] xfs_file_last_byte+0x70/0x1a0 [xfs] > > [] xfs_itruncate_start+0xc0/0x1a0 [xfs] > > [] xfs_inactive_free_eofblocks+0x290/0x460 [xfs] > > [] xfs_release+0x1b0/0x240 [xfs] > > [] xfs_file_release+0x70/0xa0 [xfs] > > [] __fput+0x1a0/0x420 > > [] 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. > > -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@oss.sgi.com > > http://oss.sgi.com/mailman/listinfo/xfs > > > > _______________________________________________ > 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