Re: [PATCH AUTOSEL 6.8 59/68] btrfs: preallocate temporary extent buffer for inode logging when needed

[Sasha Levin]

On Tue, Apr 02, 2024 at 03:35:18PM +0200, David Sterba wrote:
> On Fri, Mar 29, 2024 at 08:25:55AM -0400, Sasha Levin wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > [ Upstream commit e383e158ed1b6abc2d2d3e6736d77a46393f80fa ]
> >
> > When logging an inode and we require to copy items from subvolume leaves
> > to the log tree, we clone each subvolume leaf and than use that clone to
> > copy items to the log tree. This is required to avoid possible deadlocks
> > as stated in commit 796787c978ef ("btrfs: do not modify log tree while
> > holding a leaf from fs tree locked").
> >
> > The cloning requires allocating an extent buffer (struct extent_buffer)
> > and then allocating pages (folios) to attach to the extent buffer. This
> > may be slow in case we are under memory pressure, and since we are doing
> > the cloning while holding a read lock on a subvolume leaf, it means we
> > can be blocking other operations on that leaf for significant periods of
> > time, which can increase latency on operations like creating other files,
> > renaming files, etc. Similarly because we're under a log transaction, we
> > may also cause extra delay on other tasks doing an fsync, because syncing
> > the log requires waiting for tasks that joined a log transaction to exit
> > the transaction.
> >
> > So to improve this, for any inode logging operation that needs to copy
> > items from a subvolume leaf ("full sync" or "copy everything" bit set
> > in the inode), preallocate a dummy extent buffer before locking any
> > extent buffer from the subvolume tree, and even before joining a log
> > transaction, add it to the log context and then use it when we need to
> > copy items from a subvolume leaf to the log tree. This avoids making
> > other operations get extra latency when waiting to lock a subvolume
> > leaf that is used during inode logging and we are under heavy memory
> > pressure.
> >
> > The following test script with bonnie++ was used to test this:
> >
> >   $ cat test.sh
> >   #!/bin/bash
> >
> >   DEV=/dev/sdh
> >   MNT=/mnt/sdh
> >   MOUNT_OPTIONS="-o ssd"
> >
> >   MEMTOTAL_BYTES=`free -b | grep Mem: | awk '{ print $2 }'`
> >   NR_DIRECTORIES=20
> >   NR_FILES=20480
> >   DATASET_SIZE=$((MEMTOTAL_BYTES * 2 / 1048576))
> >   DIRECTORY_SIZE=$((MEMTOTAL_BYTES * 2 / NR_FILES))
> >   NR_FILES=$((NR_FILES / 1024))
> >
> >   echo "performance" | \
> >       tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_governor
> >
> >   umount $DEV &> /dev/null
> >   mkfs.btrfs -f $MKFS_OPTIONS $DEV
> >   mount $MOUNT_OPTIONS $DEV $MNT
> >
> >   bonnie++ -u root -d $MNT \
> >       -n $NR_FILES:$DIRECTORY_SIZE:$DIRECTORY_SIZE:$NR_DIRECTORIES \
> >       -r 0 -s $DATASET_SIZE -b
> >
> >   umount $MNT
> >
> > The results of this test on a 8G VM running a non-debug kernel (Debian's
> > default kernel config), were the following.
> >
> > Before this change:
> >
> >   Version 2.00a       ------Sequential Output------ --Sequential Input- --Random-
> >                       -Per Chr- --Block-- -Rewrite- -Per Chr- --Block-- --Seeks--
> >   Name:Size etc        /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP
> >   debian0       7501M  376k  99  1.4g  96  117m  14 1510k  99  2.5g  95 +++++ +++
> >   Latency             35068us   24976us    2944ms   30725us   71770us   26152us
> >   Version 2.00a       ------Sequential Create------ --------Random Create--------
> >   debian0             -Create-- --Read--- -Delete-- -Create-- --Read--- -Delete--
> >   files:max:min        /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP
> >   20:384100:384100/20 20480  32 20480  58 20480  48 20480  39 20480  56 20480  61
> >   Latency               411ms   11914us     119ms     617ms   10296us     110ms
> >
> > After this change:
> >
> >   Version 2.00a       ------Sequential Output------ --Sequential Input- --Random-
> >                       -Per Chr- --Block-- -Rewrite- -Per Chr- --Block-- --Seeks--
> >   Name:Size etc        /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP
> >   debian0       7501M  375k  99  1.4g  97  117m  14 1546k  99  2.3g  98 +++++ +++
> >   Latency             35975us  20945us    2144ms   10297us    2217us    6004us
> >   Version 2.00a       ------Sequential Create------ --------Random Create--------
> >   debian0             -Create-- --Read--- -Delete-- -Create-- --Read--- -Delete--
> >   files:max:min        /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP  /sec %CP
> >   20:384100:384100/20 20480  35 20480  58 20480  48 20480  40 20480  57 20480  59
> >   Latency               320ms   11237us   77779us     518ms    6470us   86389us
> >
> > Reviewed-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > Reviewed-by: David Sterba <dsterba@xxxxxxxx>
> > Signed-off-by: David Sterba <dsterba@xxxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
>
> This is a performance improvement, how does this qualify for stable? I
> read only about notable perfromance fixes but this is not one.

No objection to dropping it. Description of the commit states that it
fixes blocking for "significant amount of time".

--
Thanks,
Sasha