Re: [Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure
On Fri, Dec 06, 2013 at 10:37:34AM +0900, Jaegeuk Kim wrote:
f2fs caches a new mode bit for a while to make the consistency between
xattr's acl mode and the inode mode.
Can you explain what exactly you're trying to do there? I've been
trying to unwrap what's going on and can't really see the point:
- i_acl_mode and FI_ACL_MODE get set in __setattr_copy, but right
after that call, still under i_mutex and before marking the inode
dirty f2fs_acl_chmod makes use of it, and it gets cleared right
after. Is there any race that could happen with a locked inode
not marked dirty yet on f2fs? We could pass a mode argument
to posix_acl_create, but I'd prefer to avoid that if we can.
- on the set_acl side it gets set in __f2fs_set_acl, and then
i_mode is update in __f2fs_setxattr which could easily done with
a stack variable.
RFC patch below:
diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index 4f52fe0f..6647545 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) {
@@ -209,11 +206,11 @@ static int __f2fs_set_acl(struct inode *inode, int type,
struct posix_acl *acl, struct page *ipage)
{
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;
int error;
+ umode_t mode = 0;
if (!test_opt(sbi, POSIX_ACL))
return 0;
@@ -224,10 +221,10 @@ static int __f2fs_set_acl(struct inode *inode, int type,
case ACL_TYPE_ACCESS:
name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
if (acl) {
- error = posix_acl_equiv_mode(acl, inode-i_mode);
+ mode = inode-i_mode;
+ error = posix_acl_equiv_mode(acl, mode);
if (error 0)
return error;
- set_acl_inode(fi, inode-i_mode);
if (error == 0)
acl = NULL;
}
@@ -245,19 +242,15 @@ static int __f2fs_set_acl(struct inode *inode, int type,
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, ipage);
+ error = f2fs_setxattr(inode, name_index, , value, size, ipage, mode);
kfree(value);
if (!error)
set_cached_acl(inode, type, acl);
-
- cond_clear_inode_flag(fi, FI_ACL_MODE);
return error;
}
@@ -289,28 +282,3 @@ int f2fs_init_acl(struct inode *inode, struct inode *dir,
struct page *ipage)
return error;
}
-
-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))
- 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);
- if (error)
- return error;
-
- error = __f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl, NULL);
- posix_acl_release(acl);
- return error;
-}
diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h
index 2af31fe..e086465 100644
--- a/fs/f2fs/acl.h
+++ b/fs/f2fs/acl.h
@@ -38,18 +38,12 @@ struct f2fs_acl_header {
extern struct posix_acl *f2fs_get_acl(struct inode *, int);
extern int f2fs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
-extern int f2fs_acl_chmod(struct inode *);
extern int f2fs_init_acl(struct inode *, struct inode *, struct page *);
#else
#define f2fs_check_acl NULL
#define f2fs_get_acl NULL
#define f2fs_set_acl NULL
-static inline int f2fs_acl_chmod(struct inode *inode)
-{
- return 0;
-}
-
static inline int f2fs_init_acl(struct inode *inode, struct inode *dir,
struct page *page)
{
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 89dc750..1e774e6 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -181,7 +181,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 */
-
Re: [Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure
2013-12-08 (일), 01:14 -0800, Christoph Hellwig:
On Fri, Dec 06, 2013 at 10:37:34AM +0900, Jaegeuk Kim wrote:
f2fs caches a new mode bit for a while to make the consistency between
xattr's acl mode and the inode mode.
Can you explain what exactly you're trying to do there? I've been
trying to unwrap what's going on and can't really see the point:
- i_acl_mode and FI_ACL_MODE get set in __setattr_copy, but right
after that call, still under i_mutex and before marking the inode
dirty f2fs_acl_chmod makes use of it, and it gets cleared right
after. Is there any race that could happen with a locked inode
not marked dirty yet on f2fs?
As you guess, there is no race problem, but the problem is on acl
consistency between xattr-mode and inode-mode.
Previously, f2fs_setattr triggers:
new_mode inode-mode xattr-mode iblock-mode
f2fs_setattr x-x y y
[update_inode] x--- [ y ] --- x
[checkpoint]x y x
__f2fs_setxattr x -x x
In this flow, f2fs is able to break the consistency between xattr-mode
and iblock-mode after checkpoint followed by sudden-power-off.
So, fi-mode was introduced to address the problem.
The new f2fs_setattr triggers:
new_mode inode-mode fi-mode xattr-mode iblock-mode
f2fs_setattr x--- [y] --- x y y
[update_inode] y x y y
[checkpoint]y x y y
__f2fs_setxattr x-x-x - x
Finally, __f2fs_setxattr synchronizes inode-mode, xattr-mode, and
iblock-mode all together.
The root question is is it possible to call update_inode in the
i_mutex-covered region like f2fs_setattr?.
The update_inode of f2fs is called from a bunch of places so currently
I'm not sure it can be impossible.
Thanks,
We could pass a mode argument
to posix_acl_create, but I'd prefer to avoid that if we can.
- on the set_acl side it gets set in __f2fs_set_acl, and then
i_mode is update in __f2fs_setxattr which could easily done with
a stack variable.
RFC patch below:
diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index 4f52fe0f..6647545 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) {
@@ -209,11 +206,11 @@ static int __f2fs_set_acl(struct inode *inode, int type,
struct posix_acl *acl, struct page *ipage)
{
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;
int error;
+ umode_t mode = 0;
if (!test_opt(sbi, POSIX_ACL))
return 0;
@@ -224,10 +221,10 @@ static int __f2fs_set_acl(struct inode *inode, int type,
case ACL_TYPE_ACCESS:
name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
if (acl) {
- error = posix_acl_equiv_mode(acl, inode-i_mode);
+ mode = inode-i_mode;
+ error = posix_acl_equiv_mode(acl, mode);
if (error 0)
return error;
- set_acl_inode(fi, inode-i_mode);
if (error == 0)
acl = NULL;
}
@@ -245,19 +242,15 @@ static int __f2fs_set_acl(struct inode *inode, int type,
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, ipage);
+ error = f2fs_setxattr(inode, name_index, , value, size, ipage, mode);
kfree(value);
if (!error)
set_cached_acl(inode, type, acl);
-
- cond_clear_inode_flag(fi, FI_ACL_MODE);
return error;
}
@@ -289,28 +282,3 @@ int f2fs_init_acl(struct inode *inode, struct inode
*dir, struct page *ipage)
return error;
}
-
-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))
- 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);
-
