2021-09-23 15:39:56

by Konstantin Komarov

[permalink] [raw]
Subject: [PATCH v2 0/6] fs/ntfs3: Refactor locking in inode_operations

Speed up work with dir lock.
Theoretically in successful cases those locks aren't needed at all.
But proving the same for error cases is difficult.
So instead of removing them we just move them.

V2:
added patch, that fixes logical error in ntfs_create_inode

Konstantin Komarov (6):
fs/ntfs3: Fix logical error in ntfs_create_inode
fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode
fs/ntfs3: Refactor ntfs_get_acl_ex for better readability
fs/ntfs3: Pass flags to ntfs_set_ea in ntfs_set_acl_ex
fs/ntfs3: Change posix_acl_equiv_mode to posix_acl_update_mode
fs/ntfs3: Refactoring lock in ntfs_init_acl

fs/ntfs3/inode.c | 19 ++++++++---
fs/ntfs3/namei.c | 20 -----------
fs/ntfs3/xattr.c | 88 +++++++++++++++++-------------------------------
3 files changed, 46 insertions(+), 81 deletions(-)

--
2.33.0


2021-09-23 15:42:23

by Konstantin Komarov

[permalink] [raw]
Subject: [PATCH v2 1/6] fs/ntfs3: Fix logical error in ntfs_create_inode

We need to always call indx_delete_entry after indx_insert_entry
if error occurred.

Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index d583b71bec50..d51bf4018835 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -1575,7 +1575,7 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
if (!S_ISLNK(mode) && (sb->s_flags & SB_POSIXACL)) {
err = ntfs_init_acl(mnt_userns, inode, dir);
if (err)
- goto out6;
+ goto out7;
} else
#endif
{
--
2.33.0


2021-09-23 15:51:20

by Konstantin Komarov

[permalink] [raw]
Subject: [PATCH v2 6/6] fs/ntfs3: Refactoring lock in ntfs_init_acl

This is possible because of moving lock into ntfs_create_inode.

Reviewed-by: Kari Argillander <[email protected]>
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/xattr.c | 55 ++++++++++++------------------------------------
1 file changed, 14 insertions(+), 41 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 59ec5e61a239..83bbee277e12 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -693,54 +693,27 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
struct posix_acl *default_acl, *acl;
int err;

- /*
- * TODO: Refactoring lock.
- * ni_lock(dir) ... -> posix_acl_create(dir,...) -> ntfs_get_acl -> ni_lock(dir)
- */
- inode->i_default_acl = NULL;
-
- default_acl = ntfs_get_acl_ex(mnt_userns, dir, ACL_TYPE_DEFAULT, 1);
-
- if (!default_acl || default_acl == ERR_PTR(-EOPNOTSUPP)) {
- inode->i_mode &= ~current_umask();
- err = 0;
- goto out;
- }
-
- if (IS_ERR(default_acl)) {
- err = PTR_ERR(default_acl);
- goto out;
- }
-
- acl = default_acl;
- err = __posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
- if (err < 0)
- goto out1;
- if (!err) {
- posix_acl_release(acl);
- acl = NULL;
- }
-
- if (!S_ISDIR(inode->i_mode)) {
- posix_acl_release(default_acl);
- default_acl = NULL;
- }
+ err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
+ if (err)
+ return err;

- if (default_acl)
+ if (default_acl) {
err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
ACL_TYPE_DEFAULT, 1);
+ posix_acl_release(default_acl);
+ } else {
+ inode->i_default_acl = NULL;
+ }

if (!acl)
inode->i_acl = NULL;
- else if (!err)
- err = ntfs_set_acl_ex(mnt_userns, inode, acl, ACL_TYPE_ACCESS,
- 1);
-
- posix_acl_release(acl);
-out1:
- posix_acl_release(default_acl);
+ else {
+ if (!err)
+ err = ntfs_set_acl_ex(mnt_userns, inode, acl,
+ ACL_TYPE_ACCESS, 1);
+ posix_acl_release(acl);
+ }

-out:
return err;
}
#endif
--
2.33.0


2021-09-23 15:51:28

by Konstantin Komarov

[permalink] [raw]
Subject: [PATCH v2 2/6] fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode

Now ntfs3 locks mutex for smaller time.
Theoretically in successful cases those locks aren't needed at all.
But proving the same for error cases is difficult.
So instead of removing them we just move them.

Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/inode.c | 17 ++++++++++++++---
fs/ntfs3/namei.c | 20 --------------------
2 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index d51bf4018835..7dd162f6a7e2 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -1198,9 +1198,13 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
struct REPARSE_DATA_BUFFER *rp = NULL;
bool rp_inserted = false;

+ ni_lock_dir(dir_ni);
+
dir_root = indx_get_root(&dir_ni->dir, dir_ni, NULL, NULL);
- if (!dir_root)
- return ERR_PTR(-EINVAL);
+ if (!dir_root) {
+ err = -EINVAL;
+ goto out1;
+ }

if (S_ISDIR(mode)) {
/* Use parent's directory attributes. */
@@ -1549,6 +1553,9 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
if (err)
goto out6;

+ /* Unlock parent directory before ntfs_init_acl. */
+ ni_unlock(dir_ni);
+
inode->i_generation = le16_to_cpu(rec->seq);

dir->i_mtime = dir->i_ctime = inode->i_atime;
@@ -1605,8 +1612,10 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
out7:

/* Undo 'indx_insert_entry'. */
+ ni_lock_dir(dir_ni);
indx_delete_entry(&dir_ni->dir, dir_ni, new_de + 1,
le16_to_cpu(new_de->key_size), sbi);
+ /* ni_unlock(dir_ni); will be called later. */
out6:
if (rp_inserted)
ntfs_remove_reparse(sbi, IO_REPARSE_TAG_SYMLINK, &new_de->ref);
@@ -1630,8 +1639,10 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
kfree(rp);

out1:
- if (err)
+ if (err) {
+ ni_unlock(dir_ni);
return ERR_PTR(err);
+ }

unlock_new_inode(inode);

diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c
index 1c475da4e19d..bc741213ad84 100644
--- a/fs/ntfs3/namei.c
+++ b/fs/ntfs3/namei.c
@@ -95,16 +95,11 @@ static struct dentry *ntfs_lookup(struct inode *dir, struct dentry *dentry,
static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, bool excl)
{
- struct ntfs_inode *ni = ntfs_i(dir);
struct inode *inode;

- ni_lock_dir(ni);
-
inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFREG | mode,
0, NULL, 0, NULL);

- ni_unlock(ni);
-
return IS_ERR(inode) ? PTR_ERR(inode) : 0;
}

@@ -116,16 +111,11 @@ static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir,
static int ntfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode, dev_t rdev)
{
- struct ntfs_inode *ni = ntfs_i(dir);
struct inode *inode;

- ni_lock_dir(ni);
-
inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, mode, rdev,
NULL, 0, NULL);

- ni_unlock(ni);
-
return IS_ERR(inode) ? PTR_ERR(inode) : 0;
}

@@ -196,15 +186,10 @@ static int ntfs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
{
u32 size = strlen(symname);
struct inode *inode;
- struct ntfs_inode *ni = ntfs_i(dir);
-
- ni_lock_dir(ni);

inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFLNK | 0777,
0, symname, size, NULL);

- ni_unlock(ni);
-
return IS_ERR(inode) ? PTR_ERR(inode) : 0;
}

@@ -215,15 +200,10 @@ static int ntfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
struct dentry *dentry, umode_t mode)
{
struct inode *inode;
- struct ntfs_inode *ni = ntfs_i(dir);
-
- ni_lock_dir(ni);

inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFDIR | mode,
0, NULL, 0, NULL);

- ni_unlock(ni);
-
return IS_ERR(inode) ? PTR_ERR(inode) : 0;
}

--
2.33.0


2021-09-23 15:53:38

by Konstantin Komarov

[permalink] [raw]
Subject: [PATCH v2 5/6] fs/ntfs3: Change posix_acl_equiv_mode to posix_acl_update_mode

Right now ntfs3 uses posix_acl_equiv_mode instead of
posix_acl_update_mode like all other fs.

Reviewed-by: Kari Argillander <[email protected]>
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/xattr.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 70f2f9eb6b1e..59ec5e61a239 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -559,22 +559,15 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
if (acl) {
umode_t mode = inode->i_mode;

- err = posix_acl_equiv_mode(acl, &mode);
- if (err < 0)
- return err;
+ err = posix_acl_update_mode(mnt_userns, inode, &mode,
+ &acl);
+ if (err)
+ goto out;

if (inode->i_mode != mode) {
inode->i_mode = mode;
mark_inode_dirty(inode);
}
-
- if (!err) {
- /*
- * ACL can be exactly represented in the
- * traditional file mode permission bits.
- */
- acl = NULL;
- }
}
name = XATTR_NAME_POSIX_ACL_ACCESS;
name_len = sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1;
--
2.33.0


2021-09-23 15:55:43

by Konstantin Komarov

[permalink] [raw]
Subject: [PATCH v2 3/6] fs/ntfs3: Refactor ntfs_get_acl_ex for better readability

We can safely move set_cached_acl because it works with NULL acl too.

Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/xattr.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 5c7c5c7a5ec1..3795943efc8e 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -518,12 +518,15 @@ static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns,
/* Translate extended attribute to acl. */
if (err >= 0) {
acl = posix_acl_from_xattr(mnt_userns, buf, err);
- if (!IS_ERR(acl))
- set_cached_acl(inode, type, acl);
+ } else if (err == -ENODATA) {
+ acl = NULL;
} else {
- acl = err == -ENODATA ? NULL : ERR_PTR(err);
+ acl = ERR_PTR(err);
}

+ if (!IS_ERR(acl))
+ set_cached_acl(inode, type, acl);
+
__putname(buf);

return acl;
--
2.33.0


2021-09-23 15:56:44

by Konstantin Komarov

[permalink] [raw]
Subject: [PATCH v2 4/6] fs/ntfs3: Pass flags to ntfs_set_ea in ntfs_set_acl_ex

In case of removing of xattr there must be XATTR_REPLACE flag and
zero length. We already check XATTR_REPLACE in ntfs_set_ea, so
now we pass XATTR_REPLACE to ntfs_set_ea.

Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/xattr.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 3795943efc8e..70f2f9eb6b1e 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -549,6 +549,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
size_t size, name_len;
void *value = NULL;
int err = 0;
+ int flags;

if (S_ISLNK(inode->i_mode))
return -EOPNOTSUPP;
@@ -591,20 +592,24 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
}

if (!acl) {
+ /* Remove xattr if it can be presented via mode. */
size = 0;
value = NULL;
+ flags = XATTR_REPLACE;
} else {
size = posix_acl_xattr_size(acl->a_count);
value = kmalloc(size, GFP_NOFS);
if (!value)
return -ENOMEM;
-
err = posix_acl_to_xattr(mnt_userns, acl, value, size);
if (err < 0)
goto out;
+ flags = 0;
}

- err = ntfs_set_ea(inode, name, name_len, value, size, 0, locked);
+ err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
+ if (err == -ENODATA && !size)
+ err = 0; /* Removing non existed xattr. */
if (!err)
set_cached_acl(inode, type, acl);

--
2.33.0


2021-09-24 11:48:41

by Kari Argillander

[permalink] [raw]
Subject: Re: [PATCH v2 5/6] fs/ntfs3: Change posix_acl_equiv_mode to posix_acl_update_mode

On Thu, Sep 23, 2021 at 06:44:55PM +0300, Konstantin Komarov wrote:
> Right now ntfs3 uses posix_acl_equiv_mode instead of
> posix_acl_update_mode like all other fs.
>
> Reviewed-by: Kari Argillander <[email protected]>
> Signed-off-by: Konstantin Komarov <[email protected]>
> ---
> fs/ntfs3/xattr.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 70f2f9eb6b1e..59ec5e61a239 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -559,22 +559,15 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> if (acl) {
> umode_t mode = inode->i_mode;
>
> - err = posix_acl_equiv_mode(acl, &mode);
> - if (err < 0)
> - return err;
> + err = posix_acl_update_mode(mnt_userns, inode, &mode,
> + &acl);
> + if (err)
> + goto out;

Small nit. Maybe just straight
return err;

Put you can choose if you feel changing it.

>
> if (inode->i_mode != mode) {
> inode->i_mode = mode;
> mark_inode_dirty(inode);
> }
> -
> - if (!err) {
> - /*
> - * ACL can be exactly represented in the
> - * traditional file mode permission bits.
> - */
> - acl = NULL;
> - }
> }
> name = XATTR_NAME_POSIX_ACL_ACCESS;
> name_len = sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1;
> --
> 2.33.0
>
>
>

2021-09-24 12:20:24

by Kari Argillander

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] fs/ntfs3: Refactor locking in inode_operations

On Thu, Sep 23, 2021 at 06:38:39PM +0300, Konstantin Komarov wrote:
> Speed up work with dir lock.
> Theoretically in successful cases those locks aren't needed at all.
> But proving the same for error cases is difficult.
> So instead of removing them we just move them.
>
> V2:
> added patch, that fixes logical error in ntfs_create_inode

Whole series

Reviewed-by: Kari Argillander <[email protected]>

>
> Konstantin Komarov (6):
> fs/ntfs3: Fix logical error in ntfs_create_inode
> fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode
> fs/ntfs3: Refactor ntfs_get_acl_ex for better readability
> fs/ntfs3: Pass flags to ntfs_set_ea in ntfs_set_acl_ex
> fs/ntfs3: Change posix_acl_equiv_mode to posix_acl_update_mode
> fs/ntfs3: Refactoring lock in ntfs_init_acl
>
> fs/ntfs3/inode.c | 19 ++++++++---
> fs/ntfs3/namei.c | 20 -----------
> fs/ntfs3/xattr.c | 88 +++++++++++++++++-------------------------------
> 3 files changed, 46 insertions(+), 81 deletions(-)
>
> --
> 2.33.0
>