| Message ID | 20190403165134.12378-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | property fixes and cleanups | expand |
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?
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.
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.
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