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

[Martin Raiber <martin@urbackup.org>]

On 05.08.2020 01:08 Josef Bacik wrote:
> 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,
 From a user perspective I'd appreciate having the option to set it in a 
non-durable way (I have seen btrfs property sets hanging for a long time 
as well). But currently my application kind of depends on it being 
durable. Making it non-durable wouldn't break much and I guess the old 
behaviour could be emulated by a "btrfs fi sync <subvol>" afterwards, 
but idk how much other stuff depends on it being durable. Making it 
consistent with btrfs subvol del with the "-c" switch would be nice and 
consistent as well (and the -c switch could be done via IOC_SYNC after 
setting the properties).