[PATCH] Btrfs: fix regression in btrfs_page_mkwrite() from vm_fault_t conversion

[PATCH] Btrfs: fix regression in btrfs_page_mkwrite() from vm_fault_t conversion

[Chris Mason]

The vm_fault_t conversion commit introduced a ret2 variable for
tracking the integer return values from internal btrfs functions.  It
was sometimes returning VM_FAULT_LOCKED for pages that were actually
invalid and had been removed from the radix.  Something like this:

	ret2 = btrfs_delalloc_reserve_space() // returns zero on success

	lock_page(page)
	if (page->mapping != inode->i_mapping)
	    goto out_unlock;

...

out_unlock:
	if (!ret2) {
		...
		return VM_FAULT_LOCKED;
	}

This ends up triggering this WARNING in btrfs_destroy_inode()
	WARN_ON(BTRFS_I(inode)->block_rsv.size);

The fix used here is to use ret instead of ret2 in our check for success.

Fixes: a528a2415087 (btrfs: change return type of btrfs_page_mkwrite to vm_fault_t)
Signed-off-by: Chris Mason <clm@fb.com>
---
 fs/btrfs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 193f933..38403aa 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9013,6 +9013,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 		goto out_unlock;
 	}
 	ret2 = 0;
+	ret = 0;
 
 	/* page is wholly or partially inside EOF */
 	if (page_start + PAGE_SIZE > size)
@@ -9037,7 +9038,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	unlock_extent_cached(io_tree, page_start, page_end, &cached_state);
 
 out_unlock:
-	if (!ret2) {
+	if (!ret) {
 		btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true);
 		sb_end_pagefault(inode->i_sb);
 		extent_changeset_free(data_reserved);
-- 
2.9.5

Re: [PATCH] Btrfs: fix regression in btrfs_page_mkwrite() from vm_fault_t conversion

[David Sterba]

On Mon, Jun 25, 2018 at 06:45:32AM -0700, Chris Mason wrote:
> The vm_fault_t conversion commit introduced a ret2 variable for
> tracking the integer return values from internal btrfs functions.  It
> was sometimes returning VM_FAULT_LOCKED for pages that were actually
> invalid and had been removed from the radix.  Something like this:
> 
> 	ret2 = btrfs_delalloc_reserve_space() // returns zero on success
> 
> 	lock_page(page)
> 	if (page->mapping != inode->i_mapping)
> 	    goto out_unlock;
> 
> ...
> 
> out_unlock:
> 	if (!ret2) {
> 		...
> 		return VM_FAULT_LOCKED;
> 	}
> 
> This ends up triggering this WARNING in btrfs_destroy_inode()
> 	WARN_ON(BTRFS_I(inode)->block_rsv.size);
> 
> The fix used here is to use ret instead of ret2 in our check for success.
> 
> Fixes: a528a2415087 (btrfs: change return type of btrfs_page_mkwrite to vm_fault_t)
> Signed-off-by: Chris Mason <clm@fb.com>
> ---
>  fs/btrfs/inode.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 193f933..38403aa 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9013,6 +9013,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>  		goto out_unlock;
>  	}
>  	ret2 = 0;
> +	ret = 0;

That's something I'd rather avoid, now there are two error values that
need to be tracked. Shouldn't the 'ret2 = 0' be removed completely?

>  
>  	/* page is wholly or partially inside EOF */
>  	if (page_start + PAGE_SIZE > size)
> @@ -9037,7 +9038,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>  	unlock_extent_cached(io_tree, page_start, page_end, &cached_state);
>  
>  out_unlock:
> -	if (!ret2) {
> +	if (!ret) {

I'm not sure is safe, the new ret is one of the VM_FAULT_ values and
it's not 0 by default and in fact I don't see anything that would set it
to 0 directly or indirectly. Which means that the code in the out: label
also needs to be revisited:

9016 out:
9017         btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, (ret != 0));
9018         btrfs_delalloc_release_space(inode, data_reserved, page_start,
9019                                      reserved_space, (ret != 0));

>  		btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true);
>  		sb_end_pagefault(inode->i_sb);
>  		extent_changeset_free(data_reserved);

Re: [PATCH] Btrfs: fix regression in btrfs_page_mkwrite() from vm_fault_t conversion

[Chris Mason]

On 25 Jun 2018, at 9:54, David Sterba wrote:

> On Mon, Jun 25, 2018 at 06:45:32AM -0700, Chris Mason wrote:

>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 193f933..38403aa 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -9013,6 +9013,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>>  		goto out_unlock;
>>  	}
>>  	ret2 = 0;
>> +	ret = 0;
>
> That's something I'd rather avoid, now there are two error values that
> need to be tracked. Shouldn't the 'ret2 = 0' be removed completely?


Yeah, the whole thing hurts my eyes.  Fixing.

>
>>
>>  	/* page is wholly or partially inside EOF */
>>  	if (page_start + PAGE_SIZE > size)
>> @@ -9037,7 +9038,7 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>>  	unlock_extent_cached(io_tree, page_start, page_end, &cached_state);
>>
>>  out_unlock:
>> -	if (!ret2) {
>> +	if (!ret) {
>
> I'm not sure is safe, the new ret is one of the VM_FAULT_ values and
> it's not 0 by default and in fact I don't see anything that would set it
> to 0 directly or indirectly. Which means that the code in the out: label
> also needs to be revisited:

Good point, ok.

-chris