On Fri, Jul 3, 2020 at 1:08 PM David Sterba <dsterba@xxxxxxx> wrote:
>
> On Thu, Jul 02, 2020 at 12:31:59PM +0100, fdmanana@xxxxxxxxxx wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > Commit 2c2c452b0cafdc ("Btrfs: fix fsync when extend references are added
> > to an inode") forced a commit of the delayed inode when logging an inode
> > in order to ensure we would end up logging the inode item during a full
> > fsync. By committing the delayed inode, we updated the inode item in the
> > fs/subvolume tree and then later when copying items from leafs modified in
> > the current transaction into the log tree (with copy_inode_items_to_log())
> > we ended up copying the inode item from the fs/subvolume tree into the log
> > tree. Logging an up to date version of the inode item is required to make
> > sure at log replay time we get the link count fixup triggered among other
> > things (replay xattr deletes, etc). The test case generic/040 from fstests
> > exercises the bug which that commit fixed.
> >
> > However for a fast fsync we don't need to commit the delayed inode because
> > we always log an up to date version of the inode item based on the struct
> > btrfs_inode we have in-memory. We started doing this for fast fsyncs since
> > commit e4545de5b035c7 ("Btrfs: fix fsync data loss after append write").
> >
> > So just stop committing the delayed inode if we are doing a fast fsync,
> > we are only wasting time and adding contention on fs/subvolume 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.
>
> That's impressive. Getting reliable perf improvements in the low
> percents is hard and 10+ is beyond expectations.
Well, I don't find it that impressive. Not that it isn't good, just
not that huge.
While the reported aggregated max latency decreases by around 12%, the
aggregated throughput only increased by around 1.2 to 1.5%, probably
just noise.
>
> As the patches are short I'd like to tag them for stable. The closest
> one that applies to all is 5.4, that I determined from the commit
> references in the changelogs. However I'd appreciate if you could take a
> look if it's worth to tag the patches for older stable trees where it
> applies (since 4.4). I don't have full overview of all the logging or
> fsync updates so might miss some dependency. Thanks.
Normally I don't count changes like these for stable, unless they fix
a significant performance regression.
But I don't oppose adding them to stable either, to whichever releases
they apply cleanly.
thanks
