| Message ID | 6d8adc363ee08747d4e5ee2f521b1e0e155516a1.1632765815.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Miscellaneous error handling patches | expand |
On Mon, Sep 27, 2021 at 7:07 PM Josef Bacik <josef@toxicpanda.com> wrote: > > Error injection testing uncovered a case where we'd end up with a > corrupt file system with a missing extent in the middle of a file. This > occurs because the if statement to decide if we should abort is wrong. > The only way we would abort in this case is if we got a ret != > -EOPNOTSUPP and we called from the file clone code. However the > prealloc code uses this path too. Instead we need to abort if there is > an error, and the only error we _don't_ abort on is -EOPNOTSUPP and only > if we came from the clone file code. Looks good, but the comment above the conditional in the code needs to be updated, to reflect the new logic as mentioned in the changelog. Thanks. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/file.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index fdceab28587e..f9a1498cf030 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -2710,8 +2710,9 @@ int btrfs_replace_file_extents(struct btrfs_inode *inode, > * returned by __btrfs_drop_extents() without having > * changed anything in the file. > */ > - if (extent_info && !extent_info->is_new_extent && > - ret && ret != -EOPNOTSUPP) > + if (ret && > + (ret != -EOPNOTSUPP || > + (extent_info && extent_info->is_new_extent))) > btrfs_abort_transaction(trans, ret); > break; > } > -- > 2.26.3 >
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index fdceab28587e..f9a1498cf030 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -2710,8 +2710,9 @@ int btrfs_replace_file_extents(struct btrfs_inode *inode, * returned by __btrfs_drop_extents() without having * changed anything in the file. */ - if (extent_info && !extent_info->is_new_extent && - ret && ret != -EOPNOTSUPP) + if (ret && + (ret != -EOPNOTSUPP || + (extent_info && extent_info->is_new_extent))) btrfs_abort_transaction(trans, ret); break; }
Error injection testing uncovered a case where we'd end up with a corrupt file system with a missing extent in the middle of a file. This occurs because the if statement to decide if we should abort is wrong. The only way we would abort in this case is if we got a ret != -EOPNOTSUPP and we called from the file clone code. However the prealloc code uses this path too. Instead we need to abort if there is an error, and the only error we _don't_ abort on is -EOPNOTSUPP and only if we came from the clone file code. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/file.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)