From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [PATCH 2/2] check ATTR_SIZE contraints in inode_change_ok Date: Wed, 9 Jun 2010 20:26:23 +1000 Message-ID: <20100609102623.GB26335@laptop> References: <20100601113915.GA4861@lst.de> <20100601113937.GB4929@lst.de> <20100609073336.GV26335@laptop> <20100609094121.GC3393@quack.suse.cz> <20100609100715.GA15837@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org, mfasheh@suse.de To: Christoph Hellwig Return-path: Received: from cantor.suse.de ([195.135.220.2]:33261 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756263Ab0FIK01 (ORCPT ); Wed, 9 Jun 2010 06:26:27 -0400 Content-Disposition: inline In-Reply-To: <20100609100715.GA15837@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Jun 09, 2010 at 12:07:15PM +0200, Christoph Hellwig wrote: > On Wed, Jun 09, 2010 at 11:41:21AM +0200, Jan Kara wrote: > > That's a good point. For all local filesystems I know, holding i_mutex is > > enough for having stable i_size. But for clustered filesystems it > > definitely isn't. They have to hold cluster locks to be able to reliably > > check current i_size (at least OCFS2 does). Looking at what > > inode_newsize_ok currently does, i_size is only used to decide whether > > we need to check for rlimit or not. So we could falsely miss this > > check (other node is truncating the file below new offset)... Hmm, OK, so > > we really need the cluster lock... > > BTW: Mark, don't we need the cluster lock also for the permission > > checks in inode_change_ok? Otherwise we could see: > > Yes, we should have it for all of the checks. It would be good if > the cluster folks came up with proper patches for vfs.git #for-next > to fix up the cluster locking for all of ->setattr. What about network filesystems? Some (ceph, cifs) seem to do i_size checks under i_lock. Pseudo filesystems -- most seem OK. Miklos why do you time out the attributes on inodes? Is this accounted for when doing setattr checks (ie. might any attributes be stale here?) And what to do about intermediate breakage in vfs tree? Should we instead add a new helper which does all the required checks and then push non obvious changes through filesystem trees?