Re: [PATCH v15 43/43] btrfs: zoned: deal with holes writing out tree-log pages

On Fri, Feb 5, 2021 at 12:55 PM Naohiro Aota [off-list ref] wrote:

On Fri, Feb 05, 2021 at 11:49:05AM +0000, Filipe Manana wrote:

quoted

On Fri, Feb 5, 2021 at 9:26 AM Naohiro Aota [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Naohiro Aota <naohiro.aota@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.

quoted

quoted

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

quoted

+        * 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.

quoted

quoted

+        */
+       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

quoted

+        * 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.

quoted

Thanks.

quoted

+       } 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.”