[PATCH 0/5] Refactor locking in inode_operations

[PATCH 0/5] Refactor locking in inode_operations

[Konstantin Komarov]

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.

Konstantin Komarov (5):
  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 | 17 ++++++++--
 fs/ntfs3/namei.c | 20 -----------
 fs/ntfs3/xattr.c | 88 +++++++++++++++++-------------------------------
 3 files changed, 45 insertions(+), 80 deletions(-)

-- 
2.33.0

[PATCH 1/5] fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode

[Konstantin Komarov]

Now ntfs3 locks mutex for smaller time.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 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 d583b71bec50..6fc99eebd1c1 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

Re: [PATCH 1/5] fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode

[Kari Argillander]

On Wed, Sep 22, 2021 at 07:17:13PM +0300, Konstantin Komarov wrote:
> Now ntfs3 locks mutex for smaller time.

Really like this change. It was my todo list also. Thanks. Still some
comments below.

> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  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 d583b71bec50..6fc99eebd1c1 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);

There is err path which can go to err6 (line 1585). Then we get double
unlock situation.

> +
>  	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);

This will be double unlock if we exit with err path out6.

  Argillander

>  		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
> 
> 

[PATCH 2/5] fs/ntfs3: Refactor ntfs_get_acl_ex for better readability

[Konstantin Komarov]

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 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

Re: [PATCH 2/5] fs/ntfs3: Refactor ntfs_get_acl_ex for better readability

[Kari Argillander]

On Wed, Sep 22, 2021 at 07:18:18PM +0300, Konstantin Komarov wrote:

There should almoust always still be commit message. Even "small"
change. You have now see that people send you patch which change
just one line, but it can still contain many lines commit message.

> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  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) {

If err was ENODATA ...

>  		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);

Before we get this and we did not call set_cached_acl().

> +		acl = ERR_PTR(err);
>  	}
>  
> +	if (!IS_ERR(acl))

But now we call it with new logic. If this is correct then you change
behavier little bit. I let you talk before I look more into this.

> +		set_cached_acl(inode, type, acl);
> +
>  	__putname(buf);
>  
>  	return acl;
> -- 
> 2.33.0
> 
> 

[PATCH 3/5] fs/ntfs3: Pass flags to ntfs_set_ea in ntfs_set_acl_ex

[Konstantin Komarov]

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 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

Re: [PATCH 3/5] fs/ntfs3: Pass flags to ntfs_set_ea in ntfs_set_acl_ex

[Kari Argillander]

On Wed, Sep 22, 2021 at 07:19:19PM +0300, Konstantin Komarov wrote:
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>

Please tell why we need to pass flags to ntfs_set_ea(). Commit message
is for why we do something. It does not have to have example have any
info what we did. Code will tell that.

> ---
>  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
> 
> 
> 

[PATCH 4/5] fs/ntfs3: Change posix_acl_equiv_mode to posix_acl_update_mode

[Konstantin Komarov]

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

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 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

Re: [PATCH 4/5] fs/ntfs3: Change posix_acl_equiv_mode to posix_acl_update_mode

[Kari Argillander]

On Wed, Sep 22, 2021 at 07:20:05PM +0300, Konstantin Komarov wrote:
> Right now ntfs3 uses posix_acl_equiv_mode instead of
> posix_acl_update_mode like all other fs.
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>

Reviewed-by: Kari Argillander <kari.argillander@gmail.com>

> ---
>  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
> 
> 
> 

[PATCH 5/5] fs/ntfs3: Refactoring lock in ntfs_init_acl

[Konstantin Komarov]

This is possible because of moving lock into ntfs_create_inode.

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 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

Re: [PATCH 5/5] fs/ntfs3: Refactoring lock in ntfs_init_acl

[Kari Argillander]

On Wed, Sep 22, 2021 at 07:20:49PM +0300, Konstantin Komarov wrote:
> This is possible because of moving lock into ntfs_create_inode.
> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>

Looks good.

Reviewed-by: Kari Argillander <kari.argillander@gmail.com>

> ---
>  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
> 
> 
> 

Re: [PATCH 0/5] Refactor locking in inode_operations

[Kari Argillander]

Subject in this message needs fs/ntfs3 minor thing but try to remember
next time.

On Wed, Sep 22, 2021 at 07:15:19PM +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.

Maybe add this info also to first patch. 

Overall nice to see now good patch series which has very nice splits. It
was easy to review. Like I say in same message already try to write
little more to commit messages this will make reviewing even more easy
and we start to get nice history which can be used to develepment and
maintain work.

> 
> Konstantin Komarov (5):
>   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 | 17 ++++++++--
>  fs/ntfs3/namei.c | 20 -----------
>  fs/ntfs3/xattr.c | 88 +++++++++++++++++-------------------------------
>  3 files changed, 45 insertions(+), 80 deletions(-)
> 
> -- 
> 2.33.0