Re: PING [PATCH] vfs: reorganize inode chattr checks

[Valerie Aurora <vaurora@redhat.com>]

On Tue, Apr 06, 2010 at 02:15:10PM +0400, Dmitry Monakhov wrote:
> Al, please take a look at the patch below.
> Currently it's depends on Nick's 'new truncate sequence' patch-set
> Do you plan to accept Nick's patch, or i should resubmit
> my patch against vfs-2.6.git/untested?

I'm interested, too.

-VAL

> Dmitry Monakhov <dmonakhov@openvz.org> writes:
> 
> > This patch fix nasty bug with truncate on swapfile.
> > It contains only vfs core helpers and fix isize check path for
> > fs w/o i_op->setattr method. Later i'll send corresponding changes
> > to other fs which has ->setattr method.
> >
> > This patch is depends on Nick's patch
> > http://marc.info/?l=linux-fsdevel&m=126752788514574&w=2
> >
> > From e8587585b2e2fefb75d47f8d50ddc7099a50873c Mon Sep 17 00:00:00 2001
> > From: Dmitry Monakhov <dmonakhov@openvz.org>
> > Date: Wed, 17 Mar 2010 19:49:22 +0300
> > Subject: [PATCH] vfs: reorganize inode chattr checks
> >
> > vmtrucate may fail due to IS_SWAPFILE flag or due to RLIMIT_FSIZE.
> > In some situations it is not acceptable behaviour. Off course
> > we can check IS_SWAPFILE before but what about RLIMIT_FSIZE?
> >
> > Let's divide newsize seting logic in two parts
> >  First which perform necessery size checks
> >  Second which does the work and can not fail.
> > And perform size check before any inode modifications.
> >
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > ---
> >  fs/attr.c          |   45 ++++++++++++++++++++++++++++++++++-----------
> >  fs/libfs.c         |    8 ++++----
> >  include/linux/fs.h |    5 +++--
> >  3 files changed, 41 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index 1bca479..1abaa05 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -18,7 +18,7 @@
> >  /* Taken over from the old code... */
> >  
> >  /* POSIX UID/GID verification for setting inode attributes. */
> > -int inode_change_ok(const struct inode *inode, struct iattr *attr)
> > +int inode_change_posix_ok(const struct inode *inode, struct iattr *attr)
> >  {
> >  	int retval = -EPERM;
> >  	unsigned int ia_valid = attr->ia_valid;
> > @@ -60,7 +60,7 @@ fine:
> >  error:
> >  	return retval;
> >  }
> > -EXPORT_SYMBOL(inode_change_ok);
> > +EXPORT_SYMBOL(inode_change_posix_ok);
> >  
> >  /**
> >   * inode_newsize_ok - may this inode be truncated to a given size
> > @@ -105,6 +105,19 @@ out_big:
> >  }
> >  EXPORT_SYMBOL(inode_newsize_ok);
> >  
> > +/* Generic verification for setting inode attributes. */
> > +int inode_change_attr_ok(const struct inode *inode, struct iattr *attr)
> > +{
> > +	int ret;
> > +	ret = inode_change_posix_ok(inode, attr);
> > +	if (ret)
> > +		return ret;
> > +	if (attr->ia_valid & ATTR_SIZE)
> > +		ret = inode_newsize_ok(inode, attr->ia_size);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(inode_change_attr_ok);
> > +
> >  /**
> >   * generic_setattr - copy simple metadata updates into the generic inode
> >   * @inode:	the inode to be updated
> > @@ -241,17 +254,27 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
> >  	if (inode->i_op && inode->i_op->setattr) {
> >  		error = inode->i_op->setattr(dentry, attr);
> >  	} else {
> > -		error = inode_change_ok(inode, attr);
> > -		if (!error) {
> > -			if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
> > -			    (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid))
> > -				error = vfs_dq_transfer(inode, attr) ?
> > -					-EDQUOT : 0;
> > -			if (!error)
> > -				error = inode_setattr(inode, attr);
> > +		loff_t oldsize = inode->i_size;
> > +		error = inode_change_attr_ok(inode, attr);
> > +		if (error)
> > +			goto out;
> > +		if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
> > +			(ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid)) {
> > +			error = vfs_dq_transfer(inode, attr) ?	-EDQUOT : 0;
> > +			if (error)
> > +				goto out;
> > +		}
> > +		if (attr->ia_valid & ATTR_SIZE &&
> > +			oldsize != attr->ia_size) {
> > +			i_size_write(inode, attr->ia_size);
> > +			truncate_pagecache(inode, oldsize, attr->ia_size);
> > +			if (inode->i_op->truncate)
> > +				inode->i_op->truncate(inode);
> >  		}
> > +		generic_setattr(inode, attr);
> > +		mark_inode_dirty(inode);
> >  	}
> > -
> > +out:
> >  	if (ia_valid & ATTR_SIZE)
> >  		up_write(&dentry->d_inode->i_alloc_sem);
> >  
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index a76934d..98f9a40 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -389,7 +389,7 @@ int simple_setattr(struct dentry *dentry, struct iattr *iattr)
> >  	struct inode *inode = dentry->d_inode;
> >  	int error;
> >  
> > -	error = inode_change_ok(inode, iattr);
> > +	error = inode_change_attr_ok(inode, iattr);
> >  	if (error)
> >  		return error;
> >  
> > @@ -401,9 +401,9 @@ int simple_setattr(struct dentry *dentry, struct iattr *iattr)
> >  	}
> >  
> >  	if (iattr->ia_valid & ATTR_SIZE) {
> > -		error = simple_setsize(inode, iattr->ia_size);
> > -		if (error)
> > -			return error;
> > +		loff_t oldsize = inode->i_size;
> > +		i_size_write(inode, iattr->ia_size);
> > +		truncate_pagecache(inode, oldsize, iattr->ia_size);
> >  	}
> >  
> >  	generic_setattr(inode, iattr);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 81c91d9..fb6809d 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2388,9 +2388,10 @@ extern int buffer_migrate_page(struct address_space *,
> >  #else
> >  #define buffer_migrate_page NULL
> >  #endif
> > -
> > -extern int inode_change_ok(const struct inode *, struct iattr *);
> > +#define inode_change_ok(inode, iattr) inode_change_posix_ok(inode, iattr)
> > +extern int inode_change_posix_ok(const struct inode *, struct iattr *);
> >  extern int inode_newsize_ok(const struct inode *, loff_t offset);
> > +extern int inode_change_attr_ok(const struct inode *, struct iattr *);
> >  extern int __must_check inode_setattr(struct inode *, const struct iattr *);
> >  extern void generic_setattr(struct inode *inode, const struct iattr *attr);
> --
> 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