Re: [PATCH] btrfs: don't commit transaction for every subvol create

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/4/12 10:34, Qu Wenruo wrote:
> 
> 
> On 2023/4/12 09:45, Neal Gompa wrote:
>> On Tue, Apr 11, 2023 at 3:22 PM Sweet Tea Dorminy
>> <sweettea-kernel@dorminy.me> wrote:
>>>
>>> Recently a Meta-internal workload encountered subvolume creation taking
>>> up to 2s each, significantly slower than directory creation. As they
>>> were hoping to be able to use subvolumes instead of directories, and
>>> were looking to create hundreds, this was a significant issue. After
>>> Josef investigated, it turned out to be due to the transaction commit
>>> currently performed at the end of subvolume creation.
>>>
>>> This change improves the workload by not doing transaction commit for 
>>> every
>>> subvolume creation, and merely requiring a transaction commit on fsync.
>>> In the worst case, of doing a subvolume create and fsync in a loop, this
>>> should require an equal amount of time to the current scheme; and in the
>>> best case, the internal workload creating hundreds of subvols before
>>> fsyncing is greatly improved.
>>>
>>> While it would be nice to be able to use the log tree and use the normal
>>> fsync path, logtree replay can't deal with new subvolume inodes
>>> presently.
>>>
>>> It's possible that there's some reason that the transaction commit is
>>> necessary for correctness during subvolume creation; however,
>>> git logs indicate that the commit dates back to the beginning of
>>> subvolume creation, and there are no notes on why it would be necessary.
>>>
>>> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
>>> ---
>>>   fs/btrfs/ioctl.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index 25833b4eeaf5..a6f1ee2dc1b9 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -647,6 +647,8 @@ static noinline int create_subvol(struct 
>>> mnt_idmap *idmap,
>>>          }
>>>          trans->block_rsv = &block_rsv;
>>>          trans->bytes_reserved = block_rsv.size;
>>> +       /* tree log can't currently deal with an inode which is a new 
>>> root */
>>> +       btrfs_set_log_full_commit(trans);
>>>
>>>          ret = btrfs_qgroup_inherit(trans, 0, objectid, inherit);
>>>          if (ret)
>>> @@ -755,10 +757,7 @@ static noinline int create_subvol(struct 
>>> mnt_idmap *idmap,
>>>          trans->bytes_reserved = 0;
>>>          btrfs_subvolume_release_metadata(root, &block_rsv);
>>>
>>> -       if (ret)
>>> -               btrfs_end_transaction(trans);
>>> -       else
>>> -               ret = btrfs_commit_transaction(trans);
>>> +       btrfs_end_transaction(trans);
>>>   out_new_inode_args:
>>>          btrfs_new_inode_args_destroy(&new_inode_args);
>>>   out_inode:
>>> -- 
>>> 2.40.0
>>>
>>
>> Wouldn't the consequence of this mean that we lose some guarantees
>> around subvolume creation? That is, without it being committed, we
>> would have a window in which the subvolume and data can be written
>> without being written to disk? If so, how large is that window?
> 
> That can be avoided in btrfs-progs, by adding some -C|-c options just 
> like subvolume deletion to ensure the case.
> 
> 
> If you're really concern about the window size, it's no smaller than 
> commit interval (by default 30s).

s/smaller/larger/

> 
> 
> But that's not the full story, if you do fsync() on any inode of the new 
> subvolume, it should cause a full commit transaction, which would write 
> every needed metadata, including the subvolume itself, to the disk.
> 
> So the end result is, you don't really need to bother the subvolume 
> itself, just do your usual fsync() work on the inode you're really 
> concerned, no need to bother the subvolume itself.
> 
> Thanks,
> Qu
> 
>>
>>