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
