From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [PATCH] always set a/c/mtime through ->setattr Date: Sat, 31 May 2008 14:20:48 +0100 Message-ID: <20080531132048.GA4201@ZenIV.linux.org.uk> References: <20080520060838.GA6436@lst.de> <20080520083351.GA14826@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: hch@lst.de, linux-fsdevel@vger.kernel.org, xfs@oss.sgi.com, Artem.Bityutskiy@nokia.com To: Miklos Szeredi Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:41402 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752172AbYEaNUv (ORCPT ); Sat, 31 May 2008 09:20:51 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, May 22, 2008 at 08:10:50PM +0200, Miklos Szeredi wrote: > But there are quite a few others which don't call inode_setattr (which > means that the unchanged time optimization is lost), or which do > something possibly slow in their ->setattr(): > > adfs, 9p, afs, coda, gfs2 ... > > just to name a few at the start of the alphabet. > > So it looks to me as this could cause some unintended performance > regressions in these filesystems. Actually, there's worse one: ext3. It *does* call inode_setattr(), all right, but then it proceeds to call ext3_orphan_del(). Which will lock_super(inode->i_sb); if (list_empty(&ei->i_orphan)) { unlock_super(inode->i_sb); return 0; } and bugger off, but... * it's going to cost us * code in _caller_ is bogus - we call that sucker regardless of whether we had ATTR_SIZE in ia_valid And there's one more problem, promising very ugly code review: locking rules for notify_change() had suddenly changed - you are calling it without i_mutex now. And ext3_setattr() is not happy - especially due to this blind call of ext3_orphan_del() in there. We can easily fix that one, but you'll need to audit the rest of instances...