On 23.05.2018 18:38, Josef Bacik wrote: > It's just removing all of the code that is no longer needed with the > unconditional wait_ordered_extents, it's not that complicated. Just because something is painfully obvious to you doesn't mean it's the same for others. Especially given the current complexity of fsync code. Even your 1 sentence reply that you are removing code which is obsoleted by unconditional wait_ordered_extents is more revealing (albeit frankly this needs a lot more explanation how the code is intertwined etc...) than : "Fix this by getting rid of all of the logged extents magic and simply wait on ordered extent before we star the tree log stuff" > Thanks, > > Josef > > On Wed, May 23, 2018 at 8:24 AM, David Sterba <dsterba@xxxxxxx> wrote: >> On Tue, May 22, 2018 at 01:47:23PM -0400, Josef Bacik wrote: >>> From: Josef Bacik <jbacik@xxxxxx> >>> >>> There's a priority inversion that exists currently with btrfs fsync. In >>> some cases we will collect outstanding ordered extents onto a list and >>> only wait on them at the very last second. However this "very last >>> second" falls inside of a transaction handle, so if we are in a lower >>> priority cgroup we can end up holding the transaction open for longer >>> than needed, so if a high priority cgroup is also trying to fsync() >>> it'll see latency. >>> >>> Fix this by getting rid of all of the logged extents magic and simply >>> wait on ordered extent before we star the tree log stuff. This code has >>> changed a lot since I first wrote it and really isn't the performance >>> win it was originally because of the things we had to do around getting >>> the right checksums. Killing all of this makes our lives easier and >>> gets rid of the priority inversion. >>> >>> Signed-off-by: Josef Bacik <jbacik@xxxxxx> >>> --- >>> fs/btrfs/file.c | 56 ++------------- >>> fs/btrfs/ordered-data.c | 123 -------------------------------- >>> fs/btrfs/ordered-data.h | 20 +----- >>> fs/btrfs/tree-log.c | 166 ++++--------------------------------------- >>> include/trace/events/btrfs.h | 1 - >>> 5 files changed, 19 insertions(+), 347 deletions(-) >> >> Seriously. Just by the diffstat, this patch is going to bring more >> problems than it's supposed to solve. Please split it into reviewable >> pieces and write less vague changelogs so also other people can >> understand and review the actual changes made to the code. Thanks. > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
