| Message ID | 20230418-btrfs-extent-reads-v1-3-47ba9839f0cc@codewreck.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix and improve read code | expand |
On 2023/4/18 09:17, Dominique Martinet wrote: > From: Dominique Martinet <dominique.martinet@atmark-techno.com> > > btfs_file_read's truncate path has a comment noting '>0 means no extent' > and bailing out immediately, but the buffer has not been written so > probably needs zeroing out. > > This is a theorical fix only and hasn't been tested on a file that > actually runs this code path. IIRC there is a memset() at the very beginning of btrfs_file_read() to set the whole dest memory to zero. This is to handle cases like NO_HOLE cases, which we can skip hole extents. Thanks, Qu > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > fs/btrfs/inode.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index efffec0f2e68..23c006c98c3b 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -756,9 +756,12 @@ int btrfs_file_read(struct btrfs_root *root, u64 ino, u64 file_offset, u64 len, > btrfs_release_path(&path); > ret = lookup_data_extent(root, &path, ino, cur, > &next_offset); > - /* <0 is error, >0 means no extent */ > - if (ret) > + /* <0 is error, >0 means no extent: zero end of buffer */ > + if (ret) { > + if (ret > 0) > + memset(dest + cur, 0, end - cur); > goto out; > + } > fi = btrfs_item_ptr(path.nodes[0], path.slots[0], > struct btrfs_file_extent_item); > ret = read_and_truncate_page(&path, fi, cur, >
Qu Wenruo wrote on Tue, Apr 18, 2023 at 10:04:56AM +0800: > > This is a theorical fix only and hasn't been tested on a file that > > actually runs this code path. > > IIRC there is a memset() at the very beginning of btrfs_file_read() to set > the whole dest memory to zero. Right, sorry. I'll drop this and send a new patch that removes the duplicate memset (at "The whole file is a hole" comment) instead as seeing multiple memsets is what made me think it'd be necessary without thinking; that can be done even if we rework the function a bit in later cleanups...
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index efffec0f2e68..23c006c98c3b 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -756,9 +756,12 @@ int btrfs_file_read(struct btrfs_root *root, u64 ino, u64 file_offset, u64 len, btrfs_release_path(&path); ret = lookup_data_extent(root, &path, ino, cur, &next_offset); - /* <0 is error, >0 means no extent */ - if (ret) + /* <0 is error, >0 means no extent: zero end of buffer */ + if (ret) { + if (ret > 0) + memset(dest + cur, 0, end - cur); goto out; + } fi = btrfs_item_ptr(path.nodes[0], path.slots[0], struct btrfs_file_extent_item); ret = read_and_truncate_page(&path, fi, cur,