Re: [PATCH] fs: fix iversion handling

Re: [PATCH] fs: fix iversion handling

[Boaz Harrosh]

On 12/02/2013 07:36 PM, Christoph Hellwig wrote:
> ping?
> 
> On Tue, Nov 19, 2013 at 07:17:07AM -0800, Christoph Hellwig wrote:
>> Currently notify_change directly updates i_version for size updates,
>> which not only is counter to how all other fields are updated through
>> struct iattr, but also breaks XFS, which need inode updates to happen
>> under its own lock, and synchronized to the structure that gets written
>> to the log.
>>
>> Remove the update in the common code, and it to btrfs and ext4,
>> XFS already does a proper updaste internally and currently gets a
>> double update with the existing code.
>>
>> IMHO this is 3.13 and -stable material and should go in through the XFS
>> tree.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> Index: xfs/fs/attr.c
>> ===================================================================
>> --- xfs.orig/fs/attr.c	2013-11-19 16:08:42.275415189 +0100
>> +++ xfs/fs/attr.c	2013-11-19 16:08:51.803414994 +0100
>> @@ -182,11 +182,6 @@ int notify_change(struct dentry * dentry
>>  			return -EPERM;
>>  	}
>>  
>> -	if ((ia_valid & ATTR_SIZE) && IS_I_VERSION(inode)) {
>> -		if (attr->ia_size != inode->i_size)
>> -			inode_inc_iversion(inode);
>> -	}
>> -

Hi Christoph

What happens with all other filesystems exported under KNFSD?

As I understand inode_inc_iversion() is used in NFSv4 and up,
what will increment the inode-version on changed attributes
for them?

(ie. do I need the same change in an FS I care about?)

Thanks
Boaz


>>  	if ((ia_valid & ATTR_MODE)) {
>>  		umode_t amode = attr->ia_mode;
>>  		/* Flag setting protected by i_mutex */
>> Index: xfs/fs/btrfs/inode.c
>> ===================================================================
>> --- xfs.orig/fs/btrfs/inode.c	2013-11-19 16:08:42.275415189 +0100
>> +++ xfs/fs/btrfs/inode.c	2013-11-19 16:08:51.803414994 +0100
>> @@ -4345,8 +4345,12 @@ static int btrfs_setsize(struct inode *i
>>  	 * these flags set.  For all other operations the VFS set these flags
>>  	 * explicitly if it wants a timestamp update.
>>  	 */
>> -	if (newsize != oldsize && (!(mask & (ATTR_CTIME | ATTR_MTIME))))
>> -		inode->i_ctime = inode->i_mtime = current_fs_time(inode->i_sb);
>> +	if (newsize != oldsize) {
>> +		inode_inc_iversion(inode);
>> +		if (!(mask & (ATTR_CTIME | ATTR_MTIME)))
>> +			inode->i_ctime = inode->i_mtime =
>> +				current_fs_time(inode->i_sb);
>> +	}
>>  
>>  	if (newsize > oldsize) {
>>  		truncate_pagecache(inode, newsize);
>> Index: xfs/fs/ext4/inode.c
>> ===================================================================
>> --- xfs.orig/fs/ext4/inode.c	2013-11-19 16:08:42.275415189 +0100
>> +++ xfs/fs/ext4/inode.c	2013-11-19 16:08:51.803414994 +0100
>> @@ -4594,6 +4594,10 @@ int ext4_setattr(struct dentry *dentry,
>>  			if (attr->ia_size > sbi->s_bitmap_maxbytes)
>>  				return -EFBIG;
>>  		}
>> +
>> +		if (IS_I_VERSION(inode) && attr->ia_size != inode->i_size)
>> +			inode_inc_iversion(inode);
>> +
>>  		if (S_ISREG(inode->i_mode) &&
>>  		    (attr->ia_size < inode->i_size)) {
>>  			if (ext4_should_order_data(inode)) {
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> ---end quoted text---
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] fs: fix iversion handling

[Christoph Hellwig]

On Tue, Dec 03, 2013 at 11:35:02AM +0200, Boaz Harrosh wrote:
> Hi Christoph
> 
> What happens with all other filesystems exported under KNFSD?
> 
> As I understand inode_inc_iversion() is used in NFSv4 and up,
> what will increment the inode-version on changed attributes
> for them?

It's used by the filesystem for the change attribute that NFSDd
can optionally use.  Most filesystsems don't support it and work
okay enough when NFS exported.  No other filesystem will need
to adopt for this patch specificly, given that no other filesystem
uses this infrastructure.

Re: [PATCH] fs: fix iversion handling

[Boaz Harrosh]

On 12/03/2013 11:45 AM, Christoph Hellwig wrote:
> On Tue, Dec 03, 2013 at 11:35:02AM +0200, Boaz Harrosh wrote:
>> Hi Christoph
>>
>> What happens with all other filesystems exported under KNFSD?
>>
>> As I understand inode_inc_iversion() is used in NFSv4 and up,
>> what will increment the inode-version on changed attributes
>> for them?
> 
> It's used by the filesystem for the change attribute that NFSDd
> can optionally use.  Most filesystsems don't support it and work
> okay enough when NFS exported.  No other filesystem will need
> to adopt for this patch specificly, given that no other filesystem
> uses this infrastructure.
> 

OK Thanks, I did not know that. I assumed change-attribute is
naturally supported.

I will look at ext4 example and wire up exofs. Because it is very
important for me to support the change-attribute properly.

Thanks you
Boaz

Re: [PATCH] fs: fix iversion handling

[J. Bruce Fields]

On Tue, Dec 03, 2013 at 12:40:54PM +0200, Boaz Harrosh wrote:
> On 12/03/2013 11:45 AM, Christoph Hellwig wrote:
> > On Tue, Dec 03, 2013 at 11:35:02AM +0200, Boaz Harrosh wrote:
> >> Hi Christoph
> >>
> >> What happens with all other filesystems exported under KNFSD?
> >>
> >> As I understand inode_inc_iversion() is used in NFSv4 and up,
> >> what will increment the inode-version on changed attributes
> >> for them?
> > 
> > It's used by the filesystem for the change attribute that NFSDd
> > can optionally use.  Most filesystsems don't support it and work
> > okay enough when NFS exported.  No other filesystem will need
> > to adopt for this patch specificly, given that no other filesystem
> > uses this infrastructure.
> > 
> 
> OK Thanks, I did not know that. I assumed change-attribute is
> naturally supported.
> 
> I will look at ext4 example and wire up exofs. Because it is very
> important for me to support the change-attribute properly.

Just please don't follow ext4's example of making support dependent on a
mount option.

--b.