[PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---

Based on off-list feadback from Josef I'm also clearing the EXTENT_BOUNDARY 
flag so we don't leak it. Otherwise it's okay. 

He said to add his Reviewed-by as long as the extent clearing bit is adjusted. 
 fs/btrfs/relocation.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f0c3f00e97cb..8b2a31cef5cf 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3268,12 +3268,25 @@ static int relocate_file_extent_cluster(struct inode *inode,
 			nr++;
 		}
 
-		btrfs_set_extent_delalloc(inode, page_start, page_end, 0, NULL,
-					  0);
+		ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
+						NULL, 0);
+		if (ret) {
+			unlock_page(page);
+			put_page(page);
+			btrfs_delalloc_release_metadata(BTRFS_I(inode),
+							 PAGE_SIZE);
+			btrfs_delalloc_release_extents(BTRFS_I(inode),
+			                               PAGE_SIZE);
+
+			clear_extent_bits(&BTRFS_I(inode)->io_tree,
+					  page_start, page_end,
+					  EXTENT_LOCKED | EXTENT_BOUNDARY);
+			goto out;
+
+		}
 		set_page_dirty(page);
 
-		unlock_extent(&BTRFS_I(inode)->io_tree,
-			      page_start, page_end);
+		unlock_extent(&BTRFS_I(inode)->io_tree, page_start, page_end);
 		unlock_page(page);
 		put_page(page);
 
-- 
2.7.4

[RESEND PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[Nikolay Borisov]

Essentially duplicate the error handling from the above block which
handles the !PageUptodate(page) case and additionally clear
EXTENT_BOUNDARY.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---

Put description of the intended changes. 

 fs/btrfs/relocation.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f0c3f00e97cb..8b2a31cef5cf 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3268,12 +3268,25 @@ static int relocate_file_extent_cluster(struct inode *inode,
 			nr++;
 		}
 
-		btrfs_set_extent_delalloc(inode, page_start, page_end, 0, NULL,
-					  0);
+		ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
+						NULL, 0);
+		if (ret) {
+			unlock_page(page);
+			put_page(page);
+			btrfs_delalloc_release_metadata(BTRFS_I(inode),
+							 PAGE_SIZE);
+			btrfs_delalloc_release_extents(BTRFS_I(inode),
+			                               PAGE_SIZE);
+
+			clear_extent_bits(&BTRFS_I(inode)->io_tree,
+					  page_start, page_end,
+					  EXTENT_LOCKED | EXTENT_BOUNDARY);
+			goto out;
+
+		}
 		set_page_dirty(page);
 
-		unlock_extent(&BTRFS_I(inode)->io_tree,
-			      page_start, page_end);
+		unlock_extent(&BTRFS_I(inode)->io_tree, page_start, page_end);
 		unlock_page(page);
 		put_page(page);
 
-- 
2.7.4

Re: [RESEND PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[Filipe Manana]

On Tue, Jan 30, 2018 at 2:32 PM, Nikolay Borisov <nborisov@suse.com> wrote:
> Essentially duplicate the error handling from the above block which
> handles the !PageUptodate(page) case and additionally clear
> EXTENT_BOUNDARY.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Reviewed-by: Josef Bacik <jbacik@fb.com>
> ---
>
> Put description of the intended changes.
>
>  fs/btrfs/relocation.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index f0c3f00e97cb..8b2a31cef5cf 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -3268,12 +3268,25 @@ static int relocate_file_extent_cluster(struct inode *inode,
>                         nr++;
>                 }
>
> -               btrfs_set_extent_delalloc(inode, page_start, page_end, 0, NULL,
> -                                         0);
> +               ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
> +                                               NULL, 0);
> +               if (ret) {
> +                       unlock_page(page);
> +                       put_page(page);
> +                       btrfs_delalloc_release_metadata(BTRFS_I(inode),
> +                                                        PAGE_SIZE);
> +                       btrfs_delalloc_release_extents(BTRFS_I(inode),
> +                                                      PAGE_SIZE);
> +
> +                       clear_extent_bits(&BTRFS_I(inode)->io_tree,
> +                                         page_start, page_end,
> +                                         EXTENT_LOCKED | EXTENT_BOUNDARY);
> +                       goto out;
> +
> +               }
>                 set_page_dirty(page);
>
> -               unlock_extent(&BTRFS_I(inode)->io_tree,
> -                             page_start, page_end);
> +               unlock_extent(&BTRFS_I(inode)->io_tree, page_start, page_end);

This is an unrelated change. Please don't mix pure white
space/indentation changes with functional changes.

>                 unlock_page(page);
>                 put_page(page);
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [RESEND PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[Nikolay Borisov]

On 31.01.2018 12:49, Filipe Manana wrote:
> On Tue, Jan 30, 2018 at 2:32 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>> Essentially duplicate the error handling from the above block which
>> handles the !PageUptodate(page) case and additionally clear
>> EXTENT_BOUNDARY.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> Reviewed-by: Josef Bacik <jbacik@fb.com>
>> ---
>>
>> Put description of the intended changes.
>>
>>  fs/btrfs/relocation.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index f0c3f00e97cb..8b2a31cef5cf 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -3268,12 +3268,25 @@ static int relocate_file_extent_cluster(struct inode *inode,
>>                         nr++;
>>                 }
>>
>> -               btrfs_set_extent_delalloc(inode, page_start, page_end, 0, NULL,
>> -                                         0);
>> +               ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
>> +                                               NULL, 0);
>> +               if (ret) {
>> +                       unlock_page(page);
>> +                       put_page(page);
>> +                       btrfs_delalloc_release_metadata(BTRFS_I(inode),
>> +                                                        PAGE_SIZE);
>> +                       btrfs_delalloc_release_extents(BTRFS_I(inode),
>> +                                                      PAGE_SIZE);
>> +
>> +                       clear_extent_bits(&BTRFS_I(inode)->io_tree,
>> +                                         page_start, page_end,
>> +                                         EXTENT_LOCKED | EXTENT_BOUNDARY);
>> +                       goto out;
>> +
>> +               }
>>                 set_page_dirty(page);
>>
>> -               unlock_extent(&BTRFS_I(inode)->io_tree,
>> -                             page_start, page_end);
>> +               unlock_extent(&BTRFS_I(inode)->io_tree, page_start, page_end);
> 
> This is an unrelated change. Please don't mix pure white
> space/indentation changes with functional changes.

David seems rather adamant in not accepting pure whitespace/indention
changes on their own so I don't see a way to actually improve the code
base in that regard unless i slip them up when modifying nearby code.
There are a couple of space with trailing whitespace which I constantly
select out from my commits.

Given that you have now also expressed objection to such cleanups, how
should they eventually be fixed?

> 
>>                 unlock_page(page);
>>                 put_page(page);
>>
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

Re: [RESEND PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[Filipe Manana]

On Wed, Jan 31, 2018 at 10:53 AM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 31.01.2018 12:49, Filipe Manana wrote:
>> On Tue, Jan 30, 2018 at 2:32 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>> Essentially duplicate the error handling from the above block which
>>> handles the !PageUptodate(page) case and additionally clear
>>> EXTENT_BOUNDARY.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> Reviewed-by: Josef Bacik <jbacik@fb.com>
>>> ---
>>>
>>> Put description of the intended changes.
>>>
>>>  fs/btrfs/relocation.c | 21 +++++++++++++++++----
>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>>> index f0c3f00e97cb..8b2a31cef5cf 100644
>>> --- a/fs/btrfs/relocation.c
>>> +++ b/fs/btrfs/relocation.c
>>> @@ -3268,12 +3268,25 @@ static int relocate_file_extent_cluster(struct inode *inode,
>>>                         nr++;
>>>                 }
>>>
>>> -               btrfs_set_extent_delalloc(inode, page_start, page_end, 0, NULL,
>>> -                                         0);
>>> +               ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
>>> +                                               NULL, 0);
>>> +               if (ret) {
>>> +                       unlock_page(page);
>>> +                       put_page(page);
>>> +                       btrfs_delalloc_release_metadata(BTRFS_I(inode),
>>> +                                                        PAGE_SIZE);
>>> +                       btrfs_delalloc_release_extents(BTRFS_I(inode),
>>> +                                                      PAGE_SIZE);
>>> +
>>> +                       clear_extent_bits(&BTRFS_I(inode)->io_tree,
>>> +                                         page_start, page_end,
>>> +                                         EXTENT_LOCKED | EXTENT_BOUNDARY);
>>> +                       goto out;
>>> +
>>> +               }
>>>                 set_page_dirty(page);
>>>
>>> -               unlock_extent(&BTRFS_I(inode)->io_tree,
>>> -                             page_start, page_end);
>>> +               unlock_extent(&BTRFS_I(inode)->io_tree, page_start, page_end);
>>
>> This is an unrelated change. Please don't mix pure white
>> space/indentation changes with functional changes.
>
> David seems rather adamant in not accepting pure whitespace/indention

Well, for every project I worked on, either open source or not this
was always frowned upon.
And it's quite obvious why...

> changes on their own so I don't see a way to actually improve the code
> base in that regard unless i slip them up when modifying nearby code.
> There are a couple of space with trailing whitespace which I constantly
> select out from my commits.
>
> Given that you have now also expressed objection to such cleanups, how
> should they eventually be fixed?

You "fix" them by doing a functional change affecting that code.
If you know it's frowned upon, please don't keep sneaking this sort of changes,

>
>>
>>>                 unlock_page(page);
>>>                 put_page(page);
>>>
>>> --
>>> 2.7.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [RESEND PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[David Sterba]

On Wed, Jan 31, 2018 at 12:53:38PM +0200, Nikolay Borisov wrote:
> > This is an unrelated change. Please don't mix pure white
> > space/indentation changes with functional changes.
> 
> David seems rather adamant in not accepting pure whitespace/indention
> changes on their own so I don't see a way to actually improve the code
> base in that regard unless i slip them up when modifying nearby code.

If it's an unrelated change, than it's wrong to slip it in just because
it's near. It's still an unrelated change. Other than that, and I think
I mentioned that in your previous attempts to add whitespace changes,
it just pollutes the commit history. Looking for a commit that
potentially broke some code and finding a whitespace change just makes
anybody grumpy. Maintaner should know and not let such changes in. I've
been on both sides, and based on this experience I will not let in such
changes.

The review process in the mailinglist is there to catch that and point
out, though pointing out just whitespace is kind of not welcome, unless
the real review is also done. If there are minor things to fix, I do
that at commit time, which means I edit majority of all patches or
changelogs. In some cases I will let the patch author know so I don't
have to fix that over and over again. (But it never lasts.)

> There are a couple of space with trailing whitespace which I constantly
> select out from my commits.
> 
> Given that you have now also expressed objection to such cleanups, how
> should they eventually be fixed?

They will be fixed once the code on those lines gets changed. Which may
not anytime soon.

[PATCH v2] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[Nikolay Borisov]

Essentially duplicate the error handling from the above block which
handles the !PageUptodate(page) case and additionally clear
EXTENT_BOUNDARY.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---

V2: 
 * Remove unrelated whitespace fix 

 fs/btrfs/relocation.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f0c3f00e97cb..cd2298d185dd 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3268,8 +3268,22 @@ static int relocate_file_extent_cluster(struct inode *inode,
 			nr++;
 		}
 
-		btrfs_set_extent_delalloc(inode, page_start, page_end, 0, NULL,
-					  0);
+		ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
+						NULL, 0);
+		if (ret) {
+			unlock_page(page);
+			put_page(page);
+			btrfs_delalloc_release_metadata(BTRFS_I(inode),
+							 PAGE_SIZE);
+			btrfs_delalloc_release_extents(BTRFS_I(inode),
+			                               PAGE_SIZE);
+
+			clear_extent_bits(&BTRFS_I(inode)->io_tree,
+					  page_start, page_end,
+					  EXTENT_LOCKED | EXTENT_BOUNDARY);
+			goto out;
+
+		}
 		set_page_dirty(page);
 
 		unlock_extent(&BTRFS_I(inode)->io_tree,
-- 
2.7.4

Re: [PATCH v2] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[David Sterba]

On Wed, Jan 31, 2018 at 05:14:02PM +0200, Nikolay Borisov wrote:
> Essentially duplicate the error handling from the above block which
> handles the !PageUptodate(page) case and additionally clear
> EXTENT_BOUNDARY.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Reviewed-by: Josef Bacik <jbacik@fb.com>

Added to next, thanks.