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

[Anand Jain <Anand.Jain@oracle.com>]

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