| Message ID | 1556267cd2fd5f2d560a50b4b169ec4d58c95221.1713334462.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] btrfs: report filemap_fdata<write|wait>_range error | expand |
在 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;
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;
在 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; >
在 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; >> >
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;
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(+)