* [PATCH 1/1] btrfs: report filemap_fdata<write|wait>_range error
@ 2024-04-16 2:36 Anand Jain
2024-04-18 6:14 ` [PATCH v2] " Anand Jain
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2024-04-16 2:36 UTC (permalink / raw)
To: linux-btrfs; +Cc: Anand Jain, Josef Bacik
In the function btrfs_write_marked_extents() in the while loop check and break if
filemap_fdata<write|wait>_range() has any error.
Suggested-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/transaction.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index df2e58aa824a..a1c43fa6fd3d 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;
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error
2024-04-16 2:36 [PATCH 1/1] btrfs: report filemap_fdata<write|wait>_range error Anand Jain
@ 2024-04-18 6:14 ` Anand Jain
2024-04-18 6:30 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2024-04-18 6:14 UTC (permalink / raw)
To: linux-btrfs; +Cc: Anand Jain, Josef Bacik
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>
---
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;
--
2.39.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error
2024-04-18 6:14 ` [PATCH v2] " Anand Jain
@ 2024-04-18 6:30 ` Qu Wenruo
2024-04-18 7:18 ` Anand Jain
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2024-04-18 6:30 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: Josef Bacik
在 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?
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error
2024-04-18 6:30 ` Qu Wenruo
@ 2024-04-18 7:18 ` Anand Jain
2024-04-18 7:22 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2024-04-18 7:18 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs; +Cc: Josef Bacik
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. 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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error
2024-04-18 7:18 ` Anand Jain
@ 2024-04-18 7:22 ` Qu Wenruo
2024-04-18 8:23 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2024-04-18 7:22 UTC (permalink / raw)
To: Anand Jain, Qu Wenruo, linux-btrfs; +Cc: Josef Bacik
在 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?
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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] btrfs: report filemap_fdata<write|wait>_range error
2024-04-18 7:22 ` Qu Wenruo
@ 2024-04-18 8:23 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2024-04-18 8:23 UTC (permalink / raw)
To: Qu Wenruo, Anand Jain, linux-btrfs; +Cc: Josef Bacik
在 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;
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-18 8:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-16 2:36 [PATCH 1/1] btrfs: report filemap_fdata<write|wait>_range error Anand Jain
2024-04-18 6:14 ` [PATCH v2] " Anand Jain
2024-04-18 6:30 ` Qu Wenruo
2024-04-18 7:18 ` Anand Jain
2024-04-18 7:22 ` Qu Wenruo
2024-04-18 8:23 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).