[PATCH 0/3] btrfs: fix a bug and cleanups in btrfs_page_mkwrite()

[PATCH 0/3] btrfs: fix a bug and cleanups in btrfs_page_mkwrite()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

A bug fix for mmap writes and a couple cleanups. Details in the change logs.

Filipe Manana (3):
  btrfs: fix wrong start offset for delalloc space release during mmap write
  btrfs: pass true to btrfs_delalloc_release_space() at btrfs_page_mkwrite()
  btrfs: simplify early error checking in btrfs_page_mkwrite()

 fs/btrfs/file.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

-- 
2.47.2

[PATCH 1/3] btrfs: fix wrong start offset for delalloc space release during mmap write

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we're doing a mmap write against a folio that has i_size somewhere in
the middle and we have multiple sectors in the folio, we may have to
release excess space previously reserved, for the range going from the
rounded up (to sector size) i_size to the folio's end offset. We are
calculating the right amount to release and passing it to
btrfs_delalloc_release_space(), but we are passing the wrong start offset
of that range - we're passing the folio's start offset instead of the
end offset, plus 1, of the range for which we keep the reservation. This
may result in releasing more space then we should and eventually trigger
an underflow of the data space_info's bytes_may_use counter.

So fix this by passing the start offset as 'end + 1' instead of
'page_start' to btrfs_delalloc_release_space().

Fixes: d0b7da88f640 ("Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 660a73b6af90..32aad1b02b01 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1918,7 +1918,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 		if (reserved_space < fsize) {
 			end = page_start + reserved_space - 1;
 			btrfs_delalloc_release_space(BTRFS_I(inode),
-					data_reserved, page_start,
+					data_reserved, end + 1,
 					fsize - reserved_space, true);
 		}
 	}
-- 
2.47.2

[PATCH 2/3] btrfs: pass true to btrfs_delalloc_release_space() at btrfs_page_mkwrite()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

In the last call to btrfs_delalloc_release_space() where the value of the
variable 'ret' is never zero, we pass the expression 'ret != 0' as the
value for the argument 'qgroup_free', which always evaluates to true.
Make this less confusing and more clear by explicitly passing true
instead.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 32aad1b02b01..a2b1fc536fdd 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1971,7 +1971,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 out:
 	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
 	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
-				     reserved_space, (ret != 0));
+				     reserved_space, true);
 out_noreserve:
 	sb_end_pagefault(inode->i_sb);
 	extent_changeset_free(data_reserved);
-- 
2.47.2

[PATCH 3/3] btrfs: simplify early error checking in btrfs_page_mkwrite()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have this entangled error checks early at btrfs_page_mkwrite():

1) Try to reserve delalloc space by calling btrfs_delalloc_reserve_space()
   and storing the return value in the ret2 variable;

2) If the reservation succeed, call file_update_time() and store the
   return value in ret2 and also set the local variable 'reserved' to
   true (1);

3) Then do an error check on ret2 to see if any of the previous calls
   failed and if so, jump either to the 'out' label or to the
   'out_noreserve' label, depending on whether 'reserved' is true or
   not.

This is unnecessarily complex. Instead change this to a simpler and
more straighforward approach:

1) Call btrfs_delalloc_reserve_space(), if that returns an error jump to
   the 'out_noreserve' label;

2) The call file_update_time() and if that returns an error jump to the
   'out' label.

Like this there's less nested if statements, no need to use a local
variable to track if space was reserved and if statements are used only
to check errors.

Also move the call to extent_changeset_free() out of the 'out_noreserve'
label and under the 'out' label  since the changeset is allocated only if
the call to reserve delalloc space succeeded.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index a2b1fc536fdd..9ecb9f3bd057 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1843,7 +1843,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	size_t fsize = folio_size(folio);
 	vm_fault_t ret;
 	int ret2;
-	int reserved = 0;
 	u64 reserved_space;
 	u64 page_start;
 	u64 page_end;
@@ -1866,17 +1865,17 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	 */
 	ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
 					    page_start, reserved_space);
-	if (!ret2) {
-		ret2 = file_update_time(vmf->vma->vm_file);
-		reserved = 1;
-	}
 	if (ret2) {
 		ret = vmf_error(ret2);
-		if (reserved)
-			goto out;
 		goto out_noreserve;
 	}
 
+	ret2 = file_update_time(vmf->vma->vm_file);
+	if (ret2) {
+		ret = vmf_error(ret2);
+		goto out;
+	}
+
 	/* Make the VM retry the fault. */
 	ret = VM_FAULT_NOPAGE;
 again:
@@ -1972,9 +1971,9 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
 	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
 				     reserved_space, true);
+	extent_changeset_free(data_reserved);
 out_noreserve:
 	sb_end_pagefault(inode->i_sb);
-	extent_changeset_free(data_reserved);
 	return ret;
 }
 
-- 
2.47.2

Re: [PATCH 1/3] btrfs: fix wrong start offset for delalloc space release during mmap write

[Qu Wenruo]

在 2025/5/14 19:59, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we're doing a mmap write against a folio that has i_size somewhere in
> the middle and we have multiple sectors in the folio, we may have to
> release excess space previously reserved, for the range going from the
> rounded up (to sector size) i_size to the folio's end offset. We are
> calculating the right amount to release and passing it to
> btrfs_delalloc_release_space(), but we are passing the wrong start offset
> of that range - we're passing the folio's start offset instead of the
> end offset, plus 1, of the range for which we keep the reservation. This
> may result in releasing more space then we should and eventually trigger
> an underflow of the data space_info's bytes_may_use counter.
> 
> So fix this by passing the start offset as 'end + 1' instead of
> 'page_start' to btrfs_delalloc_release_space().
> 
> Fixes: d0b7da88f640 ("Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Just curious how did you spot this, by pure eyeballing or you have some 
tool exposing this?

Thanks,
Qu> ---
>   fs/btrfs/file.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 660a73b6af90..32aad1b02b01 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1918,7 +1918,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   		if (reserved_space < fsize) {
>   			end = page_start + reserved_space - 1;
>   			btrfs_delalloc_release_space(BTRFS_I(inode),
> -					data_reserved, page_start,
> +					data_reserved, end + 1,
>   					fsize - reserved_space, true);
>   		}
>   	}

Re: [PATCH 2/3] btrfs: pass true to btrfs_delalloc_release_space() at btrfs_page_mkwrite()

[Qu Wenruo]

在 2025/5/14 19:59, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In the last call to btrfs_delalloc_release_space() where the value of the
> variable 'ret' is never zero, we pass the expression 'ret != 0' as the
> value for the argument 'qgroup_free', which always evaluates to true.
> Make this less confusing and more clear by explicitly passing true
> instead.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/file.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 32aad1b02b01..a2b1fc536fdd 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1971,7 +1971,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   out:
>   	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
>   	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
> -				     reserved_space, (ret != 0));
> +				     reserved_space, true);
>   out_noreserve:
>   	sb_end_pagefault(inode->i_sb);
>   	extent_changeset_free(data_reserved);

Re: [PATCH 1/3] btrfs: fix wrong start offset for delalloc space release during mmap write

[Filipe Manana]

On Wed, May 14, 2025 at 11:38 AM Qu Wenruo <wqu@suse.com> wrote:
>
>
>
> 在 2025/5/14 19:59, fdmanana@kernel.org 写道:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > If we're doing a mmap write against a folio that has i_size somewhere in
> > the middle and we have multiple sectors in the folio, we may have to
> > release excess space previously reserved, for the range going from the
> > rounded up (to sector size) i_size to the folio's end offset. We are
> > calculating the right amount to release and passing it to
> > btrfs_delalloc_release_space(), but we are passing the wrong start offset
> > of that range - we're passing the folio's start offset instead of the
> > end offset, plus 1, of the range for which we keep the reservation. This
> > may result in releasing more space then we should and eventually trigger
> > an underflow of the data space_info's bytes_may_use counter.
> >
> > So fix this by passing the start offset as 'end + 1' instead of
> > 'page_start' to btrfs_delalloc_release_space().
> >
> > Fixes: d0b7da88f640 ("Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units")
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> Just curious how did you spot this, by pure eyeballing or you have some
> tool exposing this?

Just eyeballing while working on other incoming changes to this function.

>
> Thanks,
> Qu> ---
> >   fs/btrfs/file.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 660a73b6af90..32aad1b02b01 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1918,7 +1918,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> >               if (reserved_space < fsize) {
> >                       end = page_start + reserved_space - 1;
> >                       btrfs_delalloc_release_space(BTRFS_I(inode),
> > -                                     data_reserved, page_start,
> > +                                     data_reserved, end + 1,
> >                                       fsize - reserved_space, true);
> >               }
> >       }
>

Re: [PATCH 3/3] btrfs: simplify early error checking in btrfs_page_mkwrite()

[Qu Wenruo]

在 2025/5/14 19:59, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We have this entangled error checks early at btrfs_page_mkwrite():
> 
> 1) Try to reserve delalloc space by calling btrfs_delalloc_reserve_space()
>     and storing the return value in the ret2 variable;
> 
> 2) If the reservation succeed, call file_update_time() and store the
>     return value in ret2 and also set the local variable 'reserved' to
>     true (1);
> 
> 3) Then do an error check on ret2 to see if any of the previous calls
>     failed and if so, jump either to the 'out' label or to the
>     'out_noreserve' label, depending on whether 'reserved' is true or
>     not.
> 
> This is unnecessarily complex. Instead change this to a simpler and
> more straighforward approach:
> 
> 1) Call btrfs_delalloc_reserve_space(), if that returns an error jump to
>     the 'out_noreserve' label;
> 
> 2) The call file_update_time() and if that returns an error jump to the
>     'out' label.
> 
> Like this there's less nested if statements, no need to use a local
> variable to track if space was reserved and if statements are used only
> to check errors.
> 
> Also move the call to extent_changeset_free() out of the 'out_noreserve'
> label and under the 'out' label  since the changeset is allocated only if
> the call to reserve delalloc space succeeded.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Looks good to me, but I'm wondering can we do something better by 
completely merging @ret2 and @ret.

In fact we didn't really utilize @ret except two sites:

- page got truncated out

- btrfs_set_extent_delalloc() failure
   In that case, we should use vmf_error()

In the out_noreserve tag, do something like:

	if (ret)
		return vmf_error(ret);
	return VM_FAULT_NOPAGE;

Now we only need a single "int ret" as usual and not need to bother the 
VM_FAULT* return values at all.

Thanks,
Qu

> ---
>   fs/btrfs/file.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index a2b1fc536fdd..9ecb9f3bd057 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1843,7 +1843,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	size_t fsize = folio_size(folio);
>   	vm_fault_t ret;
>   	int ret2;
> -	int reserved = 0;
>   	u64 reserved_space;
>   	u64 page_start;
>   	u64 page_end;
> @@ -1866,17 +1865,17 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	 */
>   	ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
>   					    page_start, reserved_space);
> -	if (!ret2) {
> -		ret2 = file_update_time(vmf->vma->vm_file);
> -		reserved = 1;
> -	}
>   	if (ret2) {
>   		ret = vmf_error(ret2);
> -		if (reserved)
> -			goto out;
>   		goto out_noreserve;
>   	}
>   
> +	ret2 = file_update_time(vmf->vma->vm_file);
> +	if (ret2) {
> +		ret = vmf_error(ret2);
> +		goto out;
> +	}
> +
>   	/* Make the VM retry the fault. */
>   	ret = VM_FAULT_NOPAGE;
>   again:
> @@ -1972,9 +1971,9 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
>   	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
>   				     reserved_space, true);
> +	extent_changeset_free(data_reserved);
>   out_noreserve:
>   	sb_end_pagefault(inode->i_sb);
> -	extent_changeset_free(data_reserved);
>   	return ret;
>   }
>   

Re: [PATCH 3/3] btrfs: simplify early error checking in btrfs_page_mkwrite()

[Filipe Manana]

On Wed, May 14, 2025 at 11:51 AM Qu Wenruo <wqu@suse.com> wrote:
>
>
>
> 在 2025/5/14 19:59, fdmanana@kernel.org 写道:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > We have this entangled error checks early at btrfs_page_mkwrite():
> >
> > 1) Try to reserve delalloc space by calling btrfs_delalloc_reserve_space()
> >     and storing the return value in the ret2 variable;
> >
> > 2) If the reservation succeed, call file_update_time() and store the
> >     return value in ret2 and also set the local variable 'reserved' to
> >     true (1);
> >
> > 3) Then do an error check on ret2 to see if any of the previous calls
> >     failed and if so, jump either to the 'out' label or to the
> >     'out_noreserve' label, depending on whether 'reserved' is true or
> >     not.
> >
> > This is unnecessarily complex. Instead change this to a simpler and
> > more straighforward approach:
> >
> > 1) Call btrfs_delalloc_reserve_space(), if that returns an error jump to
> >     the 'out_noreserve' label;
> >
> > 2) The call file_update_time() and if that returns an error jump to the
> >     'out' label.
> >
> > Like this there's less nested if statements, no need to use a local
> > variable to track if space was reserved and if statements are used only
> > to check errors.
> >
> > Also move the call to extent_changeset_free() out of the 'out_noreserve'
> > label and under the 'out' label  since the changeset is allocated only if
> > the call to reserve delalloc space succeeded.
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> Looks good to me, but I'm wondering can we do something better by
> completely merging @ret2 and @ret.
>
> In fact we didn't really utilize @ret except two sites:
>
> - page got truncated out
>
> - btrfs_set_extent_delalloc() failure
>    In that case, we should use vmf_error()
>
> In the out_noreserve tag, do something like:
>
>         if (ret)
>                 return vmf_error(ret);
>         return VM_FAULT_NOPAGE;
>
> Now we only need a single "int ret" as usual and not need to bother the
> VM_FAULT* return values at all.

I considered all that, but it's not so simple, one thing is that 'ret'
is of a different type, vm_fault_t, which happens to be an unsigned
int - but I don't think it's good to make assumptions about that in
the code.

There's also the case where we set ret to VM_FAULT_SIGBUS.
I thought about getting rid of ret2 and have a single 'ret' or type
int, and then do something like:

if (ret < 0)
     return vmf_error(ret);

return ret ? ret : VM_FAULT_NOPAGE;

But then I was making assumptions that vm_fault_t values are positive,
and didn't think it's a good thing to do because it's an opaque type.

>
> Thanks,
> Qu
>
> > ---
> >   fs/btrfs/file.c | 15 +++++++--------
> >   1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index a2b1fc536fdd..9ecb9f3bd057 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1843,7 +1843,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> >       size_t fsize = folio_size(folio);
> >       vm_fault_t ret;
> >       int ret2;
> > -     int reserved = 0;
> >       u64 reserved_space;
> >       u64 page_start;
> >       u64 page_end;
> > @@ -1866,17 +1865,17 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> >        */
> >       ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
> >                                           page_start, reserved_space);
> > -     if (!ret2) {
> > -             ret2 = file_update_time(vmf->vma->vm_file);
> > -             reserved = 1;
> > -     }
> >       if (ret2) {
> >               ret = vmf_error(ret2);
> > -             if (reserved)
> > -                     goto out;
> >               goto out_noreserve;
> >       }
> >
> > +     ret2 = file_update_time(vmf->vma->vm_file);
> > +     if (ret2) {
> > +             ret = vmf_error(ret2);
> > +             goto out;
> > +     }
> > +
> >       /* Make the VM retry the fault. */
> >       ret = VM_FAULT_NOPAGE;
> >   again:
> > @@ -1972,9 +1971,9 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> >       btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
> >       btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
> >                                    reserved_space, true);
> > +     extent_changeset_free(data_reserved);
> >   out_noreserve:
> >       sb_end_pagefault(inode->i_sb);
> > -     extent_changeset_free(data_reserved);
> >       return ret;
> >   }
> >
>

Re: [PATCH 3/3] btrfs: simplify early error checking in btrfs_page_mkwrite()

[Filipe Manana]

On Wed, May 14, 2025 at 11:57 AM Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Wed, May 14, 2025 at 11:51 AM Qu Wenruo <wqu@suse.com> wrote:
> >
> >
> >
> > 在 2025/5/14 19:59, fdmanana@kernel.org 写道:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > We have this entangled error checks early at btrfs_page_mkwrite():
> > >
> > > 1) Try to reserve delalloc space by calling btrfs_delalloc_reserve_space()
> > >     and storing the return value in the ret2 variable;
> > >
> > > 2) If the reservation succeed, call file_update_time() and store the
> > >     return value in ret2 and also set the local variable 'reserved' to
> > >     true (1);
> > >
> > > 3) Then do an error check on ret2 to see if any of the previous calls
> > >     failed and if so, jump either to the 'out' label or to the
> > >     'out_noreserve' label, depending on whether 'reserved' is true or
> > >     not.
> > >
> > > This is unnecessarily complex. Instead change this to a simpler and
> > > more straighforward approach:
> > >
> > > 1) Call btrfs_delalloc_reserve_space(), if that returns an error jump to
> > >     the 'out_noreserve' label;
> > >
> > > 2) The call file_update_time() and if that returns an error jump to the
> > >     'out' label.
> > >
> > > Like this there's less nested if statements, no need to use a local
> > > variable to track if space was reserved and if statements are used only
> > > to check errors.
> > >
> > > Also move the call to extent_changeset_free() out of the 'out_noreserve'
> > > label and under the 'out' label  since the changeset is allocated only if
> > > the call to reserve delalloc space succeeded.
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> >
> > Looks good to me, but I'm wondering can we do something better by
> > completely merging @ret2 and @ret.
> >
> > In fact we didn't really utilize @ret except two sites:
> >
> > - page got truncated out
> >
> > - btrfs_set_extent_delalloc() failure
> >    In that case, we should use vmf_error()
> >
> > In the out_noreserve tag, do something like:
> >
> >         if (ret)
> >                 return vmf_error(ret);
> >         return VM_FAULT_NOPAGE;
> >
> > Now we only need a single "int ret" as usual and not need to bother the
> > VM_FAULT* return values at all.
>
> I considered all that, but it's not so simple, one thing is that 'ret'
> is of a different type, vm_fault_t, which happens to be an unsigned
> int - but I don't think it's good to make assumptions about that in
> the code.
>
> There's also the case where we set ret to VM_FAULT_SIGBUS.

Ok getting rid of this case, allows for a simple way to get rid of
having two return value variables.
Added two patches for that in v2 of the patchset.

> I thought about getting rid of ret2 and have a single 'ret' or type
> int, and then do something like:
>
> if (ret < 0)
>      return vmf_error(ret);
>
> return ret ? ret : VM_FAULT_NOPAGE;
>
> But then I was making assumptions that vm_fault_t values are positive,
> and didn't think it's a good thing to do because it's an opaque type.
>
> >
> > Thanks,
> > Qu
> >
> > > ---
> > >   fs/btrfs/file.c | 15 +++++++--------
> > >   1 file changed, 7 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > > index a2b1fc536fdd..9ecb9f3bd057 100644
> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -1843,7 +1843,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> > >       size_t fsize = folio_size(folio);
> > >       vm_fault_t ret;
> > >       int ret2;
> > > -     int reserved = 0;
> > >       u64 reserved_space;
> > >       u64 page_start;
> > >       u64 page_end;
> > > @@ -1866,17 +1865,17 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> > >        */
> > >       ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
> > >                                           page_start, reserved_space);
> > > -     if (!ret2) {
> > > -             ret2 = file_update_time(vmf->vma->vm_file);
> > > -             reserved = 1;
> > > -     }
> > >       if (ret2) {
> > >               ret = vmf_error(ret2);
> > > -             if (reserved)
> > > -                     goto out;
> > >               goto out_noreserve;
> > >       }
> > >
> > > +     ret2 = file_update_time(vmf->vma->vm_file);
> > > +     if (ret2) {
> > > +             ret = vmf_error(ret2);
> > > +             goto out;
> > > +     }
> > > +
> > >       /* Make the VM retry the fault. */
> > >       ret = VM_FAULT_NOPAGE;
> > >   again:
> > > @@ -1972,9 +1971,9 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
> > >       btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
> > >       btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
> > >                                    reserved_space, true);
> > > +     extent_changeset_free(data_reserved);
> > >   out_noreserve:
> > >       sb_end_pagefault(inode->i_sb);
> > > -     extent_changeset_free(data_reserved);
> > >       return ret;
> > >   }
> > >
> >

[PATCH v2 0/5] btrfs: fix a bug and cleanups in btrfs_page_mkwrite()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

A bug fix for mmap writes and a couple cleanups. Details in the change logs.

V2: Added two more patches (4/5 and 5/5).

Filipe Manana (5):
  btrfs: fix wrong start offset for delalloc space release during mmap write
  btrfs: pass true to btrfs_delalloc_release_space() at btrfs_page_mkwrite()
  btrfs: simplify early error checking in btrfs_page_mkwrite()
  btrfs: don't return VM_FAULT_SIGBUS on failure to set delalloc for mmap write
  btrfs: use a single variable to track return value at btrfs_page_mkwrite()

 fs/btrfs/file.c | 46 ++++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

-- 
2.47.2

[PATCH v2 1/5] btrfs: fix wrong start offset for delalloc space release during mmap write

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we're doing a mmap write against a folio that has i_size somewhere in
the middle and we have multiple sectors in the folio, we may have to
release excess space previously reserved, for the range going from the
rounded up (to sector size) i_size to the folio's end offset. We are
calculating the right amount to release and passing it to
btrfs_delalloc_release_space(), but we are passing the wrong start offset
of that range - we're passing the folio's start offset instead of the
end offset, plus 1, of the range for which we keep the reservation. This
may result in releasing more space then we should and eventually trigger
an underflow of the data space_info's bytes_may_use counter.

So fix this by passing the start offset as 'end + 1' instead of
'page_start' to btrfs_delalloc_release_space().

Fixes: d0b7da88f640 ("Btrfs: btrfs_page_mkwrite: Reserve space in sectorsized units")
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 660a73b6af90..32aad1b02b01 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1918,7 +1918,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 		if (reserved_space < fsize) {
 			end = page_start + reserved_space - 1;
 			btrfs_delalloc_release_space(BTRFS_I(inode),
-					data_reserved, page_start,
+					data_reserved, end + 1,
 					fsize - reserved_space, true);
 		}
 	}
-- 
2.47.2

[PATCH v2 2/5] btrfs: pass true to btrfs_delalloc_release_space() at btrfs_page_mkwrite()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

In the last call to btrfs_delalloc_release_space() where the value of the
variable 'ret' is never zero, we pass the expression 'ret != 0' as the
value for the argument 'qgroup_free', which always evaluates to true.
Make this less confusing and more clear by explicitly passing true
instead.

Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 32aad1b02b01..a2b1fc536fdd 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1971,7 +1971,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 out:
 	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
 	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
-				     reserved_space, (ret != 0));
+				     reserved_space, true);
 out_noreserve:
 	sb_end_pagefault(inode->i_sb);
 	extent_changeset_free(data_reserved);
-- 
2.47.2

[PATCH v2 3/5] btrfs: simplify early error checking in btrfs_page_mkwrite()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have this entangled error checks early at btrfs_page_mkwrite():

1) Try to reserve delalloc space by calling btrfs_delalloc_reserve_space()
   and storing the return value in the ret2 variable;

2) If the reservation succeed, call file_update_time() and store the
   return value in ret2 and also set the local variable 'reserved' to
   true (1);

3) Then do an error check on ret2 to see if any of the previous calls
   failed and if so, jump either to the 'out' label or to the
   'out_noreserve' label, depending on whether 'reserved' is true or
   not.

This is unnecessarily complex. Instead change this to a simpler and
more straighforward approach:

1) Call btrfs_delalloc_reserve_space(), if that returns an error jump to
   the 'out_noreserve' label;

2) The call file_update_time() and if that returns an error jump to the
   'out' label.

Like this there's less nested if statements, no need to use a local
variable to track if space was reserved and if statements are used only
to check errors.

Also move the call to extent_changeset_free() out of the 'out_noreserve'
label and under the 'out' label  since the changeset is allocated only if
the call to reserve delalloc space succeeded.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index a2b1fc536fdd..9ecb9f3bd057 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1843,7 +1843,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	size_t fsize = folio_size(folio);
 	vm_fault_t ret;
 	int ret2;
-	int reserved = 0;
 	u64 reserved_space;
 	u64 page_start;
 	u64 page_end;
@@ -1866,17 +1865,17 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	 */
 	ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
 					    page_start, reserved_space);
-	if (!ret2) {
-		ret2 = file_update_time(vmf->vma->vm_file);
-		reserved = 1;
-	}
 	if (ret2) {
 		ret = vmf_error(ret2);
-		if (reserved)
-			goto out;
 		goto out_noreserve;
 	}
 
+	ret2 = file_update_time(vmf->vma->vm_file);
+	if (ret2) {
+		ret = vmf_error(ret2);
+		goto out;
+	}
+
 	/* Make the VM retry the fault. */
 	ret = VM_FAULT_NOPAGE;
 again:
@@ -1972,9 +1971,9 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
 	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
 				     reserved_space, true);
+	extent_changeset_free(data_reserved);
 out_noreserve:
 	sb_end_pagefault(inode->i_sb);
-	extent_changeset_free(data_reserved);
 	return ret;
 }
 
-- 
2.47.2

[PATCH v2 4/5] btrfs: don't return VM_FAULT_SIGBUS on failure to set delalloc for mmap write

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If the call to btrfs_set_extent_delalloc() fails we are always returning
VM_FAULT_SIGBUS, which is odd since the error means "bad access" and the
most likely cause for btrfs_set_extent_delalloc() is -ENOMEM, which should
be translated to VM_FAULT_OOM.

Instead of returning VM_FAULT_SIGBUS return vmf_error(ret2), which gives
us a more appropriate return value, and we use that everywhere else too.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 9ecb9f3bd057..f6b32f24185c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1937,7 +1937,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 					&cached_state);
 	if (ret2) {
 		btrfs_unlock_extent(io_tree, page_start, page_end, &cached_state);
-		ret = VM_FAULT_SIGBUS;
+		ret = vmf_error(ret2);
 		goto out_unlock;
 	}
 
-- 
2.47.2

[PATCH v2 5/5] btrfs: use a single variable to track return value at btrfs_page_mkwrite()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have two variables to track return values, ret and ret2, with types
vm_fault_t (an unsigned int type) and int, which makes it a bit confusing
and harder to keep track. So use a single variable, of type int, and under
the 'out' label return vmf_error(ret) in case ret contains an error,
otherwise return VM_FAULT_NOPAGE. This is equivalent to what we had before
and it's simpler.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index f6b32f24185c..8ce6f45f45e0 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1841,8 +1841,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	unsigned long zero_start;
 	loff_t size;
 	size_t fsize = folio_size(folio);
-	vm_fault_t ret;
-	int ret2;
+	int ret;
 	u64 reserved_space;
 	u64 page_start;
 	u64 page_end;
@@ -1863,21 +1862,14 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	 * end up waiting indefinitely to get a lock on the page currently
 	 * being processed by btrfs_page_mkwrite() function.
 	 */
-	ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
-					    page_start, reserved_space);
-	if (ret2) {
-		ret = vmf_error(ret2);
+	ret = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
+					   page_start, reserved_space);
+	if (ret < 0)
 		goto out_noreserve;
-	}
 
-	ret2 = file_update_time(vmf->vma->vm_file);
-	if (ret2) {
-		ret = vmf_error(ret2);
+	ret = file_update_time(vmf->vma->vm_file);
+	if (ret < 0)
 		goto out;
-	}
-
-	/* Make the VM retry the fault. */
-	ret = VM_FAULT_NOPAGE;
 again:
 	down_read(&BTRFS_I(inode)->i_mmap_lock);
 	folio_lock(folio);
@@ -1891,9 +1883,8 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	folio_wait_writeback(folio);
 
 	btrfs_lock_extent(io_tree, page_start, page_end, &cached_state);
-	ret2 = set_folio_extent_mapped(folio);
-	if (ret2 < 0) {
-		ret = vmf_error(ret2);
+	ret = set_folio_extent_mapped(folio);
+	if (ret < 0) {
 		btrfs_unlock_extent(io_tree, page_start, page_end, &cached_state);
 		goto out_unlock;
 	}
@@ -1933,11 +1924,10 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 			       EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING |
 			       EXTENT_DEFRAG, &cached_state);
 
-	ret2 = btrfs_set_extent_delalloc(BTRFS_I(inode), page_start, end, 0,
+	ret = btrfs_set_extent_delalloc(BTRFS_I(inode), page_start, end, 0,
 					&cached_state);
-	if (ret2) {
+	if (ret < 0) {
 		btrfs_unlock_extent(io_tree, page_start, page_end, &cached_state);
-		ret = vmf_error(ret2);
 		goto out_unlock;
 	}
 
@@ -1974,7 +1964,12 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	extent_changeset_free(data_reserved);
 out_noreserve:
 	sb_end_pagefault(inode->i_sb);
-	return ret;
+
+	if (ret < 0)
+		return vmf_error(ret);
+
+	/* Make the VM retry the fault. */
+	return VM_FAULT_NOPAGE;
 }
 
 static const struct vm_operations_struct btrfs_file_vm_ops = {
-- 
2.47.2

Re: [PATCH v2 3/5] btrfs: simplify early error checking in btrfs_page_mkwrite()

[Qu Wenruo]

在 2025/5/14 21:08, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We have this entangled error checks early at btrfs_page_mkwrite():
> 
> 1) Try to reserve delalloc space by calling btrfs_delalloc_reserve_space()
>     and storing the return value in the ret2 variable;
> 
> 2) If the reservation succeed, call file_update_time() and store the
>     return value in ret2 and also set the local variable 'reserved' to
>     true (1);
> 
> 3) Then do an error check on ret2 to see if any of the previous calls
>     failed and if so, jump either to the 'out' label or to the
>     'out_noreserve' label, depending on whether 'reserved' is true or
>     not.
> 
> This is unnecessarily complex. Instead change this to a simpler and
> more straighforward approach:
> 
> 1) Call btrfs_delalloc_reserve_space(), if that returns an error jump to
>     the 'out_noreserve' label;
> 
> 2) The call file_update_time() and if that returns an error jump to the
>     'out' label.
> 
> Like this there's less nested if statements, no need to use a local
> variable to track if space was reserved and if statements are used only
> to check errors.
> 
> Also move the call to extent_changeset_free() out of the 'out_noreserve'
> label and under the 'out' label  since the changeset is allocated only if
> the call to reserve delalloc space succeeded.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu> ---
>   fs/btrfs/file.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index a2b1fc536fdd..9ecb9f3bd057 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1843,7 +1843,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	size_t fsize = folio_size(folio);
>   	vm_fault_t ret;
>   	int ret2;
> -	int reserved = 0;
>   	u64 reserved_space;
>   	u64 page_start;
>   	u64 page_end;
> @@ -1866,17 +1865,17 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	 */
>   	ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
>   					    page_start, reserved_space);
> -	if (!ret2) {
> -		ret2 = file_update_time(vmf->vma->vm_file);
> -		reserved = 1;
> -	}
>   	if (ret2) {
>   		ret = vmf_error(ret2);
> -		if (reserved)
> -			goto out;
>   		goto out_noreserve;
>   	}
>   
> +	ret2 = file_update_time(vmf->vma->vm_file);
> +	if (ret2) {
> +		ret = vmf_error(ret2);
> +		goto out;
> +	}
> +
>   	/* Make the VM retry the fault. */
>   	ret = VM_FAULT_NOPAGE;
>   again:
> @@ -1972,9 +1971,9 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	btrfs_delalloc_release_extents(BTRFS_I(inode), fsize);
>   	btrfs_delalloc_release_space(BTRFS_I(inode), data_reserved, page_start,
>   				     reserved_space, true);
> +	extent_changeset_free(data_reserved);
>   out_noreserve:
>   	sb_end_pagefault(inode->i_sb);
> -	extent_changeset_free(data_reserved);
>   	return ret;
>   }
>   

Re: [PATCH v2 4/5] btrfs: don't return VM_FAULT_SIGBUS on failure to set delalloc for mmap write

[Qu Wenruo]

在 2025/5/14 21:08, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If the call to btrfs_set_extent_delalloc() fails we are always returning
> VM_FAULT_SIGBUS, which is odd since the error means "bad access" and the
> most likely cause for btrfs_set_extent_delalloc() is -ENOMEM, which should
> be translated to VM_FAULT_OOM.
> 
> Instead of returning VM_FAULT_SIGBUS return vmf_error(ret2), which gives
> us a more appropriate return value, and we use that everywhere else too.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/file.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 9ecb9f3bd057..f6b32f24185c 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1937,7 +1937,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   					&cached_state);
>   	if (ret2) {
>   		btrfs_unlock_extent(io_tree, page_start, page_end, &cached_state);
> -		ret = VM_FAULT_SIGBUS;
> +		ret = vmf_error(ret2);
>   		goto out_unlock;
>   	}
>   

Re: [PATCH v2 5/5] btrfs: use a single variable to track return value at btrfs_page_mkwrite()

[Qu Wenruo]

在 2025/5/14 21:08, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We have two variables to track return values, ret and ret2, with types
> vm_fault_t (an unsigned int type) and int, which makes it a bit confusing
> and harder to keep track. So use a single variable, of type int, and under
> the 'out' label return vmf_error(ret) in case ret contains an error,
> otherwise return VM_FAULT_NOPAGE. This is equivalent to what we had before
> and it's simpler.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu


> ---
>   fs/btrfs/file.c | 37 ++++++++++++++++---------------------
>   1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index f6b32f24185c..8ce6f45f45e0 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1841,8 +1841,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	unsigned long zero_start;
>   	loff_t size;
>   	size_t fsize = folio_size(folio);
> -	vm_fault_t ret;
> -	int ret2;
> +	int ret;
>   	u64 reserved_space;
>   	u64 page_start;
>   	u64 page_end;
> @@ -1863,21 +1862,14 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	 * end up waiting indefinitely to get a lock on the page currently
>   	 * being processed by btrfs_page_mkwrite() function.
>   	 */
> -	ret2 = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
> -					    page_start, reserved_space);
> -	if (ret2) {
> -		ret = vmf_error(ret2);
> +	ret = btrfs_delalloc_reserve_space(BTRFS_I(inode), &data_reserved,
> +					   page_start, reserved_space);
> +	if (ret < 0)
>   		goto out_noreserve;
> -	}
>   
> -	ret2 = file_update_time(vmf->vma->vm_file);
> -	if (ret2) {
> -		ret = vmf_error(ret2);
> +	ret = file_update_time(vmf->vma->vm_file);
> +	if (ret < 0)
>   		goto out;
> -	}
> -
> -	/* Make the VM retry the fault. */
> -	ret = VM_FAULT_NOPAGE;
>   again:
>   	down_read(&BTRFS_I(inode)->i_mmap_lock);
>   	folio_lock(folio);
> @@ -1891,9 +1883,8 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	folio_wait_writeback(folio);
>   
>   	btrfs_lock_extent(io_tree, page_start, page_end, &cached_state);
> -	ret2 = set_folio_extent_mapped(folio);
> -	if (ret2 < 0) {
> -		ret = vmf_error(ret2);
> +	ret = set_folio_extent_mapped(folio);
> +	if (ret < 0) {
>   		btrfs_unlock_extent(io_tree, page_start, page_end, &cached_state);
>   		goto out_unlock;
>   	}
> @@ -1933,11 +1924,10 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   			       EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING |
>   			       EXTENT_DEFRAG, &cached_state);
>   
> -	ret2 = btrfs_set_extent_delalloc(BTRFS_I(inode), page_start, end, 0,
> +	ret = btrfs_set_extent_delalloc(BTRFS_I(inode), page_start, end, 0,
>   					&cached_state);
> -	if (ret2) {
> +	if (ret < 0) {
>   		btrfs_unlock_extent(io_tree, page_start, page_end, &cached_state);
> -		ret = vmf_error(ret2);
>   		goto out_unlock;
>   	}
>   
> @@ -1974,7 +1964,12 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>   	extent_changeset_free(data_reserved);
>   out_noreserve:
>   	sb_end_pagefault(inode->i_sb);
> -	return ret;
> +
> +	if (ret < 0)
> +		return vmf_error(ret);
> +
> +	/* Make the VM retry the fault. */
> +	return VM_FAULT_NOPAGE;
>   }
>   
>   static const struct vm_operations_struct btrfs_file_vm_ops = {

Re: [PATCH v2 0/5] btrfs: fix a bug and cleanups in btrfs_page_mkwrite()

[David Sterba]

On Wed, May 14, 2025 at 12:38:32PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> A bug fix for mmap writes and a couple cleanups. Details in the change logs.
> 
> V2: Added two more patches (4/5 and 5/5).
> 
> Filipe Manana (5):
>   btrfs: fix wrong start offset for delalloc space release during mmap write
>   btrfs: pass true to btrfs_delalloc_release_space() at btrfs_page_mkwrite()
>   btrfs: simplify early error checking in btrfs_page_mkwrite()
>   btrfs: don't return VM_FAULT_SIGBUS on failure to set delalloc for mmap write
>   btrfs: use a single variable to track return value at btrfs_page_mkwrite()

Reviewed-by: David Sterba <dsterba@suse.com>