Date:	Fri, 21 Jun 2013 13:42:52 +0900
From:	Changman Lee <cm224.lee@...sung.com>
To:	Namjae Jeon <linkinjeon@...il.com>
Cc:	jaegeuk.kim@...sung.com, Namjae Jeon <namjae.jeon@...sung.com>,
	Pankaj Kumar <pankaj.km@...sung.com>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH 1/4] f2fs: reorganize the f2fs_setattr()
 function.

On 금, 2013-06-14 at 13:20 +0900, Namjae Jeon wrote:
> 2013/6/11, Namjae Jeon <linkinjeon@...il.com>:
> > 2013/6/11, Changman Lee <cm224.lee@...sung.com>:
> >> On 화, 2013-06-11 at 07:57 +0900, Namjae Jeon wrote:
> >>> 2013/6/10, Changman Lee <cm224.lee@...sung.com>:
> >>> > Hello, Namjae
> >>> Hi. Changman.
> >>> >
> >>> > If using ACL, whenever i_mode is changed we should update acl_mode
> >>> > which
> >>> > is written to xattr block, too. And vice versa.
> >>> > Because update_inode() is called at any reason and anytime, so we
> >>> > should
> >>> > sync both the moment xattr is written.
> >>> > We don't hope that only i_mode is written to disk and xattr is not. So
> >>> > f2fs_setattr is dirty.
> >>> Yes, agreed this could be issue.
> >>> >
> >>> > And, below code has a bug. When error is occurred, inode->i_mode
> >>> > shouldn't be changed. Please, check one more time, Namjae.
> >>> And, below code has a bug. When error is occurred, inode->i_mode
> >>> shouldn't be changed. Please, check one more time, Namjae.
> >>>
> >>> This was part of the default code, when ‘acl’ is not set for file’
> >>> Then, inode should be updated by these conditions (i.e., it covers the
> >>> ‘chmod’ and ‘setacl’ scenario).
> >>> When ACL is not present on the file and ‘chmod’ is done, then mode is
> >>> changed from this part, as f2fs_get_acl() will fail and cause the
> >>> below code to be executed:
> >>>
> >>>     if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
> >>>              inode->i_mode = fi->i_acl_mode;
> >>>              clear_inode_flag(fi, FI_ACL_MODE);
> >>>       }
> >>>
> >>> Now, in order to make it consistent and work on all scenario we need
> >>> to make further change like this in addition to the patch changes.
> >>> setattr_copy(inode, attr);
> >>>         if (attr->ia_valid & ATTR_MODE) {
> >>> +             set_acl_inode(fi, inode->i_mode);
> >>>               err = f2fs_acl_chmod(inode);
> >>>               if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
> >>>
> >>> Let me know your opinion.
> >>> Thanks.
> >>>
> >>
> >> setattr_copy changes inode->i_mode, this is not our expectation.
> >> So I made redundant __setatt_copy that copy attr->mode to
> >> fi->i_acl_mode.
> >> When acl_mode is reflected in xattr, acl_mode is copied to
> >> inode->i_mode.
> >>
> >> Agree?
> >>
> > Hi Changman.
> >
> > First, Sorry for interrupt.
> > I think that inode i_mode should be updated regardless of f2fs_acl_chmod.
> > Actually I am still not understand the reason why we should use
> > temporarily acl mode(i_acl_mode).
> > I wroted the v2 patch to not use i_acl_mode like this.
> > Am I missing something ?
> To Changman,
> I am still waiting for your reply. Correct us if we are wrong or
> missing something.
> 
> Hi Jaegeuk,
> Could you please share your views on this?
> 
> Thanks.

Sorry for late. I was very busy.

Could you tell me if it happens difference between xattr and i_mode,
what will you do?
The purpose of i_acl_mode is used to update i_mode and xattr together in
same lock region.

> 
> >
> > ----------------------------------------------------------------------------------------------------------------
> > Subject: [PATCH v2] f2fs: reorganize the f2fs_setattr(), f2fs_set_acl,
> > f2fs_setxattr()
> > From: Namjae Jeon <namjae.jeon@...sung.com>
> >
> > Remove the redundant code from f2fs_setattr() function and make it aligned
> > with usages of generic vfs layer function e.g using the setattr_copy()
> > instead of using the f2fs specific function.
> >
> > Also correct the condition for updating the size of file via
> > truncate_setsize().
> >
> > Also modify the code of f2fs_set_acl and f2fs_setxattr for removing the
> > redundant code & add the required changes to correct the requested
> > operations.
> >
> > Remove the variable "i_acl_mode" from the f2fs_inode_info struct since
> > i_mode will
> > hold the latest 'mode' value which can be used for any further
> > references. And in
> > order to make 'chmod' work without ACL support, inode i_mode should be
> > first
> > updated correctly.
> >
> > Remove the helper functions to access and set the i_acl_mode.
> >
> > Signed-off-by: Namjae Jeon <namjae.jeon@...sung.com>
> > Signed-off-by: Pankaj Kumar <pankaj.km@...sung.com>
> > ---
> >  fs/f2fs/acl.c   |   23 +++++++++--------------
> >  fs/f2fs/f2fs.h  |   17 -----------------
> >  fs/f2fs/file.c  |   48 ++++++------------------------------------------
> >  fs/f2fs/xattr.c |    9 ++-------
> >  4 files changed, 17 insertions(+), 80 deletions(-)
> >
> > diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> > index 44abc2f..7ebddf1 100644
> > --- a/fs/f2fs/acl.c
> > +++ b/fs/f2fs/acl.c
> > @@ -17,9 +17,6 @@
> >  #include "xattr.h"
> >  #include "acl.h"
> >
> > -#define get_inode_mode(i)	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
> > -					(F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
> > -
> >  static inline size_t f2fs_acl_size(int count)
> >  {
> >  	if (count <= 4) {
> > @@ -208,7 +205,6 @@ struct posix_acl *f2fs_get_acl(struct inode
> > *inode, int type)
> >  static int f2fs_set_acl(struct inode *inode, int type, struct posix_acl
> > *acl)
> >  {
> >  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> > -	struct f2fs_inode_info *fi = F2FS_I(inode);
> >  	int name_index;
> >  	void *value = NULL;
> >  	size_t size = 0;
> > @@ -226,9 +222,12 @@ static int f2fs_set_acl(struct inode *inode, int
> > type, struct posix_acl *acl)
> >  			error = posix_acl_equiv_mode(acl, &inode->i_mode);
> >  			if (error < 0)
> >  				return error;
> > -			set_acl_inode(fi, inode->i_mode);
> > -			if (error == 0)
> > -				acl = NULL;
> > +			else {
> > +				inode->i_ctime = CURRENT_TIME;
> > +				mark_inode_dirty(inode);
> > +				if (error == 0)
> > +					acl = NULL;
> > +			}
> >  		}
> >  		break;
> >
> > @@ -244,10 +243,8 @@ static int f2fs_set_acl(struct inode *inode, int
> > type, struct posix_acl *acl)
> >
> >  	if (acl) {
> >  		value = f2fs_acl_to_disk(acl, &size);
> > -		if (IS_ERR(value)) {
> > -			cond_clear_inode_flag(fi, FI_ACL_MODE);
> > +		if (IS_ERR(value))
> >  			return (int)PTR_ERR(value);
> > -		}
> >  	}
> >
> >  	error = f2fs_setxattr(inode, name_index, "", value, size);
> > @@ -256,7 +253,6 @@ static int f2fs_set_acl(struct inode *inode, int
> > type, struct posix_acl *acl)
> >  	if (!error)
> >  		set_cached_acl(inode, type, acl);
> >
> > -	cond_clear_inode_flag(fi, FI_ACL_MODE);
> >  	return error;
> >  }
> >
> > @@ -299,18 +295,17 @@ int f2fs_acl_chmod(struct inode *inode)
> >  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> >  	struct posix_acl *acl;
> >  	int error;
> > -	umode_t mode = get_inode_mode(inode);
> >
> >  	if (!test_opt(sbi, POSIX_ACL))
> >  		return 0;
> > -	if (S_ISLNK(mode))
> > +	if (S_ISLNK(inode->i_mode))
> >  		return -EOPNOTSUPP;
> >
> >  	acl = f2fs_get_acl(inode, ACL_TYPE_ACCESS);
> >  	if (IS_ERR(acl) || !acl)
> >  		return PTR_ERR(acl);
> >
> > -	error = posix_acl_chmod(&acl, GFP_KERNEL, mode);
> > +	error = posix_acl_chmod(&acl, GFP_KERNEL, inode->i_mode);
> >  	if (error)
> >  		return error;
> >  	error = f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl);
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 40b137a..241ba31 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -159,7 +159,6 @@ struct f2fs_inode_info {
> >  	unsigned char i_advise;		/* use to give file attribute hints */
> >  	unsigned int i_current_depth;	/* use only in directory structure */
> >  	unsigned int i_pino;		/* parent inode number */
> > -	umode_t i_acl_mode;		/* keep file acl mode temporarily */
> >
> >  	/* Use below internally in f2fs*/
> >  	unsigned long flags;		/* use to pass per-file flags */
> > @@ -857,7 +856,6 @@ static inline int f2fs_clear_bit(unsigned int nr,
> > char *addr)
> >  enum {
> >  	FI_NEW_INODE,		/* indicate newly allocated inode */
> >  	FI_INC_LINK,		/* need to increment i_nlink */
> > -	FI_ACL_MODE,		/* indicate acl mode */
> >  	FI_NO_ALLOC,		/* should not allocate any blocks */
> >  	FI_DELAY_IPUT,		/* used for the recovery */
> >  };
> > @@ -877,21 +875,6 @@ static inline void clear_inode_flag(struct
> > f2fs_inode_info *fi, int flag)
> >  	clear_bit(flag, &fi->flags);
> >  }
> >
> > -static inline void set_acl_inode(struct f2fs_inode_info *fi, umode_t mode)
> > -{
> > -	fi->i_acl_mode = mode;
> > -	set_inode_flag(fi, FI_ACL_MODE);
> > -}
> > -
> > -static inline int cond_clear_inode_flag(struct f2fs_inode_info *fi, int
> > flag)
> > -{
> > -	if (is_inode_flag_set(fi, FI_ACL_MODE)) {
> > -		clear_inode_flag(fi, FI_ACL_MODE);
> > -		return 1;
> > -	}
> > -	return 0;
> > -}
> > -
> >  static inline int f2fs_readonly(struct super_block *sb)
> >  {
> >  	return sb->s_flags & MS_RDONLY;
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index deefd25..4162fbb 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -300,37 +300,6 @@ static int f2fs_getattr(struct vfsmount *mnt,
> >  	return 0;
> >  }
> >
> > -#ifdef CONFIG_F2FS_FS_POSIX_ACL
> > -static void __setattr_copy(struct inode *inode, const struct iattr *attr)
> > -{
> > -	struct f2fs_inode_info *fi = F2FS_I(inode);
> > -	unsigned int ia_valid = attr->ia_valid;
> > -
> > -	if (ia_valid & ATTR_UID)
> > -		inode->i_uid = attr->ia_uid;
> > -	if (ia_valid & ATTR_GID)
> > -		inode->i_gid = attr->ia_gid;
> > -	if (ia_valid & ATTR_ATIME)
> > -		inode->i_atime = timespec_trunc(attr->ia_atime,
> > -						inode->i_sb->s_time_gran);
> > -	if (ia_valid & ATTR_MTIME)
> > -		inode->i_mtime = timespec_trunc(attr->ia_mtime,
> > -						inode->i_sb->s_time_gran);
> > -	if (ia_valid & ATTR_CTIME)
> > -		inode->i_ctime = timespec_trunc(attr->ia_ctime,
> > -						inode->i_sb->s_time_gran);
> > -	if (ia_valid & ATTR_MODE) {
> > -		umode_t mode = attr->ia_mode;
> > -
> > -		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> > -			mode &= ~S_ISGID;
> > -		set_acl_inode(fi, mode);
> > -	}
> > -}
> > -#else
> > -#define __setattr_copy setattr_copy
> > -#endif
> > -
> >  int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
> >  {
> >  	struct inode *inode = dentry->d_inode;
> > @@ -341,24 +310,19 @@ int f2fs_setattr(struct dentry *dentry, struct
> > iattr *attr)
> >  	if (err)
> >  		return err;
> >
> > -	if ((attr->ia_valid & ATTR_SIZE) &&
> > -			attr->ia_size != i_size_read(inode)) {
> > -		truncate_setsize(inode, attr->ia_size);
> > +	if ((attr->ia_valid & ATTR_SIZE)) {
> > +		if (attr->ia_size != i_size_read(inode))
> > +			truncate_setsize(inode, attr->ia_size);
> >  		f2fs_truncate(inode);
> >  		f2fs_balance_fs(F2FS_SB(inode->i_sb));
> >  	}
> >
> > -	__setattr_copy(inode, attr);
> > +	setattr_copy(inode, attr);
> > +	mark_inode_dirty(inode);
> >
> > -	if (attr->ia_valid & ATTR_MODE) {
> > +	if (attr->ia_valid & ATTR_MODE)
> >  		err = f2fs_acl_chmod(inode);
> > -		if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
> > -			inode->i_mode = fi->i_acl_mode;
> > -			clear_inode_flag(fi, FI_ACL_MODE);
> > -		}
> > -	}
> >
> > -	mark_inode_dirty(inode);
> >  	return err;
> >  }
> >
> > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > index 0b02dce..8f02acf 100644
> > --- a/fs/f2fs/xattr.c
> > +++ b/fs/f2fs/xattr.c
> > @@ -333,7 +333,6 @@ int f2fs_setxattr(struct inode *inode, int
> > name_index, const char *name,
> >  			goto exit;
> >  		}
> >  		set_new_dnode(&dn, inode, NULL, NULL, fi->i_xattr_nid);
> > -		mark_inode_dirty(inode);
> >
> >  		page = new_node_page(&dn, XATTR_NODE_OFFSET);
> >  		if (IS_ERR(page)) {
> > @@ -430,12 +429,8 @@ int f2fs_setxattr(struct inode *inode, int
> > name_index, const char *name,
> >  	set_page_dirty(page);
> >  	f2fs_put_page(page, 1);
> >
> > -	if (is_inode_flag_set(fi, FI_ACL_MODE)) {
> > -		inode->i_mode = fi->i_acl_mode;
> > -		inode->i_ctime = CURRENT_TIME;
> > -		clear_inode_flag(fi, FI_ACL_MODE);
> > -	}
> > -	update_inode_page(inode);
> > +	inode->i_ctime = CURRENT_TIME;
> > +	mark_inode_dirty(inode);
> >  	mutex_unlock_op(sbi, ilock);
> >
> >  	return 0;
> > --
> > 1.7.2.3
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists