[PATCH 0/4] fs/ntfs3: Various fixes for xattr and files

[PATCH 0/4] fs/ntfs3: Various fixes for xattr and files

[Konstantin Komarov]

Various problems were detected by xfstests.
This series aims to fix them.

Konstantin Komarov (4):
  fs/ntfs3: Keep preallocated only if option prealloc enabled
  fs/ntfs3: Restore ntfs_xattr_get_acl and ntfs_xattr_set_acl functions
  fs/ntfs3: Optimize locking in ntfs_save_wsl_perm
  fs/ntfs3: Update i_ctime when xattr is added

 fs/ntfs3/file.c  |   2 +-
 fs/ntfs3/xattr.c | 123 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 113 insertions(+), 12 deletions(-)

-- 
2.33.0

[PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled

[Konstantin Komarov]

If size of file was reduced, we still kept allocated blocks.
This commit makes ntfs3 work as other fs like btrfs.
https://bugzilla.kernel.org/show_bug.cgi?id=214719
Fixes: 4342306f0f0d ("fs/ntfs3: Add file operations and implementation")

Reported-by: Ganapathi Kamath
Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
index 43b1451bff53..3ac0482c6880 100644
--- a/fs/ntfs3/file.c
+++ b/fs/ntfs3/file.c
@@ -494,7 +494,7 @@ static int ntfs_truncate(struct inode *inode, loff_t new_size)
 
 	down_write(&ni->file.run_lock);
 	err = attr_set_size(ni, ATTR_DATA, NULL, 0, &ni->file.run, new_size,
-			    &new_valid, true, NULL);
+			    &new_valid, ni->mi.sbi->options->prealloc, NULL);
 	up_write(&ni->file.run_lock);
 
 	if (new_valid < ni->i_valid)
-- 
2.33.0

[PATCH 2/4] fs/ntfs3: Restore ntfs_xattr_get_acl and ntfs_xattr_set_acl functions

[Konstantin Komarov]

Apparently we need to maintain these functions with
ntfs_get_acl_ex and ntfs_set_acl_ex.
This commit fixes xfstest generic/099
Fixes: 95dd8b2c1ed0 ("fs/ntfs3: Remove unnecessary functions")

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/xattr.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 2143099cffdf..62605781790b 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -112,7 +112,7 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
 		return -ENOMEM;
 
 	if (!size) {
-		;
+		/* EA info persists, but xattr is empty. Looks like EA problem. */
 	} else if (attr_ea->non_res) {
 		struct runs_tree run;
 
@@ -616,6 +616,67 @@ int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
 }
 
+static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
+			      struct inode *inode, int type, void *buffer,
+			      size_t size)
+{
+	struct posix_acl *acl;
+	int err;
+
+	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
+		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
+		return -EOPNOTSUPP;
+	}
+
+	acl = ntfs_get_acl(inode, type, false);
+	if (IS_ERR(acl))
+		return PTR_ERR(acl);
+
+	if (!acl)
+		return -ENODATA;
+
+	err = posix_acl_to_xattr(mnt_userns, acl, buffer, size);
+	posix_acl_release(acl);
+
+	return err;
+}
+
+static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
+			      struct inode *inode, int type, const void *value,
+			      size_t size)
+{
+	struct posix_acl *acl;
+	int err;
+
+	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
+		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
+		return -EOPNOTSUPP;
+	}
+
+	if (!inode_owner_or_capable(mnt_userns, inode))
+		return -EPERM;
+
+	if (!value) {
+		acl = NULL;
+	} else {
+		acl = posix_acl_from_xattr(mnt_userns, value, size);
+		if (IS_ERR(acl))
+			return PTR_ERR(acl);
+
+		if (acl) {
+			err = posix_acl_valid(mnt_userns, acl);
+			if (err)
+				goto release_and_out;
+		}
+	}
+
+	err = ntfs_set_acl(mnt_userns, inode, acl, type);
+
+release_and_out:
+	posix_acl_release(acl);
+	return err;
+}
+
 /*
  * ntfs_init_acl - Initialize the ACLs of a new inode.
  *
@@ -782,6 +843,23 @@ static int ntfs_getxattr(const struct xattr_handler *handler, struct dentry *de,
 		goto out;
 	}
 
+#ifdef CONFIG_NTFS3_FS_POSIX_ACL
+	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
+	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
+		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
+	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
+	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
+		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
+		/* TODO: init_user_ns? */
+		err = ntfs_xattr_get_acl(
+			&init_user_ns, inode,
+			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
+				? ACL_TYPE_ACCESS
+				: ACL_TYPE_DEFAULT,
+			buffer, size);
+		goto out;
+	}
+#endif
 	/* Deal with NTFS extended attribute. */
 	err = ntfs_get_ea(inode, name, name_len, buffer, size, NULL);
 
@@ -894,6 +972,22 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
 		goto out;
 	}
 
+#ifdef CONFIG_NTFS3_FS_POSIX_ACL
+	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
+	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
+		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
+	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
+	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
+		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
+		err = ntfs_xattr_set_acl(
+			mnt_userns, inode,
+			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
+				? ACL_TYPE_ACCESS
+				: ACL_TYPE_DEFAULT,
+			value, size);
+		goto out;
+	}
+#endif
 	/* Deal with NTFS extended attribute. */
 	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
 
-- 
2.33.0

[PATCH 3/4] fs/ntfs3: Optimize locking in ntfs_save_wsl_perm

[Konstantin Komarov]

Right now in ntfs_save_wsl_perm we lock/unlock 4 times.
This commit fixes this situation.
We add "locked" argument to ntfs_set_ea.

Suggested-by: Kari Argillander <kari.argillander@gmail.com>
Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/xattr.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 62605781790b..bc3144608ce1 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -259,7 +259,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)
+				size_t val_size, int flags, bool locked)
 {
 	struct ntfs_inode *ni = ntfs_i(inode);
 	struct ntfs_sb_info *sbi = ni->mi.sbi;
@@ -278,7 +278,8 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
 	u64 new_sz;
 	void *p;
 
-	ni_lock(ni);
+	if (!locked)
+		ni_lock(ni);
 
 	run_init(&ea_run);
 
@@ -467,7 +468,8 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name,
 	mark_inode_dirty(&ni->vfs_inode);
 
 out:
-	ni_unlock(ni);
+	if (!locked)
+		ni_unlock(ni);
 
 	run_close(&ea_run);
 	kfree(ea_all);
@@ -595,7 +597,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);
+	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
 	if (err == -ENODATA && !size)
 		err = 0; /* Removing non existed xattr. */
 	if (!err)
@@ -989,7 +991,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);
+	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
 
 out:
 	return err;
@@ -1004,35 +1006,37 @@ int ntfs_save_wsl_perm(struct inode *inode)
 {
 	int err;
 	__le32 value;
+	struct ntfs_inode *ni = ntfs_i(inode);
 
-	/* TODO: refactor this, so we don't lock 4 times in ntfs_set_ea */
+	ni_lock(ni);
 	value = cpu_to_le32(i_uid_read(inode));
 	err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value,
-			  sizeof(value), 0);
+			  sizeof(value), 0, true); /* true == already locked. */
 	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);
+			  sizeof(value), 0, true);
 	if (err)
 		goto out;
 
 	value = cpu_to_le32(inode->i_mode);
 	err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value,
-			  sizeof(value), 0);
+			  sizeof(value), 0, true);
 	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);
+				  sizeof(value), 0, true);
 		if (err)
 			goto out;
 	}
 
 out:
+	ni_unlock(ni);
 	/* In case of error should we delete all WSL xattr? */
 	return err;
 }
-- 
2.33.0

[PATCH 4/4] fs/ntfs3: Update i_ctime when xattr is added

[Konstantin Komarov]

Ctime wasn't updated after setfacl command.
This commit fixes xfstest generic/307
Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")

Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
 fs/ntfs3/xattr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index bc3144608ce1..702775a559f5 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -994,6 +994,9 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
 	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
 
 out:
+	inode->i_ctime = current_time(inode);
+	mark_inode_dirty(inode);
+
 	return err;
 }
 
-- 
2.33.0

Re: [PATCH 3/4] fs/ntfs3: Optimize locking in ntfs_save_wsl_perm

[Joe Perches]

On Fri, 2021-10-22 at 18:55 +0300, Konstantin Komarov wrote:
> Right now in ntfs_save_wsl_perm we lock/unlock 4 times.
> This commit fixes this situation.
> We add "locked" argument to ntfs_set_ea.
[]
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
[]
> @@ -259,7 +259,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)
> +				size_t val_size, int flags, bool locked)
[]
> @@ -595,7 +597,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);
> +	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);

generally is nicer to use true/false for bool rather than true/0

>  	err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value,
> -			  sizeof(value), 0);
> +			  sizeof(value), 0, true);

Re: [PATCH 2/4] fs/ntfs3: Restore ntfs_xattr_get_acl and ntfs_xattr_set_acl functions

[Kari Argillander]

On Fri, Oct 22, 2021 at 06:55:09PM +0300, Konstantin Komarov wrote:
> Apparently we need to maintain these functions with
> ntfs_get_acl_ex and ntfs_set_acl_ex.
> This commit fixes xfstest generic/099
> Fixes: 95dd8b2c1ed0 ("fs/ntfs3: Remove unnecessary functions")

I get build error with patch 1&2 applied.

fs/ntfs3/xattr.c: In function ‘ntfs_xattr_get_acl’:
fs/ntfs3/xattr.c:631:8: error: too many arguments to function ‘ntfs_get_acl’
  631 |  acl = ntfs_get_acl(inode, type, false);
      |        ^~~~~~~~~~~~
fs/ntfs3/xattr.c:533:19: note: declared here
  533 | struct posix_acl *ntfs_get_acl(struct inode *inode, int type)

> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/xattr.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 2143099cffdf..62605781790b 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -112,7 +112,7 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea,
>  		return -ENOMEM;
>  
>  	if (!size) {
> -		;
> +		/* EA info persists, but xattr is empty. Looks like EA problem. */
>  	} else if (attr_ea->non_res) {
>  		struct runs_tree run;
>  
> @@ -616,6 +616,67 @@ int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>  	return ntfs_set_acl_ex(mnt_userns, inode, acl, type);
>  }
>  
> +static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> +			      struct inode *inode, int type, void *buffer,
> +			      size_t size)
> +{
> +	struct posix_acl *acl;
> +	int err;
> +
> +	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
> +		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	acl = ntfs_get_acl(inode, type, false);
> +	if (IS_ERR(acl))
> +		return PTR_ERR(acl);
> +
> +	if (!acl)
> +		return -ENODATA;
> +
> +	err = posix_acl_to_xattr(mnt_userns, acl, buffer, size);
> +	posix_acl_release(acl);
> +
> +	return err;
> +}
> +
> +static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
> +			      struct inode *inode, int type, const void *value,
> +			      size_t size)
> +{
> +	struct posix_acl *acl;
> +	int err;
> +
> +	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
> +		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!inode_owner_or_capable(mnt_userns, inode))
> +		return -EPERM;
> +
> +	if (!value) {
> +		acl = NULL;
> +	} else {
> +		acl = posix_acl_from_xattr(mnt_userns, value, size);
> +		if (IS_ERR(acl))
> +			return PTR_ERR(acl);
> +
> +		if (acl) {
> +			err = posix_acl_valid(mnt_userns, acl);
> +			if (err)
> +				goto release_and_out;
> +		}
> +	}
> +
> +	err = ntfs_set_acl(mnt_userns, inode, acl, type);
> +
> +release_and_out:
> +	posix_acl_release(acl);
> +	return err;
> +}
> +
>  /*
>   * ntfs_init_acl - Initialize the ACLs of a new inode.
>   *
> @@ -782,6 +843,23 @@ static int ntfs_getxattr(const struct xattr_handler *handler, struct dentry *de,
>  		goto out;
>  	}
>  
> +#ifdef CONFIG_NTFS3_FS_POSIX_ACL
> +	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
> +	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
> +		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
> +	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
> +	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
> +		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
> +		/* TODO: init_user_ns? */
> +		err = ntfs_xattr_get_acl(
> +			&init_user_ns, inode,
> +			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
> +				? ACL_TYPE_ACCESS
> +				: ACL_TYPE_DEFAULT,
> +			buffer, size);
> +		goto out;
> +	}
> +#endif
>  	/* Deal with NTFS extended attribute. */
>  	err = ntfs_get_ea(inode, name, name_len, buffer, size, NULL);
>  
> @@ -894,6 +972,22 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>  		goto out;
>  	}
>  
> +#ifdef CONFIG_NTFS3_FS_POSIX_ACL
> +	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
> +	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
> +		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
> +	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
> +	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
> +		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
> +		err = ntfs_xattr_set_acl(
> +			mnt_userns, inode,
> +			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
> +				? ACL_TYPE_ACCESS
> +				: ACL_TYPE_DEFAULT,
> +			value, size);
> +		goto out;
> +	}
> +#endif
>  	/* Deal with NTFS extended attribute. */
>  	err = ntfs_set_ea(inode, name, name_len, value, size, flags);
>  
> -- 
> 2.33.0
> 
> 

Re: [PATCH 4/4] fs/ntfs3: Update i_ctime when xattr is added

[Kari Argillander]

On Fri, Oct 22, 2021 at 06:56:07PM +0300, Konstantin Komarov wrote:
> Ctime wasn't updated after setfacl command.
> This commit fixes xfstest generic/307
> Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")

Because this is fix I would suggest you make this patch 3/4. Usually
fixes should be first so it is easier for stable team to pick them with
confident.

> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/xattr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index bc3144608ce1..702775a559f5 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -994,6 +994,9 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>  	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
>  
>  out:
> +	inode->i_ctime = current_time(inode);
> +	mark_inode_dirty(inode);
> +
>  	return err;
>  }
>  
> -- 
> 2.33.0
> 
> 
> 

Re: [PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled

[Kari Argillander]

On Fri, Oct 22, 2021 at 06:54:31PM +0300, Konstantin Komarov wrote:
> If size of file was reduced, we still kept allocated blocks.
> This commit makes ntfs3 work as other fs like btrfs.
> https://bugzilla.kernel.org/show_bug.cgi?id=214719

Link: https://bugzilla.kernel.org/show_bug.cgi?id=214719
> Fixes: 4342306f0f0d ("fs/ntfs3: Add file operations and implementation")
> 
> Reported-by: Ganapathi Kamath

Add <hgkamath@hotmail.com>

I also added to loop here. Ganapathi can you test if this patch fix your
problem?

> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> index 43b1451bff53..3ac0482c6880 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -494,7 +494,7 @@ static int ntfs_truncate(struct inode *inode, loff_t new_size)
>  
>  	down_write(&ni->file.run_lock);
>  	err = attr_set_size(ni, ATTR_DATA, NULL, 0, &ni->file.run, new_size,
> -			    &new_valid, true, NULL);
> +			    &new_valid, ni->mi.sbi->options->prealloc, NULL);
>  	up_write(&ni->file.run_lock);
>  
>  	if (new_valid < ni->i_valid)
> -- 
> 2.33.0
> 
> 

Re: [PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled

[Ganapathi Kamath]

Hellom 

While compiling, first time around, I got the below.

fs/ntfs3/file.c: In function 'ntfs_truncate': 
fs/ntfs3/file.c:498:60: error: invalid type argument of '->' (have 'struct ntfs_mount_options')
  498 |                             &new_valid, ni->mi.sbi->options->prealloc, NULL);
      |                                                            ^~
make[2]: *** [scripts/Makefile.build:277: fs/ntfs3/file.o] Error 1
make[1]: *** [scripts/Makefile.build:540: fs/ntfs3] Error 2

So, in the file
    fs/ntfs3/file.c
I changed 
    ni->mi.sbi->options->prealloc
to
    ni->mi.sbi->options.prealloc

I don't really follow/understand the code, to understand what exactly the logic is, except that you are trying to set 
boolean 'keep_prealloc' call-argument for attr_set_size() by using the ntfs_mount_option bit-field 'prealloc' which is to "Preallocate space when file is growing", prototyped in the file fs/ntfs3/ntfs_fs.h

* Built new 5.15.0-0.rc3.20211001git4de593fb965f.30.fc35.x86_64 kernel. (4 hrs on my machine)
* I was able to include patch into rpmbuild of kernel src patch, with aforementioned correction
* first reconfirmed/verified bug on old kernel
* installed newly built kernel
* attempt reproduction no success meaning bug not present on new kernel, patch/fix makes file size on overwrite to be as expected.

note, I am not an expert, and as a user, I don't know 100% what correct behavior should be, only what seems reasonable expected behavior, But  you are experts, so please excuse me for reiterating what you know. NTFS is a filesystem that was designed by microsoft for windows, and the way its fs-driver must update is so that on-disk structures is suitable for windows in-kernel structures. A kernel driver for linux, only adapts on disk-ntfs structures to something suitable for linux in-kernel structures, but must update the on disk structures the way Windows expects/designed it to.
So if you defended old behavior, I wouldn't know.
So its your call, to decide if it is a bug, and whether your patch fixes. 
On my side, my machine is one I work on. patch seems to fix claimed bug. So I hope there is no side effect, nothing corrupts or becomes unstable. 

That was fast fix. congrats. 

Log:
[root@sirius gana]#
[root@sirius gana]# mount -t ntfs3 /dev/sda17 /mnt/a17/
[root@sirius gana]#
[root@sirius gana]# rm -f /mnt/a17/test1.bin /mnt/a17/test2.bin
[root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test2.bin bs=1M count=3000
3000+0 records in
3000+0 records out
3145728000 bytes (3.1 GB, 2.9 GiB) copied, 5.40015 s, 583 MB/s
[root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test1.bin bs=1M count=6000
6000+0 records in
6000+0 records out
6291456000 bytes (6.3 GB, 5.9 GiB) copied, 16.1809 s, 389 MB/s
[root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
6144000 -rw-r--r--. 1 root root 6291456000 Oct 24 13:42 /mnt/a17/test1.bin
3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
[root@sirius gana]# cp /mnt/a17/test2.bin /mnt/a17/test1.bin
cp: overwrite '/mnt/a17/test1.bin'? y
[root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:42 /mnt/a17/test1.bin
3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
[root@sirius gana]#  stat /mnt/a17/test1.bin
  File: /mnt/a17/test1.bin
  Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
Device: 10301h/66305d   Inode: 44          Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Context: system_u:object_r:unlabeled_t:s0
Access: 2021-10-24 13:41:59.265503300 +0530
Modify: 2021-10-24 13:42:44.738904000 +0530
Change: 2021-10-24 13:42:44.738904000 +0530
 Birth: 2021-10-24 13:41:59.265503300 +0530
[root@sirius gana]#  stat /mnt/a17/test2.bin
  File: /mnt/a17/test2.bin
  Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
Device: 10301h/66305d   Inode: 43          Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Context: system_u:object_r:unlabeled_t:s0
Access: 2021-10-24 13:42:40.610776900 +0530
Modify: 2021-10-24 13:41:52.684315600 +0530
Change: 2021-10-24 13:41:52.684315600 +0530
 Birth: 2021-10-24 13:41:47.284266100 +0530



From: Kari Argillander <kari.argillander@gmail.com>
Sent: Saturday, October 23, 2021 5:55 AM
To: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>; hgkamath@hotmail.com <hgkamath@hotmail.com>
Cc: ntfs3@lists.linux.dev <ntfs3@lists.linux.dev>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-fsdevel@vger.kernel.org <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled 
 
On Fri, Oct 22, 2021 at 06:54:31PM +0300, Konstantin Komarov wrote:
> If size of file was reduced, we still kept allocated blocks.
> This commit makes ntfs3 work as other fs like btrfs.
> https://bugzilla.kernel.org/show_bug.cgi?id=214719

Link: https://bugzilla.kernel.org/show_bug.cgi?id=214719
> Fixes: 4342306f0f0d ("fs/ntfs3: Add file operations and implementation")
> 
> Reported-by: Ganapathi Kamath

Add <hgkamath@hotmail.com>

I also added to loop here. Ganapathi can you test if this patch fix your
problem?

> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> index 43b1451bff53..3ac0482c6880 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -494,7 +494,7 @@ static int ntfs_truncate(struct inode *inode, loff_t new_size)
>  
>        down_write(&ni->file.run_lock);
>        err = attr_set_size(ni, ATTR_DATA, NULL, 0, &ni->file.run, new_size,
> -                         &new_valid, true, NULL);
> +                         &new_valid, ni->mi.sbi->options->prealloc, NULL);
>        up_write(&ni->file.run_lock);
>  
>        if (new_valid < ni->i_valid)
> -- 
> 2.33.0
> 
> 

Re: [PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled

[Kari Argillander]

On Sun, Oct 24, 2021 at 08:35:57AM +0000, Ganapathi Kamath wrote:
> Hellom 
> 
> While compiling, first time around, I got the below.

Yeah. Patch was meant for ntfs3/master...

> fs/ntfs3/file.c: In function 'ntfs_truncate': 
> fs/ntfs3/file.c:498:60: error: invalid type argument of '->' (have 'struct ntfs_mount_options')
>   498 |                             &new_valid, ni->mi.sbi->options->prealloc, NULL);
>       |                                                            ^~
> make[2]: *** [scripts/Makefile.build:277: fs/ntfs3/file.o] Error 1
> make[1]: *** [scripts/Makefile.build:540: fs/ntfs3] Error 2
> 
> So, in the file
>     fs/ntfs3/file.c
> I changed 
>     ni->mi.sbi->options->prealloc
> to
>     ni->mi.sbi->options.prealloc

but with your change it also applies to rc3. Your change was correct and
it was totally ok to test top of rc3.

> I don't really follow/understand the code, to understand what exactly
> the logic is, except that you are trying to set boolean
> 'keep_prealloc' call-argument for attr_set_size() by using the
> ntfs_mount_option bit-field 'prealloc' which is to "Preallocate space
> when file is growing", prototyped in the file fs/ntfs3/ntfs_fs.h
> 
> * Built new 5.15.0-0.rc3.20211001git4de593fb965f.30.fc35.x86_64
> kernel. (4 hrs on my machine)

I do not know if you used -j flag when making. Just if you did not know
about it use:

  make -j8

This will example use 8 threads for compiling. You choose number based
on how many threads you have in your processor.

> * I was able to include patch into rpmbuild of kernel src patch, with
> aforementioned correction * first reconfirmed/verified bug on old
> kernel
> * installed newly built kernel
> * attempt reproduction no success meaning bug not present on new
> kernel, patch/fix makes file size on overwrite to be as expected.
> 
> note, I am not an expert, and as a user, I don't know 100% what
> correct behavior should be, only what seems reasonable expected
> behavior, But  you are experts, so please excuse me for reiterating
> what you know. NTFS is a filesystem that was designed by microsoft for
> windows, and the way its fs-driver must update is so that on-disk
> structures is suitable for windows in-kernel structures. A kernel
> driver for linux, only adapts on disk-ntfs structures to something
> suitable for linux in-kernel structures, but must update the on disk
> structures the way Windows expects/designed it to.

Ntfs file structure allows actually many things which even Windows does
not understand. This is new driver and it will be evolving that user can
decide what he wants. We will also have to define good defaults so that
user get good experience.

> So if you defended old behavior, I wouldn't know.  So its your call,
> to decide if it is a bug, and whether your patch fixes.  On my side,
> my machine is one I work on. patch seems to fix claimed bug. So I hope
> there is no side effect, nothing corrupts or becomes unstable. 

This was bug. Thanks for reporting and testing it.  We really appriciate
it. This is new driver and there will be bugs so early users who report
bugs and are even willing to test patches are gold mine for us.

We will also add tags to patch:
Reported-by: Ganapathi Kamath <hgkamath@hotmail.com>
Tested-by: Ganapathi Kamath <hgkamath@hotmail.com>

if it ok to you. This way if you report new bugs to kernel people will
know you are good reporter as you have also tested what you have
reported. If you wanna know more about these tags see [1].

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

  Argillander

> That was fast fix. congrats. 
> 
> Log:
> [root@sirius gana]#
> [root@sirius gana]# mount -t ntfs3 /dev/sda17 /mnt/a17/
> [root@sirius gana]#
> [root@sirius gana]# rm -f /mnt/a17/test1.bin /mnt/a17/test2.bin
> [root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test2.bin bs=1M count=3000
> 3000+0 records in
> 3000+0 records out
> 3145728000 bytes (3.1 GB, 2.9 GiB) copied, 5.40015 s, 583 MB/s
> [root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test1.bin bs=1M count=6000
> 6000+0 records in
> 6000+0 records out
> 6291456000 bytes (6.3 GB, 5.9 GiB) copied, 16.1809 s, 389 MB/s
> [root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
> 6144000 -rw-r--r--. 1 root root 6291456000 Oct 24 13:42 /mnt/a17/test1.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
> [root@sirius gana]# cp /mnt/a17/test2.bin /mnt/a17/test1.bin
> cp: overwrite '/mnt/a17/test1.bin'? y
> [root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:42 /mnt/a17/test1.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
> [root@sirius gana]#  stat /mnt/a17/test1.bin
>   File: /mnt/a17/test1.bin
>   Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
> Device: 10301h/66305d   Inode: 44          Links: 1
> Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2021-10-24 13:41:59.265503300 +0530
> Modify: 2021-10-24 13:42:44.738904000 +0530
> Change: 2021-10-24 13:42:44.738904000 +0530
>  Birth: 2021-10-24 13:41:59.265503300 +0530
> [root@sirius gana]#  stat /mnt/a17/test2.bin
>   File: /mnt/a17/test2.bin
>   Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
> Device: 10301h/66305d   Inode: 43          Links: 1
> Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2021-10-24 13:42:40.610776900 +0530
> Modify: 2021-10-24 13:41:52.684315600 +0530
> Change: 2021-10-24 13:41:52.684315600 +0530
>  Birth: 2021-10-24 13:41:47.284266100 +0530

Re: [PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled

[Ganapathi Kamath]

Sure, okay to tag, no problem, happy to have helped. -Gana


From: Kari Argillander <kari.argillander@gmail.com>
Sent: Sunday, October 24, 2021 6:43 AM
To: Ganapathi Kamath <hgkamath@hotmail.com>
Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>; ntfs3@lists.linux.dev <ntfs3@lists.linux.dev>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-fsdevel@vger.kernel.org <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH 1/4] fs/ntfs3: Keep preallocated only if option prealloc enabled 
 
On Sun, Oct 24, 2021 at 08:35:57AM +0000, Ganapathi Kamath wrote:
> Hellom 
> 
> While compiling, first time around, I got the below.

Yeah. Patch was meant for ntfs3/master...

> fs/ntfs3/file.c: In function 'ntfs_truncate': 
> fs/ntfs3/file.c:498:60: error: invalid type argument of '->' (have 'struct ntfs_mount_options')
>   498 |                             &new_valid, ni->mi.sbi->options->prealloc, NULL);
>       |                                                            ^~
> make[2]: *** [scripts/Makefile.build:277: fs/ntfs3/file.o] Error 1
> make[1]: *** [scripts/Makefile.build:540: fs/ntfs3] Error 2
> 
> So, in the file
>     fs/ntfs3/file.c
> I changed 
>     ni->mi.sbi->options->prealloc
> to
>     ni->mi.sbi->options.prealloc

but with your change it also applies to rc3. Your change was correct and
it was totally ok to test top of rc3.

> I don't really follow/understand the code, to understand what exactly
> the logic is, except that you are trying to set boolean
> 'keep_prealloc' call-argument for attr_set_size() by using the
> ntfs_mount_option bit-field 'prealloc' which is to "Preallocate space
> when file is growing", prototyped in the file fs/ntfs3/ntfs_fs.h
> 
> * Built new 5.15.0-0.rc3.20211001git4de593fb965f.30.fc35.x86_64
> kernel. (4 hrs on my machine)

I do not know if you used -j flag when making. Just if you did not know
about it use:

  make -j8

This will example use 8 threads for compiling. You choose number based
on how many threads you have in your processor.

> * I was able to include patch into rpmbuild of kernel src patch, with
> aforementioned correction * first reconfirmed/verified bug on old
> kernel
> * installed newly built kernel
> * attempt reproduction no success meaning bug not present on new
> kernel, patch/fix makes file size on overwrite to be as expected.
> 
> note, I am not an expert, and as a user, I don't know 100% what
> correct behavior should be, only what seems reasonable expected
> behavior, But  you are experts, so please excuse me for reiterating
> what you know. NTFS is a filesystem that was designed by microsoft for
> windows, and the way its fs-driver must update is so that on-disk
> structures is suitable for windows in-kernel structures. A kernel
> driver for linux, only adapts on disk-ntfs structures to something
> suitable for linux in-kernel structures, but must update the on disk
> structures the way Windows expects/designed it to.

Ntfs file structure allows actually many things which even Windows does
not understand. This is new driver and it will be evolving that user can
decide what he wants. We will also have to define good defaults so that
user get good experience.

> So if you defended old behavior, I wouldn't know.  So its your call,
> to decide if it is a bug, and whether your patch fixes.  On my side,
> my machine is one I work on. patch seems to fix claimed bug. So I hope
> there is no side effect, nothing corrupts or becomes unstable. 

This was bug. Thanks for reporting and testing it.  We really appriciate
it. This is new driver and there will be bugs so early users who report
bugs and are even willing to test patches are gold mine for us.

We will also add tags to patch:
Reported-by: Ganapathi Kamath <hgkamath@hotmail.com>
Tested-by: Ganapathi Kamath <hgkamath@hotmail.com>

if it ok to you. This way if you report new bugs to kernel people will
know you are good reporter as you have also tested what you have
reported. If you wanna know more about these tags see [1].

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

  Argillander

> That was fast fix. congrats. 
> 
> Log:
> [root@sirius gana]#
> [root@sirius gana]# mount -t ntfs3 /dev/sda17 /mnt/a17/
> [root@sirius gana]#
> [root@sirius gana]# rm -f /mnt/a17/test1.bin /mnt/a17/test2.bin
> [root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test2.bin bs=1M count=3000
> 3000+0 records in
> 3000+0 records out
> 3145728000 bytes (3.1 GB, 2.9 GiB) copied, 5.40015 s, 583 MB/s
> [root@sirius gana]# dd if=/dev/zero of=/mnt/a17/test1.bin bs=1M count=6000
> 6000+0 records in
> 6000+0 records out
> 6291456000 bytes (6.3 GB, 5.9 GiB) copied, 16.1809 s, 389 MB/s
> [root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
> 6144000 -rw-r--r--. 1 root root 6291456000 Oct 24 13:42 /mnt/a17/test1.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
> [root@sirius gana]# cp /mnt/a17/test2.bin /mnt/a17/test1.bin
> cp: overwrite '/mnt/a17/test1.bin'? y
> [root@sirius gana]# ls -ls /mnt/a17/test1.bin /mnt/a17/test2.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:42 /mnt/a17/test1.bin
> 3072000 -rw-r--r--. 1 root root 3145728000 Oct 24 13:41 /mnt/a17/test2.bin
> [root@sirius gana]#  stat /mnt/a17/test1.bin
>   File: /mnt/a17/test1.bin
>   Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
> Device: 10301h/66305d   Inode: 44          Links: 1
> Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2021-10-24 13:41:59.265503300 +0530
> Modify: 2021-10-24 13:42:44.738904000 +0530
> Change: 2021-10-24 13:42:44.738904000 +0530
>  Birth: 2021-10-24 13:41:59.265503300 +0530
> [root@sirius gana]#  stat /mnt/a17/test2.bin
>   File: /mnt/a17/test2.bin
>   Size: 3145728000      Blocks: 6144000    IO Block: 4096   regular file
> Device: 10301h/66305d   Inode: 43          Links: 1
> Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> Context: system_u:object_r:unlabeled_t:s0
> Access: 2021-10-24 13:42:40.610776900 +0530
> Modify: 2021-10-24 13:41:52.684315600 +0530
> Change: 2021-10-24 13:41:52.684315600 +0530
>  Birth: 2021-10-24 13:41:47.284266100 +0530