[0/4] property fixes and cleanups
mbox series

Message ID 20190403165134.12378-1-anand.jain@oracle.com
Headers show
Series
  • property fixes and cleanups
Related show

Message

Anand Jain April 3, 2019, 4:51 p.m. UTC
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(-)

Comments

Anand Jain April 8, 2019, 3:25 a.m. UTC | #1
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?
David Sterba April 8, 2019, 5:02 p.m. UTC | #2
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.
David Sterba April 8, 2019, 11:23 p.m. UTC | #3
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.
Anand Jain April 9, 2019, 5:34 a.m. UTC | #4
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