| Message ID | 20210427230349.369603-37-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add data write support for subpage | expand |
On 2021/4/28 上午7:03, Qu Wenruo wrote: > [BUG] > When running the following fsx command (extracted from generic/127) on > subpage btrfs, it can create inline extent with regular extents: > > fsx -q -l 262144 -o 65536 -S 191110531 -N 9057 -R -W $mnt/file > /tmp/fsx > > The offending extent would look like: > > item 9 key (257 INODE_REF 256) itemoff 15703 itemsize 14 > index 2 namelen 4 name: file > item 10 key (257 EXTENT_DATA 0) itemoff 14975 itemsize 728 > generation 7 type 0 (inline) > inline extent data size 707 ram_bytes 707 compression 0 (none) > item 11 key (257 EXTENT_DATA 4096) itemoff 14922 itemsize 53 > generation 7 type 2 (prealloc) > prealloc data disk byte 102346752 nr 4096 > prealloc data offset 0 nr 4096 > > [CAUSE] > For subpage btrfs, the writeback is triggered in page unit, which means, > even if we just want to writeback range [16K, 20K) for 64K page system, > we will still try to writeback any dirty sector of range [0, 64K). > > This is never a problem if sectorsize == PAGE_SIZE, but for subpage, > this can cause unexpected problems. > > For above test case, the last several operations from fsx are: > > 9055 trunc from 0x40000 to 0x2c3 > 9057 falloc from 0x164c to 0x19d2 (0x386 bytes) With more investigation into this specific problem, it turns out it's really something specific to falloc() (and maybe reflink) > > In operation 9055, we dirtied sector [0, 4096), then in falloc, we call > btrfs_wait_ordered_range(inode, start=4096, len=4096), only expecting to > writeback any dirty data in [4096, 8192), but nothing else. This part still stands. > > Unfortunately, in subpage case, above btrfs_wait_ordered_range() will > trigger writeback of the range [0, 64K), which includes the data at [0, > 4096). But the problem is really in the sequence of btrfs_wait_ordered_range() and btrfs_cont_expand(). Currently, we call btrfs_cont_expand() first, then btrfs_wait_ordered_range(), which leads to the inline extent then regular extent. But the truth is, if we just call btrfs_wait_ordered_range() then btrfs_cont_expand() we will no longer got the problem. As btrfs_wait_ordered_range() will writeback the first sector as inline, then btrfs_cont_expand() re-dirty the first sector, so that it will be re-written as regular extent, after we enlarge the isize. I also checked reflink, which is doing the same cont_expand() then wait_ordered_extent(). AFAIK we could just change the sequence so we don't need to disable inline extent completely. But I'm not yet 100% sure, thus I'd prefer to make btrfs-check to report such inline + regular layout as an error, then do more tests to make sure it will work as expected. Thanks, Qu > > And since at the call site, we haven't yet increased i_size, which is > still 707, this means cow_file_range() can insert an inline extent. > > Resulting above inline + regular extent. > > [WORKAROUND] > I don't really have any good short-term solution yet, as this means all > operations that would trigger writeback need to be reviewed for any > isize change. > > So here I choose to disable inline extent creation for subpage case as a > workaround. > We have done tons of work just to avoid such extent, so I don't to > create an exception just for subpage. > > This only affects inline extent creation, btrfs subpage support has no > problem reading existing inline extents at all. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/inode.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index fd648f2c0242..a2ac8d6eeba5 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -663,7 +663,11 @@ static noinline int compress_file_range(struct async_chunk *async_chunk) > } > } > cont: > - if (start == 0) { > + /* > + * Check cow_file_range() for why we don't even try to create > + * inline extent for subpage case. > + */ > + if (start == 0 && fs_info->sectorsize == PAGE_SIZE) { > /* lets try to make an inline extent */ > if (ret || total_in < actual_end) { > /* we didn't compress the entire range, try > @@ -1061,7 +1065,17 @@ static noinline int cow_file_range(struct btrfs_inode *inode, > > inode_should_defrag(inode, start, end, num_bytes, SZ_64K); > > - if (start == 0) { > + /* > + * Due to the page size limit, for subpage we can only trigger the > + * writeback for the dirty sectors of page, that means data writeback > + * is doing more writeback than what we want. > + * > + * This is especially unexpected for some call sites like fallocate, > + * where we only increase isize after everything is done. > + * This means we can trigger inline extent even we didn't want. > + * So here we skip inline extent creation completely. > + */ > + if (start == 0 && fs_info->sectorsize == PAGE_SIZE) { > /* lets try to make an inline extent */ > ret = cow_file_range_inline(inode, start, end, 0, > BTRFS_COMPRESS_NONE, NULL); >
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index fd648f2c0242..a2ac8d6eeba5 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -663,7 +663,11 @@ static noinline int compress_file_range(struct async_chunk *async_chunk) } } cont: - if (start == 0) { + /* + * Check cow_file_range() for why we don't even try to create + * inline extent for subpage case. + */ + if (start == 0 && fs_info->sectorsize == PAGE_SIZE) { /* lets try to make an inline extent */ if (ret || total_in < actual_end) { /* we didn't compress the entire range, try @@ -1061,7 +1065,17 @@ static noinline int cow_file_range(struct btrfs_inode *inode, inode_should_defrag(inode, start, end, num_bytes, SZ_64K); - if (start == 0) { + /* + * Due to the page size limit, for subpage we can only trigger the + * writeback for the dirty sectors of page, that means data writeback + * is doing more writeback than what we want. + * + * This is especially unexpected for some call sites like fallocate, + * where we only increase isize after everything is done. + * This means we can trigger inline extent even we didn't want. + * So here we skip inline extent creation completely. + */ + if (start == 0 && fs_info->sectorsize == PAGE_SIZE) { /* lets try to make an inline extent */ ret = cow_file_range_inline(inode, start, end, 0, BTRFS_COMPRESS_NONE, NULL);
[BUG] When running the following fsx command (extracted from generic/127) on subpage btrfs, it can create inline extent with regular extents: fsx -q -l 262144 -o 65536 -S 191110531 -N 9057 -R -W $mnt/file > /tmp/fsx The offending extent would look like: item 9 key (257 INODE_REF 256) itemoff 15703 itemsize 14 index 2 namelen 4 name: file item 10 key (257 EXTENT_DATA 0) itemoff 14975 itemsize 728 generation 7 type 0 (inline) inline extent data size 707 ram_bytes 707 compression 0 (none) item 11 key (257 EXTENT_DATA 4096) itemoff 14922 itemsize 53 generation 7 type 2 (prealloc) prealloc data disk byte 102346752 nr 4096 prealloc data offset 0 nr 4096 [CAUSE] For subpage btrfs, the writeback is triggered in page unit, which means, even if we just want to writeback range [16K, 20K) for 64K page system, we will still try to writeback any dirty sector of range [0, 64K). This is never a problem if sectorsize == PAGE_SIZE, but for subpage, this can cause unexpected problems. For above test case, the last several operations from fsx are: 9055 trunc from 0x40000 to 0x2c3 9057 falloc from 0x164c to 0x19d2 (0x386 bytes) In operation 9055, we dirtied sector [0, 4096), then in falloc, we call btrfs_wait_ordered_range(inode, start=4096, len=4096), only expecting to writeback any dirty data in [4096, 8192), but nothing else. Unfortunately, in subpage case, above btrfs_wait_ordered_range() will trigger writeback of the range [0, 64K), which includes the data at [0, 4096). And since at the call site, we haven't yet increased i_size, which is still 707, this means cow_file_range() can insert an inline extent. Resulting above inline + regular extent. [WORKAROUND] I don't really have any good short-term solution yet, as this means all operations that would trigger writeback need to be reviewed for any isize change. So here I choose to disable inline extent creation for subpage case as a workaround. We have done tons of work just to avoid such extent, so I don't to create an exception just for subpage. This only affects inline extent creation, btrfs subpage support has no problem reading existing inline extents at all. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/inode.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)