Re: [PATCH 2/2] btrfs: qgroup: Fix qgroup reserved space underflow by only freeing reserved ranges

[Chandan Rajendra]

On Friday, December 02, 2016 10:03:07 AM Qu Wenruo wrote:
> [BUG]
> For the following case, btrfs can underflow qgroup reserved space
> at error path:
> (Page size 4K, function name without "btrfs_" prefix)
> 
>  Task A  | Task B
> --
> Buffered_write [0, 2K)   |
> |- check_data_free_space()   |
> |  |- qgroup_reserve_data()  |
> | Range aligned to page  |
> | range [0, 4K)  <<< |
> | 4K bytes reserved  <<< |
> |- copy pages to page cache  |
>  | Buffered_write [2K, 4K)
>  | |- check_data_free_space()
>  | |  |- qgroup_reserved_data()
>  | | Range alinged to page
>  | | range [0, 4K)
>  | | Already reserved by A <<<
>  | | 0 bytes reserved  <<<
>  | |- delalloc_reserve_metadata()
>  | |  And it *FAILED* (Maybe EQUOTA)
>  | |- free_reserved_data_space()
>   |- qgroup_free_data()
>  Range aligned to page range
>  [0, 4K)
>  Freeing 4K
> (Special thanks to Chandan for the detailed report and analyse)
> 
> [CAUSE]
> Above Task B is freeing reserved data range [0, 4K) which is actually
> reserved by Task A.
> 
> And at write back time, page dirty by Task A will go through writeback
> routine, which will free 4K reserved data space at file extent insert
> time, causing the qgroup underflow.
> 
> [FIX]
> For btrfs_qgroup_free_data(), add @reserved parameter to only free
> data ranges reserved by previous btrfs_qgroup_reserve_data().
> So in above case, Task B will try to free 0 byte, so no underflow.
>

The changes look good to me. Also, I did not notice any regressions when
executing fstests with the patch applied.

Reviewed-by: Chandan Rajendra 
Tested-by: Chandan Rajendra 

-- 
chandan

--
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

[PATCH 2/2] btrfs: qgroup: Fix qgroup reserved space underflow by only freeing reserved ranges

[Qu Wenruo]

[BUG]
For the following case, btrfs can underflow qgroup reserved space
at error path:
(Page size 4K, function name without "btrfs_" prefix)

 Task A  | Task B
--
Buffered_write [0, 2K)   |
|- check_data_free_space()   |
|  |- qgroup_reserve_data()  |
| Range aligned to page  |
| range [0, 4K)  <<< |
| 4K bytes reserved  <<< |
|- copy pages to page cache  |
 | Buffered_write [2K, 4K)
 | |- check_data_free_space()
 | |  |- qgroup_reserved_data()
 | | Range alinged to page
 | | range [0, 4K)
 | | Already reserved by A <<<
 | | 0 bytes reserved  <<<
 | |- delalloc_reserve_metadata()
 | |  And it *FAILED* (Maybe EQUOTA)
 | |- free_reserved_data_space()
  |- qgroup_free_data()
 Range aligned to page range
 [0, 4K)
 Freeing 4K
(Special thanks to Chandan for the detailed report and analyse)

[CAUSE]
Above Task B is freeing reserved data range [0, 4K) which is actually
reserved by Task A.

And at write back time, page dirty by Task A will go through writeback
routine, which will free 4K reserved data space at file extent insert
time, causing the qgroup underflow.

[FIX]
For btrfs_qgroup_free_data(), add @reserved parameter to only free
data ranges reserved by previous btrfs_qgroup_reserve_data().
So in above case, Task B will try to free 0 byte, so no underflow.

Reported-by: Chandan Rajendra 
Signed-off-by: Qu Wenruo 
---
 fs/btrfs/ctree.h   |  8 ---
 fs/btrfs/extent-tree.c | 12 ++
 fs/btrfs/file.c| 32 +++--
 fs/btrfs/inode.c   | 35 +--
 fs/btrfs/ioctl.c   |  4 ++--
 fs/btrfs/qgroup.c  | 64 ++
 fs/btrfs/qgroup.h  |  3 ++-
 fs/btrfs/relocation.c  |  8 +++
 8 files changed, 117 insertions(+), 49 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 9f7e109..1d5eaf3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2697,7 +2697,11 @@ enum btrfs_flush_state {
 int btrfs_check_data_free_space(struct inode *inode,
struct extent_changeset *reserved, u64 start, u64 len);
 int btrfs_alloc_data_chunk_ondemand(struct inode *inode, u64 bytes);
-void btrfs_free_reserved_data_space(struct inode *inode, u64 start, u64 len);
+void btrfs_free_reserved_data_space(struct inode *inode,
+   struct extent_changeset *reserved, u64 start, u64 len);
+void btrfs_delalloc_release_space(struct inode *inode,
+   struct extent_changeset *reserved, u64 start, u64 len,
+   enum btrfs_metadata_reserve_type reserve_type);
 void btrfs_free_reserved_data_space_noquota(struct inode *inode, u64 start,
u64 len);
 void btrfs_trans_release_metadata(struct btrfs_trans_handle *trans,
@@ -2720,8 +2724,6 @@ void btrfs_delalloc_release_metadata(struct inode *inode, 
u64 num_bytes,
 int btrfs_delalloc_reserve_space(struct inode *inode,
struct extent_changeset *reserved, u64 start, u64 len,
enum btrfs_metadata_reserve_type reserve_type);
-void btrfs_delalloc_release_space(struct inode *inode, u64 start, u64 len,
-   enum btrfs_metadata_reserve_type reserve_type);
 void btrfs_init_block_rsv(struct btrfs_block_rsv *rsv, unsigned short type);
 struct btrfs_block_rsv *btrfs_alloc_block_rsv(struct btrfs_root *root,
  unsigned short type);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index f116bcf..a1e9c7b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4365,7 +4365,8 @@ void btrfs_free_reserved_data_space_noquota(struct inode 
*inode, u64 start,
  * This one will handle the per-inode data rsv map for accurate reserved
  * space framework.
  */
-void btrfs_free_reserved_data_space(struct inode *inode, u64 start, u64 len)
+void btrfs_free_reserved_data_space(struct inode *inode,
+   struct extent_changeset *reserved, u64 start, u64 len)
 {
struct btrfs_root *root = BTRFS_I(inode)->root;
 
@@ -4375,7 +4376,7 @@ void btrfs_free_reserved_data_space(struct inode *inode, 
u64 start, u64 len)
start = round_down(start, root->sectorsize);
 
btrfs_free_reserved_data_space_noquota(inode, start, len);
-