[PATCH 0/4] property fixes and cleanups

[PATCH 0/4] property fixes and cleanups

[Anand Jain]

Both mainline and misc-5.2 fails the test case that transaction 
generation number must be unaltered if the property validation fails.

For example:
btrfs in dump-super /dev/sdb | grep ^generation; \
btrfs prop set /btrfs compression lz; sync; \
btrfs in dump-super /dev/sdb | grep ^generation
generation		53
ERROR: failed to set compression for /btrfs: Invalid argument
generation		54

Which the patch in the mailing list fixes and based on misc-5.2
  [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment

But, now I notice that the reason for which the original code failed
is different from the reason for which misc-5.2 failed. Former failed
because the malformed 'lz' was only abled to be caught by handle->apply(),
and in latter we were calling the start_transaction() too early before the
handle->validation() which few of the patches in misc-5.1 did. My bad.

As per your misc-5.2 branch please drop the following patches as I have
better fixes as in this patch.

584aebf26bbf btrfs: merge btrfs_setxattr and do_setxattr
ab54ee38fff7 btrfs: don't create transaction in btrfs_setxattr
0cbf560f72e1 btrfs: start transaction in btrfs_xattr_handler_set
db0f220e98eb btrfs: start transaction in btrfs_set_acl
56e9e992c00a btrfs: start transaction in btrfs_set_prop_trans
3e2299ea6a8b btrfs: refactor btrfs_set_prop and add btrfs_set_prop_trans
737e67ce0f3f btrfs: merge _btrfs_set_prop helpers

And please apply the patches as in this set.

Anand Jain (4):
  btrfs: rename __btrfs_set_prop to btrfs_set_prop_trans
  btrfs: rename __btrfs_set_acl to do_set_acl
  btrfs: fix zstd compression parameter
  btrfs: fix vanished compression property after failed set

 fs/btrfs/acl.c   | 12 +++++-------
 fs/btrfs/props.c | 23 ++++++++++-------------
 2 files changed, 15 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH 1/4] btrfs: rename __btrfs_set_prop to btrfs_set_prop_trans

[Anand Jain]

__btrfs_set_prop() accepts trans an argument, which when NULL,
the trans is created further down at btrfs_setxattr().
So btrfs_set_prop() is a redirect to __btrfs_set_prop() with the
transaction handle equal to NULL. No functional changes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/props.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 7466ffd33ba6..0939554ffa63 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -55,12 +55,9 @@ find_prop_handler(const char *name,
 	return NULL;
 }
 
-static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
-			    struct inode *inode,
-			    const char *name,
-			    const char *value,
-			    size_t value_len,
-			    int flags)
+static int btrfs_set_prop_trans(struct btrfs_trans_handle *trans,
+				struct inode *inode, const char *name,
+				const char *value, size_t value_len, int flags)
 {
 	const struct prop_handler *handler;
 	int ret;
@@ -109,7 +106,7 @@ int btrfs_set_prop(struct inode *inode,
 		   size_t value_len,
 		   int flags)
 {
-	return __btrfs_set_prop(NULL, inode, name, value, value_len, flags);
+	return btrfs_set_prop_trans(NULL, inode, name, value, value_len, flags);
 }
 
 static int iterate_object_props(struct btrfs_root *root,
@@ -355,8 +352,8 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
 		if (ret)
 			goto out;
-		ret = __btrfs_set_prop(trans, inode, h->xattr_name,
-				       value, strlen(value), 0);
+		ret = btrfs_set_prop_trans(trans, inode, h->xattr_name, value,
+					   strlen(value), 0);
 		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);
 		if (ret)
 			goto out;
-- 
2.17.1

[PATCH 2/4] btrfs: rename __btrfs_set_acl to do_set_acl

[Anand Jain]

__btrfs_set_acl() is helper function of btrfs_set_acl(), rename it to
do_set_acl(). No functional changes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/acl.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 5810463dc6d2..a22ebb04d9ca 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -52,8 +52,8 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
-static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
-			 struct inode *inode, struct posix_acl *acl, int type)
+static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
+		      struct posix_acl *acl, int type)
 {
 	int ret, size = 0;
 	const char *name;
@@ -113,7 +113,7 @@ int btrfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		if (ret)
 			return ret;
 	}
-	ret = __btrfs_set_acl(NULL, inode, acl, type);
+	ret = do_set_acl(NULL, inode, acl, type);
 	if (ret)
 		inode->i_mode = old_mode;
 	return ret;
@@ -134,15 +134,13 @@ int btrfs_init_acl(struct btrfs_trans_handle *trans,
 		return ret;
 
 	if (default_acl) {
-		ret = __btrfs_set_acl(trans, inode, default_acl,
-				      ACL_TYPE_DEFAULT);
+		ret = do_set_acl(trans, inode, default_acl, ACL_TYPE_DEFAULT);
 		posix_acl_release(default_acl);
 	}
 
 	if (acl) {
 		if (!ret)
-			ret = __btrfs_set_acl(trans, inode, acl,
-					      ACL_TYPE_ACCESS);
+			ret = do_set_acl(trans, inode, acl, ACL_TYPE_ACCESS);
 		posix_acl_release(acl);
 	}
 
-- 
2.17.1

[PATCH 3/4] btrfs: fix zstd compression parameter

[Anand Jain]

We let to pass zstd compression parameter even if its not fully written.
For example:
  btrfs prop set /btrfs compression zst
  btrfs prop get /btrfs compression
     compression=zst

zlib and lzo are fine.

Fix it by using the expected number of char in strncmp().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/props.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 0939554ffa63..b324a5fd7864 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -284,7 +284,7 @@ static int prop_compression_apply(struct inode *inode, const char *value,
 		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
 	} else if (!strncmp("zlib", value, 4)) {
 		type = BTRFS_COMPRESS_ZLIB;
-	} else if (!strncmp("zstd", value, len)) {
+	} else if (!strncmp("zstd", value, 4)) {
 		type = BTRFS_COMPRESS_ZSTD;
 		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
 	} else {
-- 
2.17.1

[PATCH 4/4] btrfs: fix vanished compression property after failed set

[Anand Jain]

The compression property resets to NULL, instead of the old value if we
fail to set the new compression parameter.

btrfs prop get /btrfs compression
  compression=lzo
btrfs prop set /btrfs compression zli
  ERROR: failed to set compression for /btrfs: Invalid argument
btrfs prop get /btrfs compression

This is because the compression property ->validate() is successful for
'zli' as the strncmp() used the len passed from the userland.

Fix it by using the expected string length in strncmp().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
---
Please note:
This patch also fixes the bug that generation number gets updated when
property validation fails. As the reason its reason is weak validate(),
which lets the bad set pass untill ->apply().

 fs/btrfs/props.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index b324a5fd7864..64b5f4695d4c 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -255,11 +255,11 @@ static int prop_compression_validate(const char *value, size_t len)
 	if (!value)
 		return 0;
 
-	if (!strncmp("lzo", value, len))
+	if (!strncmp("lzo", value, 3))
 		return 0;
-	else if (!strncmp("zlib", value, len))
+	else if (!strncmp("zlib", value, 4))
 		return 0;
-	else if (!strncmp("zstd", value, len))
+	else if (!strncmp("zstd", value, 4))
 		return 0;
 
 	return -EINVAL;
-- 
2.17.1

Re: [PATCH 0/4] property fixes and cleanups

[Anand Jain]

On 4/4/19 12:51 AM, Anand Jain wrote:
> Both mainline and misc-5.2 fails the test case that transaction
> generation number must be unaltered if the property validation fails.
> 
> For example:
> btrfs in dump-super /dev/sdb | grep ^generation; \
> btrfs prop set /btrfs compression lz; sync; \
> btrfs in dump-super /dev/sdb | grep ^generation
> generation		53
> ERROR: failed to set compression for /btrfs: Invalid argument
> generation		54
> 
> Which the patch in the mailing list fixes and based on misc-5.2
>    [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment
> 
> But, now I notice that the reason for which the original code failed
> is different from the reason for which misc-5.2 failed. Former failed
> because the malformed 'lz' was only abled to be caught by handle->apply(),
> and in latter we were calling the start_transaction() too early before the
> handle->validation() which few of the patches in misc-5.1 did. My bad.
> 
> As per your misc-5.2 branch please drop the following patches as I have
> better fixes as in this patch.
> 
> 584aebf26bbf btrfs: merge btrfs_setxattr and do_setxattr
> ab54ee38fff7 btrfs: don't create transaction in btrfs_setxattr
> 0cbf560f72e1 btrfs: start transaction in btrfs_xattr_handler_set
> db0f220e98eb btrfs: start transaction in btrfs_set_acl
> 56e9e992c00a btrfs: start transaction in btrfs_set_prop_trans
> 3e2299ea6a8b btrfs: refactor btrfs_set_prop and add btrfs_set_prop_trans
> 737e67ce0f3f btrfs: merge _btrfs_set_prop helpers
> 
> And please apply the patches as in this set.
> 
> Anand Jain (4):
>    btrfs: rename __btrfs_set_prop to btrfs_set_prop_trans
>    btrfs: rename __btrfs_set_acl to do_set_acl
>    btrfs: fix zstd compression parameter
>    btrfs: fix vanished compression property after failed set
> 
>   fs/btrfs/acl.c   | 12 +++++-------
>   fs/btrfs/props.c | 23 ++++++++++-------------
>   2 files changed, 15 insertions(+), 20 deletions(-)
> 

Ping?

Re: [PATCH 0/4] property fixes and cleanups

[David Sterba]

On Thu, Apr 04, 2019 at 12:51:30AM +0800, Anand Jain wrote:
> Both mainline and misc-5.2 fails the test case that transaction 
> generation number must be unaltered if the property validation fails.
> 
> For example:
> btrfs in dump-super /dev/sdb | grep ^generation; \
> btrfs prop set /btrfs compression lz; sync; \
> btrfs in dump-super /dev/sdb | grep ^generation
> generation		53
> ERROR: failed to set compression for /btrfs: Invalid argument
> generation		54
> 
> Which the patch in the mailing list fixes and based on misc-5.2
>   [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment
> 
> But, now I notice that the reason for which the original code failed
> is different from the reason for which misc-5.2 failed. Former failed
> because the malformed 'lz' was only abled to be caught by handle->apply(),
> and in latter we were calling the start_transaction() too early before the
> handle->validation() which few of the patches in misc-5.1 did. My bad.
> 
> As per your misc-5.2 branch please drop the following patches as I have
> better fixes as in this patch.
> 
> 584aebf26bbf btrfs: merge btrfs_setxattr and do_setxattr
> ab54ee38fff7 btrfs: don't create transaction in btrfs_setxattr
> 0cbf560f72e1 btrfs: start transaction in btrfs_xattr_handler_set
> db0f220e98eb btrfs: start transaction in btrfs_set_acl
> 56e9e992c00a btrfs: start transaction in btrfs_set_prop_trans
> 3e2299ea6a8b btrfs: refactor btrfs_set_prop and add btrfs_set_prop_trans
> 737e67ce0f3f btrfs: merge _btrfs_set_prop helpers
> 
> And please apply the patches as in this set.
> 
> Anand Jain (4):
>   btrfs: rename __btrfs_set_prop to btrfs_set_prop_trans
>   btrfs: rename __btrfs_set_acl to do_set_acl
>   btrfs: fix zstd compression parameter
>   btrfs: fix vanished compression property after failed set

I'm going to send the two fixes independently to 5.1 as they affect
current code. Regarding the increased transaction, I don't want to drop
the patches that move transaction around just yet, as they're 100
patches deep in the devel queue.

The validation should happen before the transaction, the code currently
does a few nested calls so this might need further cleanups but I'd
rather go that way than to deleting what's in misc-next and starting
again.

If deleting the current misc-next patches appears to be a better option
in the end, then I'll do that.

Re: [PATCH 0/4] property fixes and cleanups

[David Sterba]

On Mon, Apr 08, 2019 at 07:02:14PM +0200, David Sterba wrote:
> On Thu, Apr 04, 2019 at 12:51:30AM +0800, Anand Jain wrote:
> > Both mainline and misc-5.2 fails the test case that transaction 
> > generation number must be unaltered if the property validation fails.
> > 
> > For example:
> > btrfs in dump-super /dev/sdb | grep ^generation; \
> > btrfs prop set /btrfs compression lz; sync; \
> > btrfs in dump-super /dev/sdb | grep ^generation
> > generation		53
> > ERROR: failed to set compression for /btrfs: Invalid argument
> > generation		54
> > 
> > Which the patch in the mailing list fixes and based on misc-5.2
> >   [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment
> > 
> > But, now I notice that the reason for which the original code failed
> > is different from the reason for which misc-5.2 failed. Former failed
> > because the malformed 'lz' was only abled to be caught by handle->apply(),
> > and in latter we were calling the start_transaction() too early before the
> > handle->validation() which few of the patches in misc-5.1 did. My bad.
> > 
> > As per your misc-5.2 branch please drop the following patches as I have
> > better fixes as in this patch.
> > 
> > 584aebf26bbf btrfs: merge btrfs_setxattr and do_setxattr
> > ab54ee38fff7 btrfs: don't create transaction in btrfs_setxattr
> > 0cbf560f72e1 btrfs: start transaction in btrfs_xattr_handler_set
> > db0f220e98eb btrfs: start transaction in btrfs_set_acl
> > 56e9e992c00a btrfs: start transaction in btrfs_set_prop_trans
> > 3e2299ea6a8b btrfs: refactor btrfs_set_prop and add btrfs_set_prop_trans
> > 737e67ce0f3f btrfs: merge _btrfs_set_prop helpers
> > 
> > And please apply the patches as in this set.
> > 
> > Anand Jain (4):
> >   btrfs: rename __btrfs_set_prop to btrfs_set_prop_trans
> >   btrfs: rename __btrfs_set_acl to do_set_acl
> >   btrfs: fix zstd compression parameter
> >   btrfs: fix vanished compression property after failed set
> 
> I'm going to send the two fixes independently to 5.1 as they affect
> current code. Regarding the increased transaction, I don't want to drop
> the patches that move transaction around just yet, as they're 100
> patches deep in the devel queue.

I've checked how much conflicts it creates, just one so that's not a
problem with removing.

> The validation should happen before the transaction, the code currently
> does a few nested calls so this might need further cleanups but I'd
> rather go that way than to deleting what's in misc-next and starting
> again.
> 
> If deleting the current misc-next patches appears to be a better option
> in the end, then I'll do that.

So, the removed patches are:

- btrfs: merge btrfs_setxattr and do_setxattr
- btrfs: don't create transaction in btrfs_setxattr
- btrfs: start transaction in btrfs_xattr_handler_set
- btrfs: start transaction in btrfs_set_acl
- btrfs: start transaction in btrfs_set_prop_trans

There was one conflict with 'btrfs: Defer setting new inode mode until
after do_set_acl succeeds' that I removed as the patch depended on the
transaction in the function.

The inode flags that have a corresponding property, handled in
btrfs_ioctl_setflags, need to be enclosed in the same transaction so
they don't get out of sync. This will require splitting the validation
and actual change, which will require restructuring the setflags
function too.

Re: [PATCH 0/4] property fixes and cleanups

[Anand Jain]

On 9/4/19 7:23 AM, David Sterba wrote:
> On Mon, Apr 08, 2019 at 07:02:14PM +0200, David Sterba wrote:
>> On Thu, Apr 04, 2019 at 12:51:30AM +0800, Anand Jain wrote:
>>> Both mainline and misc-5.2 fails the test case that transaction
>>> generation number must be unaltered if the property validation fails.
>>>
>>> For example:
>>> btrfs in dump-super /dev/sdb | grep ^generation; \
>>> btrfs prop set /btrfs compression lz; sync; \
>>> btrfs in dump-super /dev/sdb | grep ^generation
>>> generation		53
>>> ERROR: failed to set compression for /btrfs: Invalid argument
>>> generation		54
>>>
>>> Which the patch in the mailing list fixes and based on misc-5.2
>>>    [PATCH 4/4 RESEND] btrfs: fix property validate fail should not increment
>>>
>>> But, now I notice that the reason for which the original code failed
>>> is different from the reason for which misc-5.2 failed. Former failed
>>> because the malformed 'lz' was only abled to be caught by handle->apply(),
>>> and in latter we were calling the start_transaction() too early before the
>>> handle->validation() which few of the patches in misc-5.1 did. My bad.
>>>
>>> As per your misc-5.2 branch please drop the following patches as I have
>>> better fixes as in this patch.
>>>
>>> 584aebf26bbf btrfs: merge btrfs_setxattr and do_setxattr
>>> ab54ee38fff7 btrfs: don't create transaction in btrfs_setxattr
>>> 0cbf560f72e1 btrfs: start transaction in btrfs_xattr_handler_set
>>> db0f220e98eb btrfs: start transaction in btrfs_set_acl
>>> 56e9e992c00a btrfs: start transaction in btrfs_set_prop_trans
>>> 3e2299ea6a8b btrfs: refactor btrfs_set_prop and add btrfs_set_prop_trans
>>> 737e67ce0f3f btrfs: merge _btrfs_set_prop helpers
>>>
>>> And please apply the patches as in this set.
>>>
>>> Anand Jain (4):
>>>    btrfs: rename __btrfs_set_prop to btrfs_set_prop_trans
>>>    btrfs: rename __btrfs_set_acl to do_set_acl
>>>    btrfs: fix zstd compression parameter
>>>    btrfs: fix vanished compression property after failed set
>>
>> I'm going to send the two fixes independently to 5.1 as they affect
>> current code. Regarding the increased transaction, I don't want to drop
>> the patches that move transaction around just yet, as they're 100
>> patches deep in the devel queue.
> 
> I've checked how much conflicts it creates, just one so that's not a
> problem with removing.
> 
>> The validation should happen before the transaction, the code currently
>> does a few nested calls so this might need further cleanups but I'd
>> rather go that way than to deleting what's in misc-next and starting
>> again.



>> If deleting the current misc-next patches appears to be a better option
>> in the end, then I'll do that.

> So, the removed patches are:


[1]
> - btrfs: merge btrfs_setxattr and do_setxattr
> - btrfs: don't create transaction in btrfs_setxattr
> - btrfs: start transaction in btrfs_xattr_handler_set
> - btrfs: start transaction in btrfs_set_acl
> - btrfs: start transaction in btrfs_set_prop_trans
> 
> There was one conflict with 'btrfs: Defer setting new inode mode until
> after do_set_acl succeeds' that I removed as the patch depended on the
> transaction in the function.
> 
> The inode flags that have a corresponding property, handled in
> btrfs_ioctl_setflags, need to be enclosed in the same transaction so
> they don't get out of sync. This will require splitting the validation
> and actual change, which will require restructuring the setflags
> function too.
> 

  You are right. So moving the start_transaction upward was right.
  Which means reverting these patches aren't good idea, so that
  we can enclose inode flag changes in the same transaction.

  Also similarly btrfs_set_acl() -> do_set_acl() should open code
  the do_set_acl() in btrfs_set_acl() so that it can start the
  transaction after all checks.

  Are you ok to keep these above patches [1], sorry now I have
  a stronger reason why we should start transaction in the parent
  functions.

Thanks, Anand