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

On Fri, Feb 5, 2021 at 11:49 AM Filipe Manana [off-list ref] wrote:

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?

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)

quoted

+        */
+       if (ret == -EAGAIN)
+               ret = 0;

Thinking again about this, it would be safer, and self-documenting to
check here that we are in zoned mode:

if (ret == -EAGAIN && is_zoned)
    ret = 0;

Because if we start to get -EAGAIN here one day, from non-zoned code,
we risk not writing out some extent buffer and getting a corrupt log,
which may be very hard to find.
With that additional check in place, we'll end up aborting the
transaction with -EAGAIN and notice the problem much sooner.

quoted

        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) {

I would add "&& is_zoned" here too.

Thanks.

quoted

+               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);

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