Re: [PATCH RFC] btrfs: change commit txn to end txn in subvol_setflags ioctl

[Josef Bacik <josef@toxicpanda.com>]

On 8/4/20 6:48 PM, Qu Wenruo wrote:
> 
> 
> On 2020/8/5 上午1:55, Boris Burkov wrote:
>> Currently, btrfs_ioctl_subvol_setflags forces a btrfs_commit_transaction
>> while holding subvol_sem. As a result, we have seen workloads where
>> calling `btrfs property set -ts <subvol> ro false` hangs waiting for a
>> legitimately slow commit. This gets even worse if the workload tries to
>> set flags on multiple subvolumes and the ioctls pile up on subvol_sem.
>>
>> Change the commit to a btrfs_end_transaction so that the ioctl can
>> return in a timely fashion and piggy back on a later commit.
>>
>> Signed-off-by: Boris Burkov <boris@bur.io>
>> ---
>>   fs/btrfs/ioctl.c       | 2 +-
>>   fs/btrfs/transaction.c | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index bd3511c5ca81..3ae484768ce7 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1985,7 +1985,7 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>   		goto out_reset;
>>   	}
>>   
>> -	ret = btrfs_commit_transaction(trans);
>> +	ret = btrfs_end_transaction(trans);
> 
> This means the setflag is not committed to disk, and if a powerloss
> happens before a transaction commit, then the setflag operation just get
> lost.
> 
> This means, previously if this ioctl returns, users can expect that the
> flag is always set no matter what, but now there is no guarantee.
> 
> Personally I'm not sure if we really want that operation to be committed
> to disk.
> Maybe that transaction commit can be initialized in user space, so for
> multiple setflags, we only commit once, thus saves a lot of time.
> 

I'm of the opinion that we shouldn't be committing the transaction for stuff 
like this, unless there's a really good reason to.  Especially given we're 
holding the subvol lock here, we should just do end_transaction.  Thanks,

Josef