Re: [f2fs-dev] [PATCH 1/4] f2fs: reorganize the f2fs_setattr() function.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2013/6/11, Changman Lee <cm224.lee@xxxxxxxxxxx>:
> On 화, 2013-06-11 at 07:57 +0900, Namjae Jeon wrote:
>> 2013/6/10, Changman Lee <cm224.lee@xxxxxxxxxxx>:
>> > 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 ?

----------------------------------------------------------------------------------------------------------------
Subject: [PATCH v2] f2fs: reorganize the f2fs_setattr(), f2fs_set_acl,
f2fs_setxattr()
From: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>

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@xxxxxxxxxxx>
Signed-off-by: Pankaj Kumar <pankaj.km@xxxxxxxxxxx>
---
 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux