Re: [PATCH 0/6] cleanup property and extended attribute set

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

On 17/4/19 5:31 PM, David Sterba wrote:
> On Tue, Apr 16, 2019 at 06:01:58AM +0800, Anand Jain wrote:
>>
>>
>> On 16/4/19 3:01 am, David Sterba wrote:
>>> On Fri, Apr 12, 2019 at 04:02:53PM +0800, Anand Jain wrote:
>>>> In an attempt to stream line the property and extended attribute set here
>>>> are the few cleanup patches.
>>>>
>>>>    1/6 to 3/6 are mostly non functional cleanups (except for the conversion
>>>>     to non static function in 3/6) and can be merged together.
>>>>    4/6 removes the readonly root check in btrfs_setxattr() more details in
>>>>     the change log.
>>>>    5/6 as now we have btrfs_setxattr() and btrfs_setxattr_trans() for the
>>>>     threads with transaction and without transaction respectively, so this
>>>>     patch uses them.
>>>>    6/6 as 5/6 as diverted the threads with transaction to btrfs_setxattr(),
>>>>     now btrfs_setxattr_trans() can drop the trans arg.
>>>>
>>>> Anand Jain (6):
>>>>     btrfs: rename btrfs_setxattr to btrfs_setxattr_trans
>>>>     btrfs: rename do_setxattr to btrfs_setxattr
>>>>     btrfs: declare btrfs_setxattr as a non static function
>>>>     btrfs: remove redundant readonly root check in btrfs_setxattr_trans
>>>>     btrfs: split thread with trans to use btrfs_setxattr
>>>>     btrfs: cleanup btrfs_setxattr_trans drop trans arg
>>>
>>> Looks good to me, thanks. The result is very close to what the previous
>>> patchset did. Patchset will go to for-next soon.
>>>
>>
>> Thanks. On top of these, I am writing patches to merge
>> start_transactions in btrfs_ioctl_setflags().
> 
> My current idea how to change btrfs_ioctl_setflags is like that (but I
> haven't prototyped it so it might not work):
> 
> - don't change binode->flags directly, but do all flag updates on a
>    temporary variable
> 
> - if a property needs to be changed, do validation first, then start
>    transaction and pass it to the property handler

  In btrfs_ioctl_setflags() we don't need the validate() at all, as the
  the property strings are hard coded with in the kernel or already
  verified during the mount -o option (fs_info->compress_type).
  So only place where we need the validate is
  btrfs_xattr_handler_set_prop().

Thanks, Anand

> - in the finalizing code, start a transaction unless it's been started
>    already, apply the iflags and end transaction
> 
> This means there are up to 4 starting points of transaction, but the
> property validation should never fail between start/end region. There
> are other potential failures due to ENOMEM or ENOSPC, but that's the
> general set of errors we can't avoid.
>