| Message ID | 76ddeec8b7de89c338b8cb94ee2c4015a0be6e2f.1622386360.git.riteshh@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Fix return value of btrfs_mark_extent_written() in case of error | expand |
On Sun, May 30, 2021 at 08:24:05PM +0530, Ritesh Harjani wrote: > We always return 0 even in case of an error in btrfs_mark_extent_written(). > Fix it to return proper error value in case of a failure. Oh right, this looks like it got forgotten, the whole function does the right thing regarding errors and the callers also handle it. > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > --- > Tested fstests with "-g quick" group. No new failure seen. That won't be probably enough to trigger the error paths but the patch otherwise looks correct to me. Added to misc-next, thanks.
On 21/05/31 08:49PM, David Sterba wrote: > On Sun, May 30, 2021 at 08:24:05PM +0530, Ritesh Harjani wrote: > > We always return 0 even in case of an error in btrfs_mark_extent_written(). > > Fix it to return proper error value in case of a failure. > > Oh right, this looks like it got forgotten, the whole function does the > right thing regarding errors and the callers also handle it. > > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > > --- > > Tested fstests with "-g quick" group. No new failure seen. > > That won't be probably enough to trigger the error paths but the patch Yes. However, I found the bug while stress testing btrfs/062 failure with bs < ps patch series from Qu. On analyzing the kernel panic [1], it looked like it was caused by not properly returning an error from btrfs_mark_extent_written(). So, I thought of just running "-g quick" test to avoid any obvious issue. [1]: https://www.spinics.net/lists/linux-btrfs/msg113280.html > otherwise looks correct to me. Added to misc-next, thanks. Thanks
On 2021/6/1 下午2:01, Ritesh Harjani wrote: > On 21/05/31 08:49PM, David Sterba wrote: >> On Sun, May 30, 2021 at 08:24:05PM +0530, Ritesh Harjani wrote: >>> We always return 0 even in case of an error in btrfs_mark_extent_written(). >>> Fix it to return proper error value in case of a failure. >> >> Oh right, this looks like it got forgotten, the whole function does the >> right thing regarding errors and the callers also handle it. >> >>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> >>> --- >>> Tested fstests with "-g quick" group. No new failure seen. >> >> That won't be probably enough to trigger the error paths but the patch > > Yes. However, I found the bug while stress testing btrfs/062 failure > with bs < ps patch series from Qu. > On analyzing the kernel panic [1], it looked like it was caused by not > properly returning an error from btrfs_mark_extent_written(). > So, I thought of just running "-g quick" test to avoid any obvious issue. In fact for the btrfs/062 bug, it's caused by the full page defrag behavior. I wrongly thought enabling full page defrag would be OK, as most defrag tests are fine, but it turns out that full page defrag is buggy, and can lead to btrfs/062 problem and other ordered extent related problem. Thus now on subpage branch, subpage defrag is fully disabled. For subpage defrag support, the incoming subpage defrag patchset would be the proper way to go. No some compromise like full page defrag. The patch you're submitting is completely fine, I guess it's just btrfs/062 with full page defrag creates a situation to make btrfs_mark_extent_written() to fail. Thanks, Qu > > [1]: https://www.spinics.net/lists/linux-btrfs/msg113280.html > >> otherwise looks correct to me. Added to misc-next, thanks. > > Thanks >
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 3b10d98b4ebb..55f68422061d 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1094,7 +1094,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans, int del_nr = 0; int del_slot = 0; int recow; - int ret; + int ret = 0; u64 ino = btrfs_ino(inode); path = btrfs_alloc_path(); @@ -1315,7 +1315,7 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans, } out: btrfs_free_path(path); - return 0; + return ret; } /*
We always return 0 even in case of an error in btrfs_mark_extent_written(). Fix it to return proper error value in case of a failure. Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> --- Tested fstests with "-g quick" group. No new failure seen. fs/btrfs/file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.31.1