Re: [PATCH v4 06/10] btrfs: start transaction in btrfs_set_prop_trans

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

On 2/28/19 12:08 AM, David Sterba wrote:
> On Sat, Feb 23, 2019 at 01:39:48AM +0800, Anand Jain wrote:
>> In preparation to make trans argument of btrfs_setxattr() a mandatory,
>> start the transaction in btrfs_set_prop_trans.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v4: born
>>   fs/btrfs/props.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index e581da1bfbb6..f878ba3160f0 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -4,6 +4,7 @@
>>    */
>>   
>>   #include <linux/hashtable.h>
>> +#include <linux/iversion.h>
>>   #include "props.h"
>>   #include "btrfs_inode.h"
>>   #include "transaction.h"
>> @@ -103,7 +104,25 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>>   int btrfs_set_prop_trans(struct inode *inode, const char *name,
>>   			   const char *value, size_t value_len, int flags)
>>   {
>> -	return btrfs_set_prop(NULL, inode, name, value, value_len, flags);
>> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>> +	struct btrfs_trans_handle *trans;
>> +	int ret;
>> +
>> +	trans = btrfs_start_transaction(root, 2);
>> +	if (IS_ERR(trans))
>> +		return PTR_ERR(trans);
>> +
>> +	ret = btrfs_set_prop(trans, inode, name, value, value_len, flags);
>> +
>> +	if (!ret) {
>> +		inode_inc_iversion(inode);
>> +		inode->i_ctime = current_time(inode);
>> +		set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags);
>> +		ret = btrfs_update_inode(trans, root, inode);
>> +		ASSERT(!ret);
> 
> This is not right. The previous code uses BUG_ON which is also not
> right, but does not silently continue if asserts are compiled out.
> Please add proper error handling here.

  Error handling should save and undo of inode version, i_ctime and
  runtime_flags. Is a new patch for this OK? and here will use BUG_ON
  as in the original.

Thanks, Anand

>> +	}
>> +	btrfs_end_transaction(trans);
>> +	return ret;
>>   }
>>   
>>   static int iterate_object_props(struct btrfs_root *root,
>> -- 
>> 1.8.3.1