On Thu, Feb 04, 2021 at 11:50:45AM +0000, Filipe Manana wrote:
> On Thu, Feb 4, 2021 at 10:23 AM Naohiro Aota <naohiro.a...@wdc.com> wrote:
> >
> > This is the 2/3 patch to enable tree-log on zoned filesystems.
> >
> > Since we can start more than one log transactions per subvolume
> > simultaneously, nodes from multiple transactions can be allocated
> > interleaved. Such mixed allocation results in non-sequential writes at the
> > time of a log transaction commit. The nodes of the global log root tree
> > (fs_info->log_root_tree), also have the same problem with mixed
> > allocation.
> >
> > Serializes log transactions by waiting for a committing transaction when
> > someone tries to start a new transaction, to avoid the mixed allocation
> > problem. We must also wait for running log transactions from another
> > subvolume, but there is no easy way to detect which subvolume root is
> > running a log transaction. So, this patch forbids starting a new log
> > transaction when other subvolumes already allocated the global log root
> > tree.
> >
> > Cc: Filipe Manana <fdman...@gmail.com>
> > Reviewed-by: Josef Bacik <jo...@toxicpanda.com>
> > Signed-off-by: Naohiro Aota <naohiro.a...@wdc.com>
> > ---
> > fs/btrfs/tree-log.c | 29 +++++++++++++++++++++++++++++
> > 1 file changed, 29 insertions(+)
> >
> > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > index c02eeeac439c..8be3164d4c5d 100644
> > --- a/fs/btrfs/tree-log.c
> > +++ b/fs/btrfs/tree-log.c
> > @@ -105,6 +105,7 @@ static noinline int replay_dir_deletes(struct
> > btrfs_trans_handle *trans,
> > struct btrfs_root *log,
> > struct btrfs_path *path,
> > u64 dirid, int del_all);
> > +static void wait_log_commit(struct btrfs_root *root, int transid);
> >
> > /*
> > * tree logging is a special write ahead log used to make sure that
> > @@ -140,6 +141,7 @@ static int start_log_trans(struct btrfs_trans_handle
> > *trans,
> > {
> > struct btrfs_fs_info *fs_info = root->fs_info;
> > struct btrfs_root *tree_root = fs_info->tree_root;
> > + const bool zoned = btrfs_is_zoned(fs_info);
> > int ret = 0;
> >
> > /*
> > @@ -160,12 +162,20 @@ static int start_log_trans(struct btrfs_trans_handle
> > *trans,
> >
> > mutex_lock(&root->log_mutex);
> >
> > +again:
> > if (root->log_root) {
> > + int index = (root->log_transid + 1) % 2;
> > +
> > if (btrfs_need_log_full_commit(trans)) {
> > ret = -EAGAIN;
> > goto out;
> > }
> >
> > + if (zoned && atomic_read(&root->log_commit[index])) {
> > + wait_log_commit(root, root->log_transid - 1);
> > + goto again;
> > + }
> > +
> > if (!root->log_start_pid) {
> > clear_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
> > root->log_start_pid = current->pid;
> > @@ -173,6 +183,17 @@ static int start_log_trans(struct btrfs_trans_handle
> > *trans,
> > set_bit(BTRFS_ROOT_MULTI_LOG_TASKS, &root->state);
> > }
> > } else {
> > + if (zoned) {
> > + mutex_lock(&fs_info->tree_log_mutex);
> > + if (fs_info->log_root_tree)
> > + ret = -EAGAIN;
> > + else
> > + ret = btrfs_init_log_root_tree(trans,
> > fs_info);
> > + mutex_unlock(&fs_info->tree_log_mutex);
> > + }
>
> So, nothing here changed since v14 - all my comments still apply [1]
> This is based on pre-5.10 code and is broken as it is - it results in
> every fsync falling back to a transaction commit, defeating the
> purpose of all the patches that deal with log trees on zoned
> filesystems.
>
> Thanks.
>
> [1]
> https://lore.kernel.org/linux-btrfs/cal3q7h5pv416fvwthohe+m3l5b-z_n6_zgqqxsuq5vc5fsa...@mail.gmail.com/
Yes...
As noted in the cover letter, there is a fix for this issue
itself. However, the fix revealed other failures in fsync() path.
But, with further investigation, I found the failures are not really
related to zoned fsync() code. So, I will soon post two patches (one
incremental for this one, and one to deal with a regression case)..
>
>
> > + if (ret)
> > + goto out;
> > +
> > ret = btrfs_add_log_tree(trans, root);
> > if (ret)
> > goto out;
> > @@ -201,14 +222,22 @@ static int start_log_trans(struct btrfs_trans_handle
> > *trans,
> > */
> > static int join_running_log_trans(struct btrfs_root *root)
> > {
> > + const bool zoned = btrfs_is_zoned(root->fs_info);
> > int ret = -ENOENT;
> >
> > if (!test_bit(BTRFS_ROOT_HAS_LOG_TREE, &root->state))
> > return ret;
> >
> > mutex_lock(&root->log_mutex);
> > +again:
> > if (root->log_root) {
> > + int index = (root->log_transid + 1) % 2;
> > +
> > ret = 0;
> > + if (zoned && atomic_read(&root->log_commit[index])) {
> > + wait_log_commit(root, root->log_transid - 1);
> > + goto again;
> > + }
> > atomic_inc(&root->log_writers);
> > }
> > mutex_unlock(&root->log_mutex);
> > --
> > 2.30.0
> >
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”
