Re: [PATCH] btrfs: Handle btrfs_set_extent_delalloc failure
On Wed, Dec 06, 2017 at 09:27:50AM +0200, Nikolay Borisov wrote:
> On 5.12.2017 19:50, David Sterba wrote:
> > On Tue, Dec 05, 2017 at 04:10:59PM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2017年12月05日 15:29, Nikolay Borisov wrote:
> >>> This function was introduced by 247e743cbe6e ("Btrfs: Use async helpers
> >>> to deal
> >>> with pages that have been improperly dirtied") and it didn't do any error
> >>> handling then. This function might very well fail in ENOMEM situation, yet
> >>> it's not handled, this could lead to inconsistent state. So let's handle
> >>> the
> >>> failure by setting the mapping error bit.
> >>>
> >>> Signed-off-by: Nikolay Borisov
> >>> Cc: sta...@vger.kernel.org
> >>
> >> Reviewed-by: Qu Wenruo
> >>
> >> That's the only missing one. Nice catch.
> >
> > You mean the only unhandled call of btrfs_set_extent_delalloc? There's
> > one more in relocate_file_extent_cluster.
>
> I'd prefer this call site be handled in a separate patch.
Agreed, just that Qu indicated there's no such call site :)
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: Handle btrfs_set_extent_delalloc failure
On 5.12.2017 19:50, David Sterba wrote:
> On Tue, Dec 05, 2017 at 04:10:59PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2017年12月05日 15:29, Nikolay Borisov wrote:
>>> This function was introduced by 247e743cbe6e ("Btrfs: Use async helpers to
>>> deal
>>> with pages that have been improperly dirtied") and it didn't do any error
>>> handling then. This function might very well fail in ENOMEM situation, yet
>>> it's not handled, this could lead to inconsistent state. So let's handle the
>>> failure by setting the mapping error bit.
>>>
>>> Signed-off-by: Nikolay Borisov
>>> Cc: sta...@vger.kernel.org
>>
>> Reviewed-by: Qu Wenruo
>>
>> That's the only missing one. Nice catch.
>
> You mean the only unhandled call of btrfs_set_extent_delalloc? There's
> one more in relocate_file_extent_cluster.
I'd prefer this call site be handled in a separate patch.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: Handle btrfs_set_extent_delalloc failure
On Tue, Dec 05, 2017 at 09:29:19AM +0200, Nikolay Borisov wrote:
> This function was introduced by 247e743cbe6e ("Btrfs: Use async helpers to
> deal
> with pages that have been improperly dirtied") and it didn't do any error
> handling then. This function might very well fail in ENOMEM situation, yet
> it's not handled, this could lead to inconsistent state. So let's handle the
> failure by setting the mapping error bit.
In current code the ENOMEM cannot happen (returned by __set_extent_bit),
but EEXIST could, so there's a possibility of an unhandled error.
> Signed-off-by: Nikolay Borisov
> Cc: sta...@vger.kernel.org
> ---
> fs/btrfs/inode.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 993061f83067..7a5a46fefdb4 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2098,8 +2098,15 @@ static void btrfs_writepage_fixup_worker(struct
> btrfs_work *work)
> goto out;
>}
>
> - btrfs_set_extent_delalloc(inode, page_start, page_end, 0, _state,
> - 0);
> + ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
> + _state, 0);
> + if (ret) {
> + mapping_set_error(page->mapping, ret);
> + end_extent_writepage(page, ret, page_start, page_end);
> + ClearPageChecked(page);
So this repeats the cleanup code after the preceding call to
btrfs_delalloc_reserve_space fails, I don't see a better way how to get
out of that so it's probably ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: Handle btrfs_set_extent_delalloc failure
On Tue, Dec 05, 2017 at 04:10:59PM +0800, Qu Wenruo wrote:
>
>
> On 2017年12月05日 15:29, Nikolay Borisov wrote:
> > This function was introduced by 247e743cbe6e ("Btrfs: Use async helpers to
> > deal
> > with pages that have been improperly dirtied") and it didn't do any error
> > handling then. This function might very well fail in ENOMEM situation, yet
> > it's not handled, this could lead to inconsistent state. So let's handle the
> > failure by setting the mapping error bit.
> >
> > Signed-off-by: Nikolay Borisov
> > Cc: sta...@vger.kernel.org
>
> Reviewed-by: Qu Wenruo
>
> That's the only missing one. Nice catch.
You mean the only unhandled call of btrfs_set_extent_delalloc? There's
one more in relocate_file_extent_cluster.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: Handle btrfs_set_extent_delalloc failure
On 2017年12月05日 15:29, Nikolay Borisov wrote:
> This function was introduced by 247e743cbe6e ("Btrfs: Use async helpers to
> deal
> with pages that have been improperly dirtied") and it didn't do any error
> handling then. This function might very well fail in ENOMEM situation, yet
> it's not handled, this could lead to inconsistent state. So let's handle the
> failure by setting the mapping error bit.
>
> Signed-off-by: Nikolay Borisov
> Cc: sta...@vger.kernel.org
Reviewed-by: Qu Wenruo
That's the only missing one. Nice catch.
Thanks,
Qu
> ---
> fs/btrfs/inode.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 993061f83067..7a5a46fefdb4 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2098,8 +2098,15 @@ static void btrfs_writepage_fixup_worker(struct
> btrfs_work *work)
> goto out;
>}
>
> - btrfs_set_extent_delalloc(inode, page_start, page_end, 0, _state,
> - 0);
> + ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
> + _state, 0);
> + if (ret) {
> + mapping_set_error(page->mapping, ret);
> + end_extent_writepage(page, ret, page_start, page_end);
> + ClearPageChecked(page);
> + goto out;
> + }
> +
> ClearPageChecked(page);
> set_page_dirty(page);
> btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE);
>
signature.asc
Description: OpenPGP digital signature
