[PATCH] btrfs: fix encoded write i_size corruption

[PATCH] btrfs: fix encoded write i_size corruption

[Boris Burkov]

We have observed a btrfs filesystem corruption on workloads using
NOHOLES and encoded writes via sendstream v2. The symptom is that a file
appears to be truncated to the end of its last aligned extent, even
though the final unaligned extent and even the file extent and otherwise
correctly updated inode item have been written.

So if we were writing out a 1MiB+X file via 8 128K extents and one
extent of length X, isize would be set to 1MiB, but the ninth extent,
nbyte, etc.. would all appear correct otherwise.

The source of the race is a narrow (one code line..) window in which a
noholes fs has read in an updated isize, but has not yet set a shared
disk_i_size variable to write. Therefore, if two ordered extents run in
parallel (par for the course for receive workloads), the following
sequence can play out: (following "threads" a bit loosely, since there
are callbacks involved for endio but extra threads aren't needed to
cause the issue)

ENC-WR1 (second to last)                                         ENC-WR2 (last)
-------                                                          -------
btrfs_do_encoded_write
  set isize = 1M
  submit bio B1 ending at 1M
endio B1
btrfs_inode_safe_disk_i_size_write
  local isize = 1M
  falls off a cliff for some reason
                                                            btrfs_do_encoded_write
                                                              set isize = 1M+X
                                                              submit bio B2 ending at 1M+X
                                                            endio B2
							    btrfs_inode_safe_disk_i_size_write
                                                              local isize = 1M+X
                                                              disk_i_size = 1M+X
  disk_i_size = 1M
							    btrfs_delayed_update_inode
  btrfs_delayed_update_inode

And the delayed inode ends up filled with nbytes=1M+X and isize=1M, and
writes respect isize and present a corruted file missing its last
extents.

Fix this by holding the inode lock in the noholes case so that a thread
can't sneak in a write to disk_i_size that gets overwritten with an out
of date isize.

Fixes: 41a2ee75aab0290 btrfs: introduce per-inode file extent tree
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/file-item.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 018c711a0bc8..cd4cce9ba443 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -52,13 +52,13 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode, u64 new_i_siz
 	u64 start, end, i_size;
 	int ret;
 
+	spin_lock(&inode->lock);
 	i_size = new_i_size ?: i_size_read(&inode->vfs_inode);
 	if (btrfs_fs_incompat(fs_info, NO_HOLES)) {
 		inode->disk_i_size = i_size;
-		return;
+		goto out_unlock;
 	}
 
-	spin_lock(&inode->lock);
 	ret = find_contiguous_extent_bit(&inode->file_extent_tree, 0, &start,
 					 &end, EXTENT_DIRTY);
 	if (!ret && start == 0)
@@ -66,6 +66,7 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode, u64 new_i_siz
 	else
 		i_size = 0;
 	inode->disk_i_size = i_size;
+out_unlock:
 	spin_unlock(&inode->lock);
 }
 
-- 
2.34.1

Re: [PATCH] btrfs: fix encoded write i_size corruption

[Josef Bacik]

On Fri, Apr 28, 2023 at 02:02:11PM -0700, Boris Burkov wrote:
> We have observed a btrfs filesystem corruption on workloads using
> NOHOLES and encoded writes via sendstream v2. The symptom is that a file
> appears to be truncated to the end of its last aligned extent, even
> though the final unaligned extent and even the file extent and otherwise
> correctly updated inode item have been written.
> 
> So if we were writing out a 1MiB+X file via 8 128K extents and one
> extent of length X, isize would be set to 1MiB, but the ninth extent,
> nbyte, etc.. would all appear correct otherwise.
> 
> The source of the race is a narrow (one code line..) window in which a
> noholes fs has read in an updated isize, but has not yet set a shared
> disk_i_size variable to write. Therefore, if two ordered extents run in
> parallel (par for the course for receive workloads), the following
> sequence can play out: (following "threads" a bit loosely, since there
> are callbacks involved for endio but extra threads aren't needed to
> cause the issue)
> 
> ENC-WR1 (second to last)                                         ENC-WR2 (last)
> -------                                                          -------
> btrfs_do_encoded_write
>   set isize = 1M
>   submit bio B1 ending at 1M
> endio B1
> btrfs_inode_safe_disk_i_size_write
>   local isize = 1M
>   falls off a cliff for some reason
>                                                             btrfs_do_encoded_write
>                                                               set isize = 1M+X
>                                                               submit bio B2 ending at 1M+X
>                                                             endio B2
> 							    btrfs_inode_safe_disk_i_size_write
>                                                               local isize = 1M+X
>                                                               disk_i_size = 1M+X
>   disk_i_size = 1M
> 							    btrfs_delayed_update_inode
>   btrfs_delayed_update_inode
> 
> And the delayed inode ends up filled with nbytes=1M+X and isize=1M, and
> writes respect isize and present a corruted file missing its last
> extents.
> 
> Fix this by holding the inode lock in the noholes case so that a thread
> can't sneak in a write to disk_i_size that gets overwritten with an out
> of date isize.
> 
> Fixes: 41a2ee75aab0290 btrfs: introduce per-inode file extent tree
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs: fix encoded write i_size corruption

[David Sterba]

On Fri, Apr 28, 2023 at 02:02:11PM -0700, Boris Burkov wrote:
> We have observed a btrfs filesystem corruption on workloads using
> NOHOLES and encoded writes via sendstream v2. The symptom is that a file
> appears to be truncated to the end of its last aligned extent, even
> though the final unaligned extent and even the file extent and otherwise
> correctly updated inode item have been written.
> 
> So if we were writing out a 1MiB+X file via 8 128K extents and one
> extent of length X, isize would be set to 1MiB, but the ninth extent,
> nbyte, etc.. would all appear correct otherwise.
> 
> The source of the race is a narrow (one code line..) window in which a
> noholes fs has read in an updated isize, but has not yet set a shared
> disk_i_size variable to write. Therefore, if two ordered extents run in
> parallel (par for the course for receive workloads), the following
> sequence can play out: (following "threads" a bit loosely, since there
> are callbacks involved for endio but extra threads aren't needed to
> cause the issue)
> 
> ENC-WR1 (second to last)                                         ENC-WR2 (last)
> -------                                                          -------
> btrfs_do_encoded_write
>   set isize = 1M
>   submit bio B1 ending at 1M
> endio B1
> btrfs_inode_safe_disk_i_size_write
>   local isize = 1M
>   falls off a cliff for some reason
>                                                             btrfs_do_encoded_write
>                                                               set isize = 1M+X
>                                                               submit bio B2 ending at 1M+X
>                                                             endio B2
> 							    btrfs_inode_safe_disk_i_size_write
>                                                               local isize = 1M+X
>                                                               disk_i_size = 1M+X
>   disk_i_size = 1M
> 							    btrfs_delayed_update_inode
>   btrfs_delayed_update_inode
> 
> And the delayed inode ends up filled with nbytes=1M+X and isize=1M, and
> writes respect isize and present a corruted file missing its last
> extents.
> 
> Fix this by holding the inode lock in the noholes case so that a thread
> can't sneak in a write to disk_i_size that gets overwritten with an out
> of date isize.
> 
> Fixes: 41a2ee75aab0290 btrfs: introduce per-inode file extent tree
> Signed-off-by: Boris Burkov <boris@bur.io>

Added to misc-next, thanks.