Re: [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/4/18 16:52, Qu Wenruo 写道:
>
>
> 在 2024/4/18 16:48, Anand Jain 写道:
>>
>>
>> On 4/18/24 14:30, Qu Wenruo wrote:
>>>
>>>
>>> 在 2024/4/18 15:44, Anand Jain 写道:
>>>> In the function btrfs_write_marked_extents() and in
>>>> __btrfs_wait_marked_extents()
>>>> return the actual error if when filemap_fdata<write|wait>_range()
>>>> fails.
>>>>
>>>> Suggested-by: Josef Bacik <josef@toxicpanda.com>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>
>>> Looks fine for the patch, although I have a small question.
>>>
>>> If we failed to write some metadata extents, we break out, meaning there
>>> would be dirty metadata still hanging there.
>>>
>>> Would it be a problem?
>>>
>>
>> I had the exact same question, but what I discovered is that the
>> error originated here will lead to our handle errors and to readonly
>> state. So it should be fine.
>
> Yep, I know we would mark the fs error, but would things like
> close_ctree() report other warnings as we still have dirty pages for
> metadata inode?

Nevermind, I just injected a metadata leaf writeback error with this
patch applied, and everything looks fine, no extra warning on btree inode.

So I believe we're already doing proper cleanup.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
>
> Thanks,
> Qu
>
>> Further, if submit layer write/wait is
>> failing there is nothing much we can do as of now. However, in the
>> long run we probably should provide an option to fail-safe.
>>
>> Thanks, Anand
>>
>>> Thanks,
>>> Qu
>>>> ---
>>>> v2: add __btrfs_wait_marked_extents() as well.
>>>>
>>>>   fs/btrfs/transaction.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>>>> index 3e3bcc5f64c6..8c3b3cda1390 100644
>>>> --- a/fs/btrfs/transaction.c
>>>> +++ b/fs/btrfs/transaction.c
>>>> @@ -1156,6 +1156,8 @@ int btrfs_write_marked_extents(struct
>>>> btrfs_fs_info *fs_info,
>>>>           else if (wait_writeback)
>>>>               werr = filemap_fdatawait_range(mapping, start, end);
>>>>           free_extent_state(cached_state);
>>>> +        if (werr)
>>>> +            break;
>>>>           cached_state = NULL;
>>>>           cond_resched();
>>>>           start = end + 1;
>>>> @@ -1198,6 +1200,8 @@ static int __btrfs_wait_marked_extents(struct
>>>> btrfs_fs_info *fs_info,
>>>>           if (err)
>>>>               werr = err;
>>>>           free_extent_state(cached_state);
>>>> +        if (werr)
>>>> +            break;
>>>>           cached_state = NULL;
>>>>           cond_resched();
>>>>           start = end + 1;
>>
>