* [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve
@ 2019-03-26 3:56 robbieko
2019-03-26 10:54 ` Filipe Manana
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: robbieko @ 2019-03-26 3:56 UTC (permalink / raw)
To: linux-btrfs; +Cc: Robbie Ko
From: Robbie Ko <robbieko@synology.com>
When doing fallocate, we first add the range to the reserve_list
and then reserve the quota.
If quota reservation fails, we'll release all reserved parts of
reserve_list.
However, cur_offset is not updated to indicate that this
range is already been inserted into the list.
Therefore, the same range is freed twice.
Once at list_for_each_entry loop, and once at the end of the
function.
This will result in WARN_ON on bytes_may_use when we free the
remaining space.
At the end, under the 'out' label we have a call to:
btrfs_free_reserved_data_space(inode, data_reserved, alloc_start,
alloc_end - cur_offset);
The start offset, third argument, should be cur_offset.
Everything from alloc_start to cur_offset was freed by the
list_for_each_entry_safe_loop.
Fixes: 18513091af94 ("btrfs: update btrfs_space_info's bytes_may_use timely")
Signed-off-by: Robbie Ko <robbieko@synology.com>
---
fs/btrfs/file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 34fe8a5..0832449 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3132,6 +3132,7 @@ static long btrfs_fallocate(struct file *file, int mode,
ret = btrfs_qgroup_reserve_data(inode, &data_reserved,
cur_offset, last_byte - cur_offset);
if (ret < 0) {
+ cur_offset = last_byte;
free_extent_map(em);
break;
}
@@ -3181,7 +3182,7 @@ static long btrfs_fallocate(struct file *file, int mode,
/* Let go of our reservation. */
if (ret != 0 && !(mode & FALLOC_FL_ZERO_RANGE))
btrfs_free_reserved_data_space(inode, data_reserved,
- alloc_start, alloc_end - cur_offset);
+ cur_offset, alloc_end - cur_offset);
extent_changeset_free(data_reserved);
return ret;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve
2019-03-26 3:56 [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve robbieko
@ 2019-03-26 10:54 ` Filipe Manana
2019-04-09 0:10 ` David Sterba
2019-04-11 17:52 ` David Sterba
2 siblings, 0 replies; 6+ messages in thread
From: Filipe Manana @ 2019-03-26 10:54 UTC (permalink / raw)
To: robbieko; +Cc: linux-btrfs
On Tue, Mar 26, 2019 at 3:57 AM robbieko <robbieko@synology.com> wrote:
>
> From: Robbie Ko <robbieko@synology.com>
>
> When doing fallocate, we first add the range to the reserve_list
> and then reserve the quota.
> If quota reservation fails, we'll release all reserved parts of
> reserve_list.
> However, cur_offset is not updated to indicate that this
> range is already been inserted into the list.
> Therefore, the same range is freed twice.
> Once at list_for_each_entry loop, and once at the end of the
> function.
> This will result in WARN_ON on bytes_may_use when we free the
> remaining space.
>
> At the end, under the 'out' label we have a call to:
> btrfs_free_reserved_data_space(inode, data_reserved, alloc_start,
> alloc_end - cur_offset);
> The start offset, third argument, should be cur_offset.
> Everything from alloc_start to cur_offset was freed by the
> list_for_each_entry_safe_loop.
>
> Fixes: 18513091af94 ("btrfs: update btrfs_space_info's bytes_may_use timely")
> Signed-off-by: Robbie Ko <robbieko@synology.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Now it looks good, thanks.
> ---
> fs/btrfs/file.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 34fe8a5..0832449 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3132,6 +3132,7 @@ static long btrfs_fallocate(struct file *file, int mode,
> ret = btrfs_qgroup_reserve_data(inode, &data_reserved,
> cur_offset, last_byte - cur_offset);
> if (ret < 0) {
> + cur_offset = last_byte;
> free_extent_map(em);
> break;
> }
> @@ -3181,7 +3182,7 @@ static long btrfs_fallocate(struct file *file, int mode,
> /* Let go of our reservation. */
> if (ret != 0 && !(mode & FALLOC_FL_ZERO_RANGE))
> btrfs_free_reserved_data_space(inode, data_reserved,
> - alloc_start, alloc_end - cur_offset);
> + cur_offset, alloc_end - cur_offset);
> extent_changeset_free(data_reserved);
> return ret;
> }
> --
> 1.9.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve
2019-03-26 3:56 [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve robbieko
2019-03-26 10:54 ` Filipe Manana
@ 2019-04-09 0:10 ` David Sterba
2019-04-11 17:52 ` David Sterba
2 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2019-04-09 0:10 UTC (permalink / raw)
To: robbieko; +Cc: linux-btrfs
On Tue, Mar 26, 2019 at 11:56:11AM +0800, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
>
> When doing fallocate, we first add the range to the reserve_list
> and then reserve the quota.
> If quota reservation fails, we'll release all reserved parts of
> reserve_list.
> However, cur_offset is not updated to indicate that this
> range is already been inserted into the list.
> Therefore, the same range is freed twice.
> Once at list_for_each_entry loop, and once at the end of the
> function.
> This will result in WARN_ON on bytes_may_use when we free the
> remaining space.
>
> At the end, under the 'out' label we have a call to:
> btrfs_free_reserved_data_space(inode, data_reserved, alloc_start,
> alloc_end - cur_offset);
> The start offset, third argument, should be cur_offset.
> Everything from alloc_start to cur_offset was freed by the
> list_for_each_entry_safe_loop.
>
> Fixes: 18513091af94 ("btrfs: update btrfs_space_info's bytes_may_use timely")
> Signed-off-by: Robbie Ko <robbieko@synology.com>
Added to misc-next, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve
2019-03-26 3:56 [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve robbieko
2019-03-26 10:54 ` Filipe Manana
2019-04-09 0:10 ` David Sterba
@ 2019-04-11 17:52 ` David Sterba
2019-04-11 18:01 ` Filipe Manana
2 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2019-04-11 17:52 UTC (permalink / raw)
To: robbieko; +Cc: linux-btrfs, fdmanana
On Tue, Mar 26, 2019 at 11:56:11AM +0800, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
>
> When doing fallocate, we first add the range to the reserve_list
> and then reserve the quota.
> If quota reservation fails, we'll release all reserved parts of
> reserve_list.
> However, cur_offset is not updated to indicate that this
> range is already been inserted into the list.
> Therefore, the same range is freed twice.
> Once at list_for_each_entry loop, and once at the end of the
> function.
> This will result in WARN_ON on bytes_may_use when we free the
> remaining space.
>
> At the end, under the 'out' label we have a call to:
> btrfs_free_reserved_data_space(inode, data_reserved, alloc_start,
> alloc_end - cur_offset);
> The start offset, third argument, should be cur_offset.
> Everything from alloc_start to cur_offset was freed by the
> list_for_each_entry_safe_loop.
>
> Fixes: 18513091af94 ("btrfs: update btrfs_space_info's bytes_may_use timely")
> Signed-off-by: Robbie Ko <robbieko@synology.com>
I see this error:
generic/102 [02:15:05]
[ 7731.223975] run fstests generic/102 at 2019-04-09 02:15:05
[ 7731.355111] BTRFS info (device vda): disk space caching is enabled
[ 7731.358367] BTRFS info (device vda): has skinny extents
[ 7731.407393] BTRFS: device fsid 99c22651-33bd-4543-96df-da02cc320166 devid 1 transid 5 /dev/vdb
[ 7731.419081] BTRFS info (device vdb): disk space caching is enabled
[ 7731.420671] BTRFS info (device vdb): has skinny extents
[ 7731.421955] BTRFS info (device vdb): flagging fs with big metadata feature
[ 7731.426107] BTRFS info (device vdb): checking UUID tree
[ 7865.389231] BTRFS info (device vdb): disk space caching is enabled
[ 7865.393289] BTRFS info (device vdb): has skinny extents
[02:17:19]- output mismatch (see /tmp/fstests/results//generic/102.out.bad)
--- tests/generic/102.out 2018-10-08 09:51:53.890556726 +0000
+++ /tmp/fstests/results//generic/102.out.bad 2019-04-09 02:17:19.460000000 +0000
@@ -1,7 +1,7 @@
QA output created by 102
wrote 838860800/838860800 bytes at offset 0
XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-wrote 838860800/838860800 bytes at offset 0
+wrote 109707264/838860800 bytes at offset 0
XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
wrote 838860800/838860800 bytes at offset 0
...
(Run 'diff -u /tmp/fstests/tests/generic/102.out /tmp/fstests/results//generic/102.out.bad' to see the entire diff)
This is with current misc-next, with 2 other patches to tree-checker
that are unrelated. I've run fstests twice to be sure that it's not some
random fluke, it happened in both cases.
Running with with one patch below the test passes so I'm reasonably sure
it's happening due to your patch.
For reference:
- misc-next top commit fbcee956b888abbcbfed295a2191b1d0eccb0f09
(reproduced twice)
- one below your patch c8d27001b9067085bd62adf5fdf7cc8a53bd2f7f
(not reproduced)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve
2019-04-11 17:52 ` David Sterba
@ 2019-04-11 18:01 ` Filipe Manana
2019-04-11 18:28 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Filipe Manana @ 2019-04-11 18:01 UTC (permalink / raw)
To: dsterba, robbieko, linux-btrfs, Filipe David Borba Manana
On Thu, Apr 11, 2019 at 6:53 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Tue, Mar 26, 2019 at 11:56:11AM +0800, robbieko wrote:
> > From: Robbie Ko <robbieko@synology.com>
> >
> > When doing fallocate, we first add the range to the reserve_list
> > and then reserve the quota.
> > If quota reservation fails, we'll release all reserved parts of
> > reserve_list.
> > However, cur_offset is not updated to indicate that this
> > range is already been inserted into the list.
> > Therefore, the same range is freed twice.
> > Once at list_for_each_entry loop, and once at the end of the
> > function.
> > This will result in WARN_ON on bytes_may_use when we free the
> > remaining space.
> >
> > At the end, under the 'out' label we have a call to:
> > btrfs_free_reserved_data_space(inode, data_reserved, alloc_start,
> > alloc_end - cur_offset);
> > The start offset, third argument, should be cur_offset.
> > Everything from alloc_start to cur_offset was freed by the
> > list_for_each_entry_safe_loop.
> >
> > Fixes: 18513091af94 ("btrfs: update btrfs_space_info's bytes_may_use timely")
> > Signed-off-by: Robbie Ko <robbieko@synology.com>
>
> I see this error:
>
> generic/102 [02:15:05]
>
> [ 7731.223975] run fstests generic/102 at 2019-04-09 02:15:05
> [ 7731.355111] BTRFS info (device vda): disk space caching is enabled
> [ 7731.358367] BTRFS info (device vda): has skinny extents
> [ 7731.407393] BTRFS: device fsid 99c22651-33bd-4543-96df-da02cc320166 devid 1 transid 5 /dev/vdb
> [ 7731.419081] BTRFS info (device vdb): disk space caching is enabled
> [ 7731.420671] BTRFS info (device vdb): has skinny extents
> [ 7731.421955] BTRFS info (device vdb): flagging fs with big metadata feature
> [ 7731.426107] BTRFS info (device vdb): checking UUID tree
> [ 7865.389231] BTRFS info (device vdb): disk space caching is enabled
> [ 7865.393289] BTRFS info (device vdb): has skinny extents
> [02:17:19]- output mismatch (see /tmp/fstests/results//generic/102.out.bad)
> --- tests/generic/102.out 2018-10-08 09:51:53.890556726 +0000
> +++ /tmp/fstests/results//generic/102.out.bad 2019-04-09 02:17:19.460000000 +0000
> @@ -1,7 +1,7 @@
> QA output created by 102
> wrote 838860800/838860800 bytes at offset 0
> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -wrote 838860800/838860800 bytes at offset 0
> +wrote 109707264/838860800 bytes at offset 0
> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> wrote 838860800/838860800 bytes at offset 0
> ...
> (Run 'diff -u /tmp/fstests/tests/generic/102.out /tmp/fstests/results//generic/102.out.bad' to see the entire diff)
Unrelated to this patch.
The test fails often for at least a few kernel releases now.
The test does not even exercises fallocate() at all...
>
> This is with current misc-next, with 2 other patches to tree-checker
> that are unrelated. I've run fstests twice to be sure that it's not some
> random fluke, it happened in both cases.
>
> Running with with one patch below the test passes so I'm reasonably sure
> it's happening due to your patch.
>
> For reference:
>
> - misc-next top commit fbcee956b888abbcbfed295a2191b1d0eccb0f09
> (reproduced twice)
>
> - one below your patch c8d27001b9067085bd62adf5fdf7cc8a53bd2f7f
> (not reproduced)
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve
2019-04-11 18:01 ` Filipe Manana
@ 2019-04-11 18:28 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2019-04-11 18:28 UTC (permalink / raw)
To: Filipe Manana; +Cc: dsterba, robbieko, linux-btrfs, Filipe David Borba Manana
On Thu, Apr 11, 2019 at 06:01:16PM +0000, Filipe Manana wrote:
> > [ 7731.355111] BTRFS info (device vda): disk space caching is enabled
> > [ 7731.358367] BTRFS info (device vda): has skinny extents
> > [ 7731.407393] BTRFS: device fsid 99c22651-33bd-4543-96df-da02cc320166 devid 1 transid 5 /dev/vdb
> > [ 7731.419081] BTRFS info (device vdb): disk space caching is enabled
> > [ 7731.420671] BTRFS info (device vdb): has skinny extents
> > [ 7731.421955] BTRFS info (device vdb): flagging fs with big metadata feature
> > [ 7731.426107] BTRFS info (device vdb): checking UUID tree
> > [ 7865.389231] BTRFS info (device vdb): disk space caching is enabled
> > [ 7865.393289] BTRFS info (device vdb): has skinny extents
> > [02:17:19]- output mismatch (see /tmp/fstests/results//generic/102.out.bad)
> > --- tests/generic/102.out 2018-10-08 09:51:53.890556726 +0000
> > +++ /tmp/fstests/results//generic/102.out.bad 2019-04-09 02:17:19.460000000 +0000
> > @@ -1,7 +1,7 @@
> > QA output created by 102
> > wrote 838860800/838860800 bytes at offset 0
> > XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > -wrote 838860800/838860800 bytes at offset 0
> > +wrote 109707264/838860800 bytes at offset 0
> > XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > wrote 838860800/838860800 bytes at offset 0
> > ...
> > (Run 'diff -u /tmp/fstests/tests/generic/102.out /tmp/fstests/results//generic/102.out.bad' to see the entire diff)
>
> Unrelated to this patch.
> The test fails often for at least a few kernel releases now.
Ok noted, thanks. After checking more logs, the test indeed fails,
though I see only a few failures in past week so it was a conincidence
that it happened just around this patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-11 18:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-26 3:56 [PATCH v2] Btrfs: fix data bytes_may_use underflow with fallocate due to failed quota reserve robbieko
2019-03-26 10:54 ` Filipe Manana
2019-04-09 0:10 ` David Sterba
2019-04-11 17:52 ` David Sterba
2019-04-11 18:01 ` Filipe Manana
2019-04-11 18:28 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.