From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cuda.sgi.com (cuda3.sgi.com [192.48.176.15]) by oss.sgi.com (8.14.3/8.14.3/SuSE Linux 0.8) with ESMTP id n3O2o1Tl053795 for ; Thu, 23 Apr 2009 21:50:02 -0500 Received: from mail.sandeen.net (localhost [127.0.0.1]) by cuda.sgi.com (Spam Firewall) with ESMTP id 5E2F11CE3944 for ; Thu, 23 Apr 2009 19:49:59 -0700 (PDT) Received: from mail.sandeen.net (sandeen.net [209.173.210.139]) by cuda.sgi.com with ESMTP id 6fivHabdV0Fyklz0 for ; Thu, 23 Apr 2009 19:49:59 -0700 (PDT) Message-ID: <49F128D5.2090200@sandeen.net> Date: Thu, 23 Apr 2009 21:49:57 -0500 From: Eric Sandeen MIME-Version: 1.0 Subject: Re: [PATCH] xfs_file_last_byte() needs to acquire ilock References: <1624785772.3251240539480564.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> In-Reply-To: <1624785772.3251240539480564.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com> 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: Lachlan McIlroy Cc: xfs@oss.sgi.com 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? :) -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