On Fri, Feb 5, 2021 at 12:55 PM Naohiro Aota <naohiro.a...@wdc.com> wrote:
>
> On Fri, Feb 05, 2021 at 11:49:05AM +0000, Filipe Manana wrote:
> > On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota <naohiro.a...@wdc.com> wrote:
> > >
> > > Since the zoned filesystem requires sequential write out of metadata, we
> > > cannot proceed with a hole in tree-log pages. When such a hole exists,
> > > btree_write_cache_pages() will return -EAGAIN. We cannot wait for the
> > > range
> > > to be written, because it will cause a deadlock. So, let's bail out to a
> > > full commit in this case.
> > >
> > > Cc: Filipe Manana <fdman...@gmail.com>
> > > Signed-off-by: Naohiro Aota <naohiro.a...@wdc.com>
> > > ---
> > > fs/btrfs/tree-log.c | 19 ++++++++++++++++++-
> > > 1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > This patch solves a regression introduced by fixing patch 40. I'm
> > > sorry for the confusing patch numbering.
> >
> > Hum, how does patch 40 can cause this?
> > And is it before the fixup or after?
>
> With pre-5.10 code base + zoned series at that time, it passed
> xfstests without this patch.
>
> With current code base + zoned series without the fixup for patch 40,
> it also passed the tests, because we are mostly bailing out to a full
> commit.
>
> The fixup now stressed the new fsync code on zoned mode and revealed
> an issue to have -EAGAIN from btrfs_write_marked_extents(). This error
> happens when a concurrent transaction commit is writing a dirty extent
> in this tree-log commit. This issue didn't occur previously because of
> a longer critical section, I guess.
Ok, if I understand you correctly, the problem is a transaction commit
and an fsync both allocating metadata extents at the same time.
>
> >
> > >
> > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> > > index 4e72794342c0..629e605cd62d 100644
> > > --- a/fs/btrfs/tree-log.c
> > > +++ b/fs/btrfs/tree-log.c
> > > @@ -3120,6 +3120,14 @@ int btrfs_sync_log(struct btrfs_trans_handle
> > > *trans,
> > > */
> > > blk_start_plug(&plug);
> > > ret = btrfs_write_marked_extents(fs_info, &log->dirty_log_pages,
> > > mark);
> > > + /*
> > > + * There is a hole writing out the extents and cannot proceed it
> > > on
> > > + * zoned filesystem, which require sequential writing. We can
> >
> > require -> requires
> >
> > > + * ignore the error for now, since we don't wait for completion
> > > for
> > > + * now.
> >
> > So why can we ignore the error for now?
> > Why not just bail out here and mark the log for full commit? (without
> > a transaction abort)
>
> As described above, -EAGAIN happens when a concurrent process writes
> out an extent buffer of this tree-log commit. This concurrent write
> out will fill a hole for us, so the next write out might
> succeed. Indeed we can bail out here, but I opted to try the next
> write.
Ok, if I understand you correctly, you mean it will be fine if after
this point no one allocates metadata extents from the hole?
I think such a clear explanation would fit nicely in the comment.
Thanks.
>
> > > + */
> > > + if (ret == -EAGAIN)
> > > + ret = 0;
> > > if (ret) {
> > > blk_finish_plug(&plug);
> > > btrfs_abort_transaction(trans, ret);
> > > @@ -3229,7 +3237,16 @@ int btrfs_sync_log(struct btrfs_trans_handle
> > > *trans,
> > > &log_root_tree->dirty_log_pages,
> > > EXTENT_DIRTY | EXTENT_NEW);
> > > blk_finish_plug(&plug);
> > > - if (ret) {
> > > + /*
> > > + * There is a hole in the extents, and failed to sequential write
> > > + * on zoned filesystem. We cannot wait for this write outs, sinc
> > > it
> >
> > this -> these
> >
> > > + * cause a deadlock. Bail out to the full commit, instead.
> > > + */
> > > + if (ret == -EAGAIN) {
> > > + btrfs_wait_tree_log_extents(log, mark);
> > > + mutex_unlock(&log_root_tree->log_mutex);
> > > + goto out_wake_log_root;
> >
> > Must also call btrfs_set_log_full_commit(trans);
>
> Oops, I missed this one.
>
> > Thanks.
> >
> > > + } else if (ret) {
> > > btrfs_set_log_full_commit(trans);
> > > btrfs_abort_transaction(trans, ret);
> > > mutex_unlock(&log_root_tree->log_mutex);
> > > --
> > > 2.30.0
> > >
> >
> >
> > --
> > Filipe David Manana,
> >
> > “Whether you think you can, or you think you can't — you're right.”
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
