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

[Filipe Manana <fdmanana@gmail.com>]

On Fri, Feb 5, 2021 at 2:58 PM Naohiro Aota <naohiro.aota@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. This happens when someone,
> e.g., a concurrent transaction commit, writes a dirty extent in this
> tree-log commit.
>
> If we are not going to wait for the extents, we can hope the concurrent
> writing fills the hole for us. So, we can ignore the error in this case and
> hope the next write will succeed.
>
> If we want to wait for them and got the error, we cannot wait for them
> because it will cause a deadlock. So, let's bail out to a full commit in
> this case.
>
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

It looks good now, thanks!

> ---
>  fs/btrfs/tree-log.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index fc04625cbbd1..d90695c1ab6c 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3120,6 +3120,17 @@ 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);
> +       /*
> +        * -EAGAIN happens when someone, e.g., a concurrent transaction
> +        *  commit, writes a dirty extent in this tree-log commit. This
> +        *  concurrent write will create a hole writing out the extents,
> +        *  and we cannot proceed on a zoned filesystem, requiring
> +        *  sequential writing. While we can bail out to a full commit
> +        *  here, but we can continue hoping the concurrent writing fills
> +        *  the hole.
> +        */
> +       if (ret == -EAGAIN && btrfs_is_zoned(fs_info))
> +               ret = 0;
>         if (ret) {
>                 blk_finish_plug(&plug);
>                 btrfs_abort_transaction(trans, ret);
> @@ -3242,7 +3253,17 @@ 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) {
> +       /*
> +        * As described above, -EAGAIN indicates a hole in the extents. We
> +        * cannot wait for these write outs since the waiting cause a
> +        * deadlock. Bail out to the full commit instead.
> +        */
> +       if (ret == -EAGAIN && btrfs_is_zoned(fs_info)) {
> +               btrfs_set_log_full_commit(trans);
> +               btrfs_wait_tree_log_extents(log, mark);
> +               mutex_unlock(&log_root_tree->log_mutex);
> +               goto out_wake_log_root;
> +       } 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.”