| Message ID | f0ac3032fcd07344a84a9b1f7d05f8862aa60760.1655391633.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix error handling of cow_file_range(unlock = 0) | expand |
On Fri, Jun 17, 2022 at 12:45:41AM +0900, Naohiro Aota wrote: > When cow_file_range() fails in the middle of the allocation loop, it > unlocks the pages but remains the ordered extents intact. Thus, we need to s/remains/leaves/ > call btrfs_cleanup_ordered_extents() to finish the created ordered extents. > > Also, we need to call end_extent_writepage() if locked_page is available > because btrfs_cleanup_ordered_extents() never process the region on the > locked_page. > > Furthermore, we need to set the mapping as error if locked_page is > unavailable before unlocking the pages, so that the errno is properly > propagated to the userland. > > CC: stable@vger.kernel.org It would be better to specify a version here. The delalloc paths for compression were (a bit heavily) refactored last year in preparation for the subpage sector size support, so blindly adding this to any stable releases might introduce regressions (assuming the patch does not fail to apply). > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > --- > fs/btrfs/inode.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 4e1100f84a88..cae15924fc99 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -934,8 +934,18 @@ static int submit_uncompressed_range(struct btrfs_inode *inode, > goto out; > } > if (ret < 0) { > - if (locked_page) > + btrfs_cleanup_ordered_extents(inode, locked_page, start, end - start + 1); > + if (locked_page) { > + u64 page_start = page_offset(locked_page); > + u64 page_end = page_start + PAGE_SIZE - 1; > + > + btrfs_page_set_error(inode->root->fs_info, locked_page, > + page_start, PAGE_SIZE); > + set_page_writeback(locked_page); > + end_page_writeback(locked_page); > + end_extent_writepage(locked_page, ret, page_start, page_end); > unlock_page(locked_page); > + } > goto out; > } So as I commented in the previous patch, something is missing here: the call to btrfs_cleanup_ordered_extents() at submit_uncompressed_range() in case cow_file_range() returns an error. Otherwise it looks fine. Thanks. > > @@ -1390,9 +1400,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > * However, in case of unlock == 0, we still need to unlock the pages > * (except @locked_page) to ensure all the pages are unlocked. > */ > - if (!unlock && orig_start < start) > + if (!unlock && orig_start < start) { > + if (!locked_page) > + mapping_set_error(inode->vfs_inode.i_mapping, ret); > extent_clear_unlock_delalloc(inode, orig_start, start - 1, > locked_page, 0, page_ops); > + } > > /* > * For the range (2). If we reserved an extent for our delalloc range > -- > 2.35.1 >
On Fri, Jun 17, 2022 at 11:21:44AM +0100, Filipe Manana wrote: > On Fri, Jun 17, 2022 at 12:45:41AM +0900, Naohiro Aota wrote: > > When cow_file_range() fails in the middle of the allocation loop, it > > unlocks the pages but remains the ordered extents intact. Thus, we need to > > s/remains/leaves/ Will fix. > > > call btrfs_cleanup_ordered_extents() to finish the created ordered extents. > > > > Also, we need to call end_extent_writepage() if locked_page is available > > because btrfs_cleanup_ordered_extents() never process the region on the > > locked_page. > > > > Furthermore, we need to set the mapping as error if locked_page is > > unavailable before unlocking the pages, so that the errno is properly > > propagated to the userland. > > > > CC: stable@vger.kernel.org > > It would be better to specify a version here. > > The delalloc paths for compression were (a bit heavily) refactored last year > in preparation for the subpage sector size support, so blindly adding this > to any stable releases might introduce regressions (assuming the patch does > not fail to apply). Yeah ... I could not find the exact commit. It looks like there is no ordered extent clean up from the beginning. I'll check if the modified cow_file_range() cause a hang on the stable versions. > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > --- > > fs/btrfs/inode.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 4e1100f84a88..cae15924fc99 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -934,8 +934,18 @@ static int submit_uncompressed_range(struct btrfs_inode *inode, > > goto out; > > } > > if (ret < 0) { > > - if (locked_page) > > + btrfs_cleanup_ordered_extents(inode, locked_page, start, end - start + 1); > > + if (locked_page) { > > + u64 page_start = page_offset(locked_page); > > + u64 page_end = page_start + PAGE_SIZE - 1; > > + > > + btrfs_page_set_error(inode->root->fs_info, locked_page, > > + page_start, PAGE_SIZE); > > + set_page_writeback(locked_page); > > + end_page_writeback(locked_page); > > + end_extent_writepage(locked_page, ret, page_start, page_end); > > unlock_page(locked_page); > > + } > > goto out; > > } > > So as I commented in the previous patch, something is missing here: the call to > btrfs_cleanup_ordered_extents() at submit_uncompressed_range() in case cow_file_range() > returns an error. > > Otherwise it looks fine. > Thanks. btrfs_cleanup_ordered_extents() is called at the first line of the added lines. We need to call it regardless of locked_page != NULL or not, because submit_uncompressed_range() can be called with locked_page = NULL. > > > > @@ -1390,9 +1400,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > * However, in case of unlock == 0, we still need to unlock the pages > > * (except @locked_page) to ensure all the pages are unlocked. > > */ > > - if (!unlock && orig_start < start) > > + if (!unlock && orig_start < start) { > > + if (!locked_page) > > + mapping_set_error(inode->vfs_inode.i_mapping, ret); > > extent_clear_unlock_delalloc(inode, orig_start, start - 1, > > locked_page, 0, page_ops); > > + } > > > > /* > > * For the range (2). If we reserved an extent for our delalloc range > > -- > > 2.35.1 > >
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 4e1100f84a88..cae15924fc99 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -934,8 +934,18 @@ static int submit_uncompressed_range(struct btrfs_inode *inode, goto out; } if (ret < 0) { - if (locked_page) + btrfs_cleanup_ordered_extents(inode, locked_page, start, end - start + 1); + if (locked_page) { + u64 page_start = page_offset(locked_page); + u64 page_end = page_start + PAGE_SIZE - 1; + + btrfs_page_set_error(inode->root->fs_info, locked_page, + page_start, PAGE_SIZE); + set_page_writeback(locked_page); + end_page_writeback(locked_page); + end_extent_writepage(locked_page, ret, page_start, page_end); unlock_page(locked_page); + } goto out; } @@ -1390,9 +1400,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode, * However, in case of unlock == 0, we still need to unlock the pages * (except @locked_page) to ensure all the pages are unlocked. */ - if (!unlock && orig_start < start) + if (!unlock && orig_start < start) { + if (!locked_page) + mapping_set_error(inode->vfs_inode.i_mapping, ret); extent_clear_unlock_delalloc(inode, orig_start, start - 1, locked_page, 0, page_ops); + } /* * For the range (2). If we reserved an extent for our delalloc range
When cow_file_range() fails in the middle of the allocation loop, it unlocks the pages but remains the ordered extents intact. Thus, we need to call btrfs_cleanup_ordered_extents() to finish the created ordered extents. Also, we need to call end_extent_writepage() if locked_page is available because btrfs_cleanup_ordered_extents() never process the region on the locked_page. Furthermore, we need to set the mapping as error if locked_page is unavailable before unlocking the pages, so that the errno is properly propagated to the userland. CC: stable@vger.kernel.org Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- fs/btrfs/inode.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)