From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [PATCH] always set a/c/mtime through ->setattr Date: Tue, 20 May 2008 10:33:51 +0200 Message-ID: <20080520083351.GA14826@lst.de> References: <20080520060838.GA6436@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: hch@lst.de, viro@ZenIV.linux.org.uk, linux-fsdevel@vger.kernel.org, xfs@oss.sgi.com, Artem.Bityutskiy@nokia.com To: Miklos Szeredi Return-path: Received: from verein.lst.de ([213.95.11.210]:45133 "EHLO verein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755364AbYETIeI (ORCPT ); Tue, 20 May 2008 04:34:08 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, May 20, 2008 at 09:40:56AM +0200, Miklos Szeredi wrote: > > Currently touch_atime and file_update_time directly update a/c/mtime > > in the inode and just mark the inode dirty afterwards. This is pretty > > bad for some more complex filesystems that have various different types > > of dirtying an inode and/or need to store the data in another place > > for example for a buffer to be logged. > > > > This patch changes touch_atime and file_update_time to not update the > > inode directly but rather call through ->setattr into the filessystem. > > Do we know what effect this will have on read/write performance? I > can imagine that some ->setattr() implementations are orders of > magnitude slower than just dirtying the inode. All major disk or in-memory filesystems except for XFS just pass down ATTR_*TIME requests to inode_setattr which is not more than just dirtying the inode. NFS and CIFS set S_NOCMTIME so they're not affected by this at all. > This optimization is fishy. Remember, inode->i_*time are just cached > values, and the actual times on the (remote) filesystem itself can > differ. Which means that we will now optimize out a "touch" because > we happened to have the current time cached in the inode. Not that > this would be a likely event, but still... > > So at least this check should be made dependent on ATTR_UPDTIMES, and > explicit time updates left alone. Good catch. I'll fix by either/or moving the check into ->setattr and making it conditional on ATTR_UPDTIMES.