From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [patch] fix truncate inode time modification breakage Date: Tue, 1 Jun 2010 23:56:55 +1000 Message-ID: <20100601135655.GU9453@laptop> References: <20100601133923.GT9453@laptop> <20100601134801.GA11061@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Al Viro , linux-fsdevel@vger.kernel.org To: Christoph Hellwig Return-path: Received: from cantor2.suse.de ([195.135.220.15]:51156 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755020Ab0FAN47 (ORCPT ); Tue, 1 Jun 2010 09:56:59 -0400 Content-Disposition: inline In-Reply-To: <20100601134801.GA11061@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Jun 01, 2010 at 03:48:01PM +0200, Christoph Hellwig wrote: > On Tue, Jun 01, 2010 at 11:39:23PM +1000, Nick Piggin wrote: > > It appears that I've broken inode time modifications on tmpfs/ext2. > > While ftruncate always updates these attributes, truncate must not > > unless size is changed. I hadn't actually understood that until > > Christoph told me. > > > > Confusion is increased because other filesystems get this wrong. > > Those without ->setattr or ->truncate get it wrong by default. > > Others appear to have problems too. > > > > I haven't gone through many yet, but is there any reason not to > > just do it in the vfs? > > Doing it in the VFS is fine with me, we still have the the file > pointer in struct iatta to indicate a ftruncate / open O_TRUNC > if any filesystem really cares. But I think you need to audit > all instances if they care about this. And while you're at it > also remove the code handling this and the comments about it in > XFS. Yes I was starting to look at that. Just want to see if I'm on the right track. > > - if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) { > > - loff_t newsize = attr->ia_size; > > + if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE) && > > + newsize != inode->i_size) { > > Btw, the S_ISREG is superflous - we only ever set ATTR_SIZE for > regular files from the upper layer code. OK I'll get rid of it in the same patch.