Re: [PATCH 0/5] Misc props.c cleanups

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

On 2/8/19 3:15 PM, Nikolay Borisov wrote:
> 
> 
> On 8.02.19 г. 9:02 ч., Anand Jain wrote:
>> While adding the readmirror property found few cleanup things which
>> can be fixed. As these aren't part of upcoming readmirror property
>> I am sending these separately. Except for 1/5 rest fixes are in
>> props.c. 1/5 is in volume.c.
>>
>> Anand Jain (5):
>>    btrfs: fix comment its device list mutex not volume lock
>>    btrfs: kill __btrfs_set_prop()
>>    btrfs: reorg functions to drop forward declaration
>>    btrfs: drop redundant forward declaration in props.c
>>    btrfs: trivial, fix c coding style
>>
>>   fs/btrfs/ioctl.c   |  10 +--
>>   fs/btrfs/props.c   | 201 ++++++++++++++++++++++++-----------------------------
>>   fs/btrfs/props.h   |   6 +-
>>   fs/btrfs/volumes.c |   2 +-
>>   fs/btrfs/xattr.c   |   2 +-
>>   5 files changed, 100 insertions(+), 121 deletions(-)
>>
> 
> Allow me to suggest to introduce another patch with the following hunk:
> 
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index ea78c3d6dcfc..37e9af2a9f8c 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -229,12 +229,11 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans,
>          if (btrfs_root_readonly(root))
>                  return -EROFS;
> 
> -       if (trans)
> -               return do_setxattr(trans, inode, name, value, size, flags);
> -
> -       trans = btrfs_start_transaction(root, 2);
> -       if (IS_ERR(trans))
> -               return PTR_ERR(trans);
> +       if (!trans) {
> +               trans = btrfs_start_transaction(root, 2);
> +               if (IS_ERR(trans))
> +                       return PTR_ERR(trans);
> +       }
> 
>          ret = do_setxattr(trans, inode, name, value, size, flags);
>          if (ret)
> 
> 
> This just inverts the logic for !trans and makes the code a bit more
> linear and (IMO) easier to follow.
> 


Thanks for the comments.

But we still have to break the linear flow for the case of
  trans != NULL. Where we don't do the
    inode_inc_iversion(inode);
and the rest part.

-Anand