[PATCH] fs: fix iversion handling

[PATCH] fs: fix iversion handling

[Christoph Hellwig]

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);
-	}
-
 	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)) {

Re: [PATCH] fs: fix iversion handling

[Christoph Hellwig]

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);
> -	}
> -
>  	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---

Re: [PATCH] fs: fix iversion handling

[Andreas Dilger]

[-- Attachment #1.1: Type: text/plain, Size: 3645 bytes --]

On Dec 2, 2013, at 10:36 AM, Christoph Hellwig <hch@infradead.org> wrote:
> ping?

Added linux-ext4 to the CC list.

I'm happy to see this cleanup.  You can add my

  Reviewed-by: Andreas Dilger <adilger@dilger.ca>

for the ext4 part, one question inline for the btrfs change (someone with more
btrfs knowledge needs to comment if that is important or not.

Cheers, Andreas

> 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);
>> -	}
>> -
>> 	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);

Should this be conditional on IS_I_VERSION(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


Cheers, Andreas






[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] fs: fix iversion handling

[Jan Kara]

On Mon 02-12-13 09:36:36, 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>
  Feel free to add my:
Acked-by: Jan Kara <jack@suse.cz>
  for the ext4 part.

								Honza

> > 
> > 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);
> > -	}
> > -
> >  	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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] fs: fix iversion handling

[Dave Chinner]

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>

Looks good to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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-jcswGhMUV9g@public.gmane.org>
>>
>> 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-VZNHf3L845pBDgjK7y7TUQ@public.gmane.org
>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: fix iversion handling

[Christoph Hellwig]

On Mon, Dec 02, 2013 at 03:05:07PM -0700, Andreas Dilger wrote:
> >> 	 */
> >> -	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);
> 
> Should this be conditional on IS_I_VERSION(inode)?

btrfs sets MS_I_VERSION unconditionally.

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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: fix iversion handling

[Ben Myers]

Hey Christoph,

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>

Does this still need a btrfs ack, or is it ready to go?

-Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] fs: fix iversion handling

[Chris Mason]

Quoting Ben Myers (2013-12-04 16:25:04)
> Hey Christoph,
> 
> 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>
> 
> Does this still need a btrfs ack, or is it ready to go?

The btrfs parts look good to me.

Signed-off-by: Chris Mason <clm@fb.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] fs: fix iversion handling

[Ben Myers]

On Wed, Dec 04, 2013 at 05:01:44PM -0500, Chris Mason wrote:
> Quoting Ben Myers (2013-12-04 16:25:04)
> > Hey Christoph,
> > 
> > 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>
> > 
> > Does this still need a btrfs ack, or is it ready to go?
> 
> The btrfs parts look good to me.
> 
> Signed-off-by: Chris Mason <clm@fb.com>

Great.  Applied to the xfs tree as per Christoph's suggestion.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs