Removed function, that already have been in kernel.
Changed locking policy to fix some potential bugs.
Changed code for readability.
Konstantin Komarov (3):
fs/ntfs3: Use available posix_acl_release instead of
ntfs_posix_acl_release
fs/ntfs3: Remove locked argument in ntfs_set_ea
fs/ntfs3: Refactoring of ntfs_set_ea
fs/ntfs3/xattr.c | 69 ++++++++++++++++++++++--------------------------
1 file changed, 32 insertions(+), 37 deletions(-)
--
2.33.0
We don't need to maintain ntfs_posix_acl_release.
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/xattr.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 83bbee277e12..253a07d9aa7b 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -475,12 +475,6 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
}
#ifdef CONFIG_NTFS3_FS_POSIX_ACL
-static inline void ntfs_posix_acl_release(struct posix_acl *acl)
-{
- if (acl && refcount_dec_and_test(&acl->a_refcount))
- kfree(acl);
-}
-
static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns,
struct inode *inode, int type,
int locked)
@@ -641,7 +635,7 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
return -ENODATA;
err = posix_acl_to_xattr(mnt_userns, acl, buffer, size);
- ntfs_posix_acl_release(acl);
+ posix_acl_release(acl);
return err;
}
@@ -678,7 +672,7 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
err = ntfs_set_acl(mnt_userns, inode, acl, type);
release_and_out:
- ntfs_posix_acl_release(acl);
+ posix_acl_release(acl);
return err;
}
--
2.33.0
We always need to lock now, because locks became smaller
(see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/xattr.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 253a07d9aa7b..1ab109723b10 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
static noinline int ntfs_set_ea(struct inode *inode, const char *name,
size_t name_len, const void *value,
- size_t val_size, int flags, int locked)
+ size_t val_size, int flags)
{
struct ntfs_inode *ni = ntfs_i(inode);
struct ntfs_sb_info *sbi = ni->mi.sbi;
@@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
u64 new_sz;
void *p;
- if (!locked)
- ni_lock(ni);
+ ni_lock(ni);
run_init(&ea_run);
@@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
mark_inode_dirty(&ni->vfs_inode);
out:
- if (!locked)
- ni_unlock(ni);
+ ni_unlock(ni);
run_close(&ea_run);
kfree(ea_all);
@@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
struct inode *inode, struct posix_acl *acl,
- int type, int locked)
+ int type)
{
const char *name;
size_t size, name_len;
@@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
flags = 0;
}
- err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
+ err = ntfs_set_ea(inode, name, name_len, value, size, flags);
if (err == -ENODATA && !size)
err = 0; /* Removing non existed xattr. */
if (!err)
@@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
struct posix_acl *acl, int type)
{
- return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
+ return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
}
static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
@@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
if (default_acl) {
err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
- ACL_TYPE_DEFAULT, 1);
+ ACL_TYPE_DEFAULT);
posix_acl_release(default_acl);
} else {
inode->i_default_acl = NULL;
@@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
else {
if (!err)
err = ntfs_set_acl_ex(mnt_userns, inode, acl,
- ACL_TYPE_ACCESS, 1);
+ ACL_TYPE_ACCESS);
posix_acl_release(acl);
}
@@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
}
#endif
/* Deal with NTFS extended attribute. */
- err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
+ err = ntfs_set_ea(inode, name, name_len, value, size, flags);
out:
return err;
@@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
value = cpu_to_le32(i_uid_read(inode));
err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
- sizeof(value), 0, 0);
+ sizeof(value), 0);
if (err)
goto out;
value = cpu_to_le32(i_gid_read(inode));
err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
- sizeof(value), 0, 0);
+ sizeof(value), 0);
if (err)
goto out;
value = cpu_to_le32(inode->i_mode);
err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
- sizeof(value), 0, 0);
+ sizeof(value), 0);
if (err)
goto out;
if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
value = cpu_to_le32(inode->i_rdev);
err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
- sizeof(value), 0, 0);
+ sizeof(value), 0);
if (err)
goto out;
}
--
2.33.0
Make code more readable.
Don't try to read zero bytes.
Add warning when size of exteneded attribute exceeds limit.
Signed-off-by: Konstantin Komarov <[email protected]>
---
fs/ntfs3/xattr.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 1ab109723b10..395e71291e28 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -75,6 +75,7 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
size_t add_bytes, const struct EA_INFO **info)
{
int err;
+ struct ntfs_sb_info *sbi = ni->mi.sbi;
struct ATTR_LIST_ENTRY *le = NULL;
struct ATTRIB *attr_info, *attr_ea;
void *ea_p;
@@ -99,10 +100,10 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
/* Check Ea limit. */
size = le32_to_cpu((*info)->size);
- if (size > ni->mi.sbi->ea_max_size)
+ if (size > sbi->ea_max_size)
return -EFBIG;
- if (attr_size(attr_ea) > ni->mi.sbi->ea_max_size)
+ if (attr_size(attr_ea) > sbi->ea_max_size)
return -EFBIG;
/* Allocate memory for packed Ea. */
@@ -110,15 +111,16 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
if (!ea_p)
return -ENOMEM;
- if (attr_ea->non_res) {
+ if (!size) {
+ ;
+ } else if (attr_ea->non_res) {
struct runs_tree run;
run_init(&run);
err = attr_load_runs(attr_ea, ni, &run, NULL);
if (!err)
- err = ntfs_read_run_nb(ni->mi.sbi, &run, 0, ea_p, size,
- NULL);
+ err = ntfs_read_run_nb(sbi, &run, 0, ea_p, size, NULL);
run_close(&run);
if (err)
@@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
new_ea->name[name_len] = 0;
memcpy(new_ea->name + name_len + 1, value, val_size);
new_pack = le16_to_cpu(ea_info.size_pack) + packed_ea_size(new_ea);
-
- /* Should fit into 16 bits. */
- if (new_pack > 0xffff) {
- err = -EFBIG; // -EINVAL?
- goto out;
- }
ea_info.size_pack = cpu_to_le16(new_pack);
-
/* New size of ATTR_EA. */
size += add;
- if (size > sbi->ea_max_size) {
+ ea_info.size = cpu_to_le32(size);
+
+ /*
+ * 1. Check ea_info.size_pack for overflow.
+ * 2. New attibute size must fit value from $AttrDef
+ */
+ if (new_pack > 0xffff || size > sbi->ea_max_size) {
+ ntfs_inode_warn(
+ inode,
+ "The size of exteneded attributes must not exceed 64K");
err = -EFBIG; // -EINVAL?
goto out;
}
- ea_info.size = cpu_to_le32(size);
update_ea:
--
2.33.0
On Fri, 2021-09-24 at 19:16 +0300, Konstantin Komarov wrote:
> Make code more readable.
> Don't try to read zero bytes.
> Add warning when size of exteneded attribute exceeds limit.
[]
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
[]
> @@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
[]
> + ea_info.size = cpu_to_le32(size);
> +
> + /*
> + * 1. Check ea_info.size_pack for overflow.
> + * 2. New attibute size must fit value from $AttrDef
> + */
> + if (new_pack > 0xffff || size > sbi->ea_max_size) {
> + ntfs_inode_warn(
> + inode,
> + "The size of exteneded attributes must not exceed 64K");
trivial typo of extended. Pedants might suggest KiB.
On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
> We always need to lock now, because locks became smaller
> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
So basically this actually fixes that commit?
Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
Or if you do not use fixes atleast use
d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
You can add these to your gitconfig
[core]
abbrev = 12
[pretty]
fixes = Fixes: %h (\"%s\")
fixed = Fixes: %h (\"%s\")
And get this annotation with
git show --pretty=fixes <sha>
Have some comments below also.
>
> Signed-off-by: Konstantin Komarov <[email protected]>
> ---
> fs/ntfs3/xattr.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 253a07d9aa7b..1ab109723b10 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
>
> static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> size_t name_len, const void *value,
> - size_t val_size, int flags, int locked)
> + size_t val_size, int flags)
> {
> struct ntfs_inode *ni = ntfs_i(inode);
> struct ntfs_sb_info *sbi = ni->mi.sbi;
> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> u64 new_sz;
> void *p;
>
> - if (!locked)
> - ni_lock(ni);
> + ni_lock(ni);
>
> run_init(&ea_run);
>
> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> mark_inode_dirty(&ni->vfs_inode);
>
> out:
> - if (!locked)
> - ni_unlock(ni);
> + ni_unlock(ni);
>
> run_close(&ea_run);
> kfree(ea_all);
> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
>
> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> struct inode *inode, struct posix_acl *acl,
> - int type, int locked)
> + int type)
> {
> const char *name;
> size_t size, name_len;
> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> flags = 0;
> }
>
> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> if (err == -ENODATA && !size)
> err = 0; /* Removing non existed xattr. */
> if (!err)
> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> struct posix_acl *acl, int type)
> {
> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
> }
>
> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>
> if (default_acl) {
> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> - ACL_TYPE_DEFAULT, 1);
> + ACL_TYPE_DEFAULT);
> posix_acl_release(default_acl);
> } else {
> inode->i_default_acl = NULL;
> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> else {
> if (!err)
> err = ntfs_set_acl_ex(mnt_userns, inode, acl,
> - ACL_TYPE_ACCESS, 1);
> + ACL_TYPE_ACCESS);
> posix_acl_release(acl);
> }
>
> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
> }
> #endif
> /* Deal with NTFS extended attribute. */
> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>
> out:
> return err;
> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
>
> value = cpu_to_le32(i_uid_read(inode));
> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
> - sizeof(value), 0, 0);
> + sizeof(value), 0);
> if (err)
> goto out;
>
> value = cpu_to_le32(i_gid_read(inode));
> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
> - sizeof(value), 0, 0);
> + sizeof(value), 0);
> if (err)
> goto out;
>
> value = cpu_to_le32(inode->i_mode);
> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
> - sizeof(value), 0, 0);
> + sizeof(value), 0);
> if (err)
> goto out;
>
> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
> value = cpu_to_le32(inode->i_rdev);
> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
> - sizeof(value), 0, 0);
> + sizeof(value), 0);
Is this really that we can lock/unlock same lock 4 times in a row in a
ntfs_set_ea? This does not feel correct.
Argillander
> if (err)
> goto out;
> }
> --
> 2.33.0
>
>
On Fri, Sep 24, 2021 at 07:14:53PM +0300, Konstantin Komarov wrote:
> We don't need to maintain ntfs_posix_acl_release.
>
> Signed-off-by: Konstantin Komarov <[email protected]>
Reviewed-by: Kari Argillander <[email protected]>
> ---
> fs/ntfs3/xattr.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 83bbee277e12..253a07d9aa7b 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -475,12 +475,6 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> }
>
> #ifdef CONFIG_NTFS3_FS_POSIX_ACL
> -static inline void ntfs_posix_acl_release(struct posix_acl *acl)
> -{
> - if (acl && refcount_dec_and_test(&acl->a_refcount))
> - kfree(acl);
> -}
> -
> static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns,
> struct inode *inode, int type,
> int locked)
> @@ -641,7 +635,7 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> return -ENODATA;
>
> err = posix_acl_to_xattr(mnt_userns, acl, buffer, size);
> - ntfs_posix_acl_release(acl);
> + posix_acl_release(acl);
>
> return err;
> }
> @@ -678,7 +672,7 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
> err = ntfs_set_acl(mnt_userns, inode, acl, type);
>
> release_and_out:
> - ntfs_posix_acl_release(acl);
> + posix_acl_release(acl);
> return err;
> }
>
> --
> 2.33.0
>
>
On 25.09.2021 11:49, Kari Argillander wrote:
> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
>> We always need to lock now, because locks became smaller
>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
>
> So basically this actually fixes that commit?
>
> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
>
> Or if you do not use fixes atleast use
>
> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
>
> You can add these to your gitconfig
>
> [core]
> abbrev = 12
> [pretty]
> fixes = Fixes: %h (\"%s\")
> fixed = Fixes: %h (\"%s\")
>
> And get this annotation with
> git show --pretty=fixes <sha>
>
> Have some comments below also.
>
>>
>> Signed-off-by: Konstantin Komarov <[email protected]>
>> ---
>> fs/ntfs3/xattr.c | 28 +++++++++++++---------------
>> 1 file changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
>> index 253a07d9aa7b..1ab109723b10 100644
>> --- a/fs/ntfs3/xattr.c
>> +++ b/fs/ntfs3/xattr.c
>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
>>
>> static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>> size_t name_len, const void *value,
>> - size_t val_size, int flags, int locked)
>> + size_t val_size, int flags)
>> {
>> struct ntfs_inode *ni = ntfs_i(inode);
>> struct ntfs_sb_info *sbi = ni->mi.sbi;
>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>> u64 new_sz;
>> void *p;
>>
>> - if (!locked)
>> - ni_lock(ni);
>> + ni_lock(ni);
>>
>> run_init(&ea_run);
>>
>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>> mark_inode_dirty(&ni->vfs_inode);
>>
>> out:
>> - if (!locked)
>> - ni_unlock(ni);
>> + ni_unlock(ni);
>>
>> run_close(&ea_run);
>> kfree(ea_all);
>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
>>
>> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>> struct inode *inode, struct posix_acl *acl,
>> - int type, int locked)
>> + int type)
>> {
>> const char *name;
>> size_t size, name_len;
>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>> flags = 0;
>> }
>>
>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>> if (err == -ENODATA && !size)
>> err = 0; /* Removing non existed xattr. */
>> if (!err)
>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>> struct posix_acl *acl, int type)
>> {
>> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
>> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
>> }
>>
>> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>
>> if (default_acl) {
>> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
>> - ACL_TYPE_DEFAULT, 1);
>> + ACL_TYPE_DEFAULT);
>> posix_acl_release(default_acl);
>> } else {
>> inode->i_default_acl = NULL;
>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>> else {
>> if (!err)
>> err = ntfs_set_acl_ex(mnt_userns, inode, acl,
>> - ACL_TYPE_ACCESS, 1);
>> + ACL_TYPE_ACCESS);
>> posix_acl_release(acl);
>> }
>>
>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>> }
>> #endif
>> /* Deal with NTFS extended attribute. */
>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>>
>> out:
>> return err;
>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
>>
>> value = cpu_to_le32(i_uid_read(inode));
>> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
>> - sizeof(value), 0, 0);
>> + sizeof(value), 0);
>> if (err)
>> goto out;
>>
>> value = cpu_to_le32(i_gid_read(inode));
>> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
>> - sizeof(value), 0, 0);
>> + sizeof(value), 0);
>> if (err)
>> goto out;
>>
>> value = cpu_to_le32(inode->i_mode);
>> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
>> - sizeof(value), 0, 0);
>> + sizeof(value), 0);
>> if (err)
>> goto out;
>>
>> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
>> value = cpu_to_le32(inode->i_rdev);
>> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
>> - sizeof(value), 0, 0);
>> + sizeof(value), 0);
>
> Is this really that we can lock/unlock same lock 4 times in a row in a
> ntfs_set_ea? This does not feel correct.
>
> Argillander
>
How it was working before d562e901f25d
"fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":
ntfs_create (lock mutex) =>
ntfs_create_inode =>
ntfs_save_wsl_perm (we are under lock here) =>
return to ntfs_create and unlock
How it works with d562e901f25d:
ntfs_create =>
ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c
and unlock in line 1557) =>
ntfs_save_wsl_perm (we aren't under lock here in line 1605)
So we need to lock 4 times because there are 4 ntfs_set_ea calls.
But now there can be done more work between those calls
in other threads, locks became more granular.
>> if (err)
>> goto out;
>> }
>> --
>> 2.33.0
>>
>>
On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote:
>
>
> On 25.09.2021 11:49, Kari Argillander wrote:
> > On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
> >> We always need to lock now, because locks became smaller
> >> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
> >
> > So basically this actually fixes that commit?
> >
> > Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> >
> > Or if you do not use fixes atleast use
> >
> > d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> >
> > You can add these to your gitconfig
> >
> > [core]
> > abbrev = 12
> > [pretty]
> > fixes = Fixes: %h (\"%s\")
> > fixed = Fixes: %h (\"%s\")
> >
> > And get this annotation with
> > git show --pretty=fixes <sha>
> >
> > Have some comments below also.
> >
> >>
> >> Signed-off-by: Konstantin Komarov <[email protected]>
> >> ---
> >> fs/ntfs3/xattr.c | 28 +++++++++++++---------------
> >> 1 file changed, 13 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> >> index 253a07d9aa7b..1ab109723b10 100644
> >> --- a/fs/ntfs3/xattr.c
> >> +++ b/fs/ntfs3/xattr.c
> >> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
> >>
> >> static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >> size_t name_len, const void *value,
> >> - size_t val_size, int flags, int locked)
Maybe we should leave int locked and ...
> >> + size_t val_size, int flags)
> >> {
> >> struct ntfs_inode *ni = ntfs_i(inode);
> >> struct ntfs_sb_info *sbi = ni->mi.sbi;
> >> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >> u64 new_sz;
> >> void *p;
> >>
> >> - if (!locked)
> >> - ni_lock(ni);
> >> + ni_lock(ni);
> >>
> >> run_init(&ea_run);
> >>
> >> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >> mark_inode_dirty(&ni->vfs_inode);
> >>
> >> out:
> >> - if (!locked)
> >> - ni_unlock(ni);
> >> + ni_unlock(ni);
> >>
> >> run_close(&ea_run);
> >> kfree(ea_all);
> >> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
> >>
> >> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >> struct inode *inode, struct posix_acl *acl,
> >> - int type, int locked)
> >> + int type)
> >> {
> >> const char *name;
> >> size_t size, name_len;
> >> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >> flags = 0;
> >> }
> >>
> >> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
> >> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >> if (err == -ENODATA && !size)
> >> err = 0; /* Removing non existed xattr. */
> >> if (!err)
> >> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >> struct posix_acl *acl, int type)
> >> {
> >> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
> >> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
> >> }
> >>
> >> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> >> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>
> >> if (default_acl) {
> >> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> >> - ACL_TYPE_DEFAULT, 1);
> >> + ACL_TYPE_DEFAULT);
> >> posix_acl_release(default_acl);
> >> } else {
> >> inode->i_default_acl = NULL;
> >> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >> else {
> >> if (!err)
> >> err = ntfs_set_acl_ex(mnt_userns, inode, acl,
> >> - ACL_TYPE_ACCESS, 1);
> >> + ACL_TYPE_ACCESS);
> >> posix_acl_release(acl);
> >> }
> >>
> >> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
> >> }
> >> #endif
> >> /* Deal with NTFS extended attribute. */
> >> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
> >> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >>
> >> out:
> >> return err;
> >> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
> >>
do lock here and ...
> >> value = cpu_to_le32(i_uid_read(inode));
> >> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
> >> - sizeof(value), 0, 0);
> >> + sizeof(value), 0);
> >> if (err)
> >> goto out;
> >>
> >> value = cpu_to_le32(i_gid_read(inode));
> >> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
> >> - sizeof(value), 0, 0);
> >> + sizeof(value), 0);
> >> if (err)
> >> goto out;
> >>
> >> value = cpu_to_le32(inode->i_mode);
> >> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
> >> - sizeof(value), 0, 0);
> >> + sizeof(value), 0);
> >> if (err)
> >> goto out;
> >>
> >> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
> >> value = cpu_to_le32(inode->i_rdev);
> >> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
> >> - sizeof(value), 0, 0);
> >> + sizeof(value), 0);
unlock here. Of course unlock also in error path.
> >
> > Is this really that we can lock/unlock same lock 4 times in a row in a
> > ntfs_set_ea? This does not feel correct.
> >
> > Argillander
> >
>
> How it was working before d562e901f25d
> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":
>
> ntfs_create (lock mutex) =>
> ntfs_create_inode =>
> ntfs_save_wsl_perm (we are under lock here) =>
> return to ntfs_create and unlock
>
> How it works with d562e901f25d:
>
> ntfs_create =>
> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c
> and unlock in line 1557) =>
> ntfs_save_wsl_perm (we aren't under lock here in line 1605)
>
> So we need to lock 4 times because there are 4 ntfs_set_ea calls.
> But now there can be done more work between those calls
> in other threads, locks became more granular.
Yeah but locking and locking 4 times when we can do it just ones is
quite waste. Please consider my suggestion above or tell what is wrong
with it.
Argillander
>
> >> if (err)
> >> goto out;
> >> }
> >> --
> >> 2.33.0
> >>
> >>
On 27.09.2021 22:10, Kari Argillander wrote:
> On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote:
>>
>>
>> On 25.09.2021 11:49, Kari Argillander wrote:
>>> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
>>>> We always need to lock now, because locks became smaller
>>>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
>>>
>>> So basically this actually fixes that commit?
>>>
>>> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
>>>
>>> Or if you do not use fixes atleast use
>>>
>>> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
>>>
>>> You can add these to your gitconfig
>>>
>>> [core]
>>> abbrev = 12
>>> [pretty]
>>> fixes = Fixes: %h (\"%s\")
>>> fixed = Fixes: %h (\"%s\")
>>>
>>> And get this annotation with
>>> git show --pretty=fixes <sha>
>>>
>>> Have some comments below also.
>>>
>>>>
>>>> Signed-off-by: Konstantin Komarov <[email protected]>
>>>> ---
>>>> fs/ntfs3/xattr.c | 28 +++++++++++++---------------
>>>> 1 file changed, 13 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
>>>> index 253a07d9aa7b..1ab109723b10 100644
>>>> --- a/fs/ntfs3/xattr.c
>>>> +++ b/fs/ntfs3/xattr.c
>>>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
>>>>
>>>> static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>>> size_t name_len, const void *value,
>>>> - size_t val_size, int flags, int locked)
>
> Maybe we should leave int locked and ...
>
>>>> + size_t val_size, int flags)
>>>> {
>>>> struct ntfs_inode *ni = ntfs_i(inode);
>>>> struct ntfs_sb_info *sbi = ni->mi.sbi;
>>>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>>> u64 new_sz;
>>>> void *p;
>>>>
>>>> - if (!locked)
>>>> - ni_lock(ni);
>>>> + ni_lock(ni);
>>>>
>>>> run_init(&ea_run);
>>>>
>>>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
>>>> mark_inode_dirty(&ni->vfs_inode);
>>>>
>>>> out:
>>>> - if (!locked)
>>>> - ni_unlock(ni);
>>>> + ni_unlock(ni);
>>>>
>>>> run_close(&ea_run);
>>>> kfree(ea_all);
>>>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
>>>>
>>>> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>>> struct inode *inode, struct posix_acl *acl,
>>>> - int type, int locked)
>>>> + int type)
>>>> {
>>>> const char *name;
>>>> size_t size, name_len;
>>>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>>> flags = 0;
>>>> }
>>>>
>>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
>>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>>>> if (err == -ENODATA && !size)
>>>> err = 0; /* Removing non existed xattr. */
>>>> if (!err)
>>>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
>>>> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>>> struct posix_acl *acl, int type)
>>>> {
>>>> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
>>>> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
>>>> }
>>>>
>>>> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
>>>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>>>
>>>> if (default_acl) {
>>>> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
>>>> - ACL_TYPE_DEFAULT, 1);
>>>> + ACL_TYPE_DEFAULT);
>>>> posix_acl_release(default_acl);
>>>> } else {
>>>> inode->i_default_acl = NULL;
>>>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
>>>> else {
>>>> if (!err)
>>>> err = ntfs_set_acl_ex(mnt_userns, inode, acl,
>>>> - ACL_TYPE_ACCESS, 1);
>>>> + ACL_TYPE_ACCESS);
>>>> posix_acl_release(acl);
>>>> }
>>>>
>>>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>>>> }
>>>> #endif
>>>> /* Deal with NTFS extended attribute. */
>>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
>>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>>>>
>>>> out:
>>>> return err;
>>>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
>>>>
>
> do lock here and ...
>
>>>> value = cpu_to_le32(i_uid_read(inode));
>>>> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
>>>> - sizeof(value), 0, 0);
>>>> + sizeof(value), 0);
>>>> if (err)
>>>> goto out;
>>>>
>>>> value = cpu_to_le32(i_gid_read(inode));
>>>> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
>>>> - sizeof(value), 0, 0);
>>>> + sizeof(value), 0);
>>>> if (err)
>>>> goto out;
>>>>
>>>> value = cpu_to_le32(inode->i_mode);
>>>> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
>>>> - sizeof(value), 0, 0);
>>>> + sizeof(value), 0);
>>>> if (err)
>>>> goto out;
>>>>
>>>> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
>>>> value = cpu_to_le32(inode->i_rdev);
>>>> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
>>>> - sizeof(value), 0, 0);
>>>> + sizeof(value), 0);
>
> unlock here. Of course unlock also in error path.
>
>>>
>>> Is this really that we can lock/unlock same lock 4 times in a row in a
>>> ntfs_set_ea? This does not feel correct.
>>>
>>> Argillander
>>>
>>
>> How it was working before d562e901f25d
>> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":
>>
>> ntfs_create (lock mutex) =>
>> ntfs_create_inode =>
>> ntfs_save_wsl_perm (we are under lock here) =>
>> return to ntfs_create and unlock
>>
>> How it works with d562e901f25d:
>>
>> ntfs_create =>
>> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c
>> and unlock in line 1557) =>
>> ntfs_save_wsl_perm (we aren't under lock here in line 1605)
>>
>> So we need to lock 4 times because there are 4 ntfs_set_ea calls.
>> But now there can be done more work between those calls
>> in other threads, locks became more granular.
>
> Yeah but locking and locking 4 times when we can do it just ones is
> quite waste. Please consider my suggestion above or tell what is wrong
> with it.
>
> Argillander
>
If I've understood correctly, you want to lock once in start of function
ntfs_save_wsl_perm and unlock at the end of it.
It takes care of locks for those 4 calls to ntfs_set_ea.
But there are still other calls to ntfs_set_ea, that aren't protected
in this case:
function ntfs_set_acl_ex (line 603 file fs/ntfs3/xattr.c)
err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
This function called there:
- function ntfs_set_acl (line 621 file fs/ntfs3/xattr.c)
return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
- function ntfs_init_acl (line 701 file fs/ntfs3/xattr.c)
err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
- function ntfs_init_acl (line 712 file fs/ntfs3/xattr.c)
err = ntfs_set_acl_ex(mnt_userns, inode, acl,
So there are many entry points to ntfs_set_ea (and can be added more).
Of course we can let int locked remain for these situations,
but in my opinion it makes code a lot less readable
(that was the reason to start looking into locking).
I'm not sure, that winning some lock/unlocking
in one specific scenario is worth it.
>>
>>>> if (err)
>>>> goto out;
>>>> }
>>>> --
>>>> 2.33.0
>>>>
>>>>
On Tue, Sep 28, 2021 at 09:01:38PM +0300, Konstantin Komarov wrote:
>
>
> On 27.09.2021 22:10, Kari Argillander wrote:
> > On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote:
> >>
> >>
> >> On 25.09.2021 11:49, Kari Argillander wrote:
> >>> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote:
> >>>> We always need to lock now, because locks became smaller
> >>>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode").
> >>>
> >>> So basically this actually fixes that commit?
> >>>
> >>> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> >>>
> >>> Or if you do not use fixes atleast use
> >>>
> >>> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode")
> >>>
> >>> You can add these to your gitconfig
> >>>
> >>> [core]
> >>> abbrev = 12
> >>> [pretty]
> >>> fixes = Fixes: %h (\"%s\")
> >>> fixed = Fixes: %h (\"%s\")
> >>>
> >>> And get this annotation with
> >>> git show --pretty=fixes <sha>
> >>>
> >>> Have some comments below also.
> >>>
> >>>>
> >>>> Signed-off-by: Konstantin Komarov <[email protected]>
> >>>> ---
> >>>> fs/ntfs3/xattr.c | 28 +++++++++++++---------------
> >>>> 1 file changed, 13 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> >>>> index 253a07d9aa7b..1ab109723b10 100644
> >>>> --- a/fs/ntfs3/xattr.c
> >>>> +++ b/fs/ntfs3/xattr.c
> >>>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len,
> >>>>
> >>>> static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>>> size_t name_len, const void *value,
> >>>> - size_t val_size, int flags, int locked)
> >
> > Maybe we should leave int locked and ...
> >
> >>>> + size_t val_size, int flags)
> >>>> {
> >>>> struct ntfs_inode *ni = ntfs_i(inode);
> >>>> struct ntfs_sb_info *sbi = ni->mi.sbi;
> >>>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>>> u64 new_sz;
> >>>> void *p;
> >>>>
> >>>> - if (!locked)
> >>>> - ni_lock(ni);
> >>>> + ni_lock(ni);
> >>>>
> >>>> run_init(&ea_run);
> >>>>
> >>>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
> >>>> mark_inode_dirty(&ni->vfs_inode);
> >>>>
> >>>> out:
> >>>> - if (!locked)
> >>>> - ni_unlock(ni);
> >>>> + ni_unlock(ni);
> >>>>
> >>>> run_close(&ea_run);
> >>>> kfree(ea_all);
> >>>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type)
> >>>>
> >>>> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>>> struct inode *inode, struct posix_acl *acl,
> >>>> - int type, int locked)
> >>>> + int type)
> >>>> {
> >>>> const char *name;
> >>>> size_t size, name_len;
> >>>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>>> flags = 0;
> >>>> }
> >>>>
> >>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
> >>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >>>> if (err == -ENODATA && !size)
> >>>> err = 0; /* Removing non existed xattr. */
> >>>> if (!err)
> >>>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns,
> >>>> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>>> struct posix_acl *acl, int type)
> >>>> {
> >>>> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0);
> >>>> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
> >>>> }
> >>>>
> >>>> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> >>>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>>>
> >>>> if (default_acl) {
> >>>> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> >>>> - ACL_TYPE_DEFAULT, 1);
> >>>> + ACL_TYPE_DEFAULT);
> >>>> posix_acl_release(default_acl);
> >>>> } else {
> >>>> inode->i_default_acl = NULL;
> >>>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
> >>>> else {
> >>>> if (!err)
> >>>> err = ntfs_set_acl_ex(mnt_userns, inode, acl,
> >>>> - ACL_TYPE_ACCESS, 1);
> >>>> + ACL_TYPE_ACCESS);
> >>>> posix_acl_release(acl);
> >>>> }
> >>>>
> >>>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
> >>>> }
> >>>> #endif
> >>>> /* Deal with NTFS extended attribute. */
> >>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
> >>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags);
> >>>>
> >>>> out:
> >>>> return err;
> >>>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode)
> >>>>
> >
> > do lock here and ...
> >
> >>>> value = cpu_to_le32(i_uid_read(inode));
> >>>> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
> >>>> - sizeof(value), 0, 0);
> >>>> + sizeof(value), 0);
> >>>> if (err)
> >>>> goto out;
> >>>>
> >>>> value = cpu_to_le32(i_gid_read(inode));
> >>>> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
> >>>> - sizeof(value), 0, 0);
> >>>> + sizeof(value), 0);
> >>>> if (err)
> >>>> goto out;
> >>>>
> >>>> value = cpu_to_le32(inode->i_mode);
> >>>> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
> >>>> - sizeof(value), 0, 0);
> >>>> + sizeof(value), 0);
> >>>> if (err)
> >>>> goto out;
> >>>>
> >>>> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
> >>>> value = cpu_to_le32(inode->i_rdev);
> >>>> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value,
> >>>> - sizeof(value), 0, 0);
> >>>> + sizeof(value), 0);
> >
> > unlock here. Of course unlock also in error path.
> >
> >>>
> >>> Is this really that we can lock/unlock same lock 4 times in a row in a
> >>> ntfs_set_ea? This does not feel correct.
> >>>
> >>> Argillander
> >>>
> >>
> >> How it was working before d562e901f25d
> >> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode":
> >>
> >> ntfs_create (lock mutex) =>
> >> ntfs_create_inode =>
> >> ntfs_save_wsl_perm (we are under lock here) =>
> >> return to ntfs_create and unlock
> >>
> >> How it works with d562e901f25d:
> >>
> >> ntfs_create =>
> >> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c
> >> and unlock in line 1557) =>
> >> ntfs_save_wsl_perm (we aren't under lock here in line 1605)
> >>
> >> So we need to lock 4 times because there are 4 ntfs_set_ea calls.
> >> But now there can be done more work between those calls
> >> in other threads, locks became more granular.
> >
> > Yeah but locking and locking 4 times when we can do it just ones is
> > quite waste. Please consider my suggestion above or tell what is wrong
> > with it.
> >
> > Argillander
> >
>
> If I've understood correctly, you want to lock once in start of function
> ntfs_save_wsl_perm and unlock at the end of it.
> It takes care of locks for those 4 calls to ntfs_set_ea.
>
> But there are still other calls to ntfs_set_ea, that aren't protected
> in this case:
> function ntfs_set_acl_ex (line 603 file fs/ntfs3/xattr.c)
> err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked);
>
> This function called there:
> - function ntfs_set_acl (line 621 file fs/ntfs3/xattr.c)
> return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
> - function ntfs_init_acl (line 701 file fs/ntfs3/xattr.c)
> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl,
> - function ntfs_init_acl (line 712 file fs/ntfs3/xattr.c)
> err = ntfs_set_acl_ex(mnt_userns, inode, acl,
>
> So there are many entry points to ntfs_set_ea (and can be added more).
> Of course we can let int locked remain for these situations,
> but in my opinion it makes code a lot less readable
> (that was the reason to start looking into locking).
Yeah this what I was suggesting. And I totally agree that it is less
readable.
> I'm not sure, that winning some lock/unlocking
> in one specific scenario is worth it.
Still 4 locking / 4 unlocking in row is painfull. One option is to lock
outside of ntfs_set_acl if that is ok to you. If not maybe atleast add
todo comment to ntfs_set_wsl_perm that locking needs to be rethinked. I
know that there is lot of work to do with locks in here so maybe it is
not right time to nit pick about them.
>
> >>
> >>>> if (err)
> >>>> goto out;
> >>>> }
> >>>> --
> >>>> 2.33.0
> >>>>
> >>>>