On Thu, Jul 2, 2020 at 2:04 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> On 7/2/20 7:32 AM, fdmanana@xxxxxxxxxx wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > When syncing the log, we used to update the log root tree without holding
> > neither the log_mutex of the subvolume root nor the log_mutex of log root
> > tree.
> >
> > We used to have two critical sections delimited by the log_mutex of the
> > log root tree, so in the first one we incremented the log_writers of the
> > log root tree and on the second one we decremented it and waited for the
> > log_writers counter to go down to zero. This was because the update of
> > the log root tree happened between the two critical sections.
> >
> > The use of two critical sections allowed a little bit more of parallelism
> > and required the use of the log_writers counter, necessary to make sure
> > we didn't miss any log root tree update when we have multiple tasks trying
> > to sync the log in parallel.
> >
> > However after commit 06989c799f0481 ("Btrfs: fix race updating log root
> > item during fsync") the log root tree update was moved into a critical
> > section delimited by the subvolume's log_mutex. Later another commit
> > moved the log tree update from that critical section into the second
> > critical section delimited by the log_mutex of the log root tree. Both
> > commits addressed different bugs.
> >
> > The end result is that the first critical section delimited by the
> > log_mutex of the log root tree became pointless, since there's nothing
> > done between it and the second critical section, we just have an unlock
> > of the log_mutex followed by a lock operation. This means we can merge
> > both critical sections, as the first one does almost nothing now, and we
> > can stop using the log_writers counter of the log root tree, which was
> > incremented in the first critical section and decremented in the second
> > criticial section, used to make sure no one in the second critical section
> > started writeback of the log root tree before some other task updated it.
> >
> > So just remove the mutex_unlock() followed by mutex_lock() of the log root
> > tree, as well as the use of the log_writers counter for the log root tree.
> >
> > This patch is part of a series that has the following patches:
> >
> > 1/4 btrfs: only commit the delayed inode when doing a full fsync
> > 2/4 btrfs: only commit delayed items at fsync if we are logging a directory
> > 3/4 btrfs: stop incremening log_batch for the log root tree when syncing log
> > 4/4 btrfs: remove no longer needed use of log_writers for the log root tree
> >
> > After the entire patchset applied I saw about 12% decrease on max latency
> > reported by dbench. The test was done on a qemu vm, with 8 cores, 16Gb of
> > ram, using kvm and using a raw NVMe device directly (no intermediary fs on
> > the host). The test was invoked like the following:
> >
> > mkfs.btrfs -f /dev/sdk
> > mount -o ssd -o nospace_cache /dev/sdk /mnt/sdk
> > dbench -D /mnt/sdk -t 300 8
> > umount /mnt/dsk
> >
> > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
>
> Lol oops, did I leave it like that?
Well, before you moved the log root tree update, I moved it first.
So back then I didn't notice it or left it for later and then forgot.
And later you missed it too, so we're even ;)
>
> Reviewed-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
>
> Thanks,
>
> Josef
