fixes for handling of split direct I/O bios

fixes for handling of split direct I/O bios

[Christoph Hellwig]

Hi all,

this series fixes two problems in the direct I/O code where the
file_offset field in the dio_private structure is used in a context where
we really need the file_offset for the given low-level bios and not for
the bio submitted by the iomap direct I/O as recorded in the dio_private
structure.  To do so we need a new file_offset in the btrfs_dio
structure.

Found by code inspection as part of my bio cleanups.

Diffstat:
 extent_io.c |    1 +
 inode.c     |   18 ++++++++----------
 volumes.h   |    3 +++
 3 files changed, 12 insertions(+), 10 deletions(-)

[PATCH 1/2] btrfs: fix direct I/O read repair for split bios

[Christoph Hellwig]

When a bio is split in btrfs_submit_direct, dip->file_offset contains
the file offset for the first bio.  But this means the start value used
in btrfs_check_read_dio_bio is incorrect for subsequent bios.  Add
a file_offset field to struct btrfs_bio to pass along the correct offset.

Given that check_data_csum only uses start of an error message this
means problems with this miscalculation will only show up when I/O
fails or checksums mismatch.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c |  1 +
 fs/btrfs/inode.c     | 13 +++++--------
 fs/btrfs/volumes.h   |  3 +++
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d78b3a2d04e3b..c50d3a1225086 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2658,6 +2658,7 @@ int btrfs_repair_one_sector(struct inode *inode,
 
 	repair_bio = btrfs_bio_alloc(1);
 	repair_bbio = btrfs_bio(repair_bio);
+	repair_bbio->file_offset = start;
 	repair_bio->bi_opf = REQ_OP_READ;
 	repair_bio->bi_end_io = failed_bio->bi_end_io;
 	repair_bio->bi_iter.bi_sector = failrec->logical >> 9;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index aa0a60ee26cb7..762133fac0df1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7805,8 +7805,6 @@ static blk_status_t btrfs_check_read_dio_bio(struct btrfs_dio_private *dip,
 	const bool csum = !(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM);
 	struct bio_vec bvec;
 	struct bvec_iter iter;
-	const u64 orig_file_offset = dip->file_offset;
-	u64 start = orig_file_offset;
 	u32 bio_offset = 0;
 	blk_status_t err = BLK_STS_OK;
 
@@ -7816,6 +7814,8 @@ static blk_status_t btrfs_check_read_dio_bio(struct btrfs_dio_private *dip,
 		nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info, bvec.bv_len);
 		pgoff = bvec.bv_offset;
 		for (i = 0; i < nr_sectors; i++) {
+			u64 start = bbio->file_offset + bio_offset;
+
 			ASSERT(pgoff < PAGE_SIZE);
 			if (uptodate &&
 			    (!csum || !check_data_csum(inode, bbio,
@@ -7828,17 +7828,13 @@ static blk_status_t btrfs_check_read_dio_bio(struct btrfs_dio_private *dip,
 			} else {
 				int ret;
 
-				ASSERT((start - orig_file_offset) < UINT_MAX);
-				ret = btrfs_repair_one_sector(inode,
-						&bbio->bio,
-						start - orig_file_offset,
-						bvec.bv_page, pgoff,
+				ret = btrfs_repair_one_sector(inode, &bbio->bio,
+						bio_offset, bvec.bv_page, pgoff,
 						start, bbio->mirror_num,
 						submit_dio_repair_bio);
 				if (ret)
 					err = errno_to_blk_status(ret);
 			}
-			start += sectorsize;
 			ASSERT(bio_offset + sectorsize > bio_offset);
 			bio_offset += sectorsize;
 			pgoff += sectorsize;
@@ -8041,6 +8037,7 @@ static void btrfs_submit_direct(const struct iomap_iter *iter,
 		bio = btrfs_bio_clone_partial(dio_bio, clone_offset, clone_len);
 		bio->bi_private = dip;
 		bio->bi_end_io = btrfs_end_dio_bio;
+		btrfs_bio(bio)->file_offset = file_offset;
 
 		if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
 			status = extract_ordered_extent(BTRFS_I(inode), bio,
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index bd297f23d19e7..f3e28f11cfb6e 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -328,6 +328,9 @@ struct btrfs_fs_devices {
 struct btrfs_bio {
 	unsigned int mirror_num;
 
+	/* for direct I/O */
+	u64 file_offset;
+
 	/* @device is for stripe IO submission. */
 	struct btrfs_device *device;
 	u8 *csum;
-- 
2.30.2

Re: [PATCH 1/2] btrfs: fix direct I/O read repair for split bios

[David Sterba]

On Thu, Mar 24, 2022 at 05:06:27PM +0100, Christoph Hellwig wrote:
> When a bio is split in btrfs_submit_direct, dip->file_offset contains
> the file offset for the first bio.  But this means the start value used
> in btrfs_check_read_dio_bio is incorrect for subsequent bios.  Add
> a file_offset field to struct btrfs_bio to pass along the correct offset.
> 
> Given that check_data_csum only uses start of an error message this
> means problems with this miscalculation will only show up when I/O
> fails or checksums mismatch.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Qu, you've removed the same logic in f4f39fc5dc30 ("btrfs: remove
btrfs_bio::logical member") where it was a different name for the same
variable. What changed in the logic that we don't need to store it along
the btrfs_bio and that btrfs_dio_private can't provide anymore?

I'm a bit worried about your changes that remove/rewrite code, silently
introducing bugs so it has to be reinstated. We don't have enough
review coverage and in the amount of patches you send I'm increasingly
worried how many bugs I've inadvertently let in.

Re: [PATCH 1/2] btrfs: fix direct I/O read repair for split bios

[Qu Wenruo]

On 2022/3/30 22:43, David Sterba wrote:
> On Thu, Mar 24, 2022 at 05:06:27PM +0100, Christoph Hellwig wrote:
>> When a bio is split in btrfs_submit_direct, dip->file_offset contains
>> the file offset for the first bio.  But this means the start value used
>> in btrfs_check_read_dio_bio is incorrect for subsequent bios.  Add
>> a file_offset field to struct btrfs_bio to pass along the correct offset.
>>
>> Given that check_data_csum only uses start of an error message this
>> means problems with this miscalculation will only show up when I/O
>> fails or checksums mismatch.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> Qu, you've removed the same logic in f4f39fc5dc30 ("btrfs: remove
> btrfs_bio::logical member") where it was a different name for the same
> variable. What changed in the logic that we don't need to store it along
> the btrfs_bio and that btrfs_dio_private can't provide anymore?

All my fault, I didn't realize that in btrfs_submit_direct() what we
really do is splitting the iomap bio.

Thus we still need that @logical member as dip is only allocated for the
whole iomap bio, not for each split btrfs bio.

Thus we need the fixes: tag.

>
> I'm a bit worried about your changes that remove/rewrite code, silently
> introducing bugs so it has to be reinstated. We don't have enough
> review coverage and in the amount of patches you send I'm increasingly
> worried how many bugs I've inadvertently let in.

Normally it should be caught by test cases. But test case coverage is
not that better than our review coverage, especially for read repair, as
it's a btrfs specific feature, and almost impossible to do stress tests.

The good news is, for most of my subpage related rewrite, the existing
test cases are pretty good catching the bugs.


I don't really have better way other than adding regression tests cases
until we found some regression.

Thanks,
Qu

Re: [PATCH 1/2] btrfs: fix direct I/O read repair for split bios

[Qu Wenruo]

On 2022/3/31 06:24, Qu Wenruo wrote:
>
>
> On 2022/3/30 22:43, David Sterba wrote:
>> On Thu, Mar 24, 2022 at 05:06:27PM +0100, Christoph Hellwig wrote:
>>> When a bio is split in btrfs_submit_direct, dip->file_offset contains
>>> the file offset for the first bio.  But this means the start value used
>>> in btrfs_check_read_dio_bio is incorrect for subsequent bios.  Add
>>> a file_offset field to struct btrfs_bio to pass along the correct
>>> offset.
>>>
>>> Given that check_data_csum only uses start of an error message this
>>> means problems with this miscalculation will only show up when I/O
>>> fails or checksums mismatch.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Qu, you've removed the same logic in f4f39fc5dc30 ("btrfs: remove
>> btrfs_bio::logical member") where it was a different name for the same
>> variable. What changed in the logic that we don't need to store it along
>> the btrfs_bio and that btrfs_dio_private can't provide anymore?
>
> All my fault, I didn't realize that in btrfs_submit_direct() what we
> really do is splitting the iomap bio.
>
> Thus we still need that @logical member as dip is only allocated for the
> whole iomap bio, not for each split btrfs bio.
>
> Thus we need the fixes: tag.
>
>>
>> I'm a bit worried about your changes that remove/rewrite code, silently
>> introducing bugs so it has to be reinstated. We don't have enough
>> review coverage and in the amount of patches you send I'm increasingly
>> worried how many bugs I've inadvertently let in.
>
> Normally it should be caught by test cases. But test case coverage is
> not that better than our review coverage, especially for read repair, as
> it's a btrfs specific feature, and almost impossible to do stress tests.
>
> The good news is, for most of my subpage related rewrite, the existing
> test cases are pretty good catching the bugs.
>
>
> I don't really have better way other than adding regression tests cases
> until we found some regression.

While crafting the test case for this particular case, I found all the
existing tools, like dd (iflag=direct) or xfs_io (pwrite -d -i), are all
reading the content using 4K block, even block size is specified.

Thus unable to reproduce the bug (no split will happen).

Any good tool to cause a large direct IO read?

Thanks,
Qu

>
> Thanks,
> Qu

Re: [PATCH 1/2] btrfs: fix direct I/O read repair for split bios

[Qu Wenruo]

On 2022/3/31 07:14, Qu Wenruo wrote:
>
>
> On 2022/3/31 06:24, Qu Wenruo wrote:
>>
>>
>> On 2022/3/30 22:43, David Sterba wrote:
>>> On Thu, Mar 24, 2022 at 05:06:27PM +0100, Christoph Hellwig wrote:
>>>> When a bio is split in btrfs_submit_direct, dip->file_offset contains
>>>> the file offset for the first bio.  But this means the start value used
>>>> in btrfs_check_read_dio_bio is incorrect for subsequent bios.  Add
>>>> a file_offset field to struct btrfs_bio to pass along the correct
>>>> offset.
>>>>
>>>> Given that check_data_csum only uses start of an error message this
>>>> means problems with this miscalculation will only show up when I/O
>>>> fails or checksums mismatch.
>>>>
>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>>
>>> Qu, you've removed the same logic in f4f39fc5dc30 ("btrfs: remove
>>> btrfs_bio::logical member") where it was a different name for the same
>>> variable. What changed in the logic that we don't need to store it along
>>> the btrfs_bio and that btrfs_dio_private can't provide anymore?
>>
>> All my fault, I didn't realize that in btrfs_submit_direct() what we
>> really do is splitting the iomap bio.
>>
>> Thus we still need that @logical member as dip is only allocated for the
>> whole iomap bio, not for each split btrfs bio.
>>
>> Thus we need the fixes: tag.
>>
>>>
>>> I'm a bit worried about your changes that remove/rewrite code, silently
>>> introducing bugs so it has to be reinstated. We don't have enough
>>> review coverage and in the amount of patches you send I'm increasingly
>>> worried how many bugs I've inadvertently let in.
>>
>> Normally it should be caught by test cases. But test case coverage is
>> not that better than our review coverage, especially for read repair, as
>> it's a btrfs specific feature, and almost impossible to do stress tests.
>>
>> The good news is, for most of my subpage related rewrite, the existing
>> test cases are pretty good catching the bugs.
>>
>>
>> I don't really have better way other than adding regression tests cases
>> until we found some regression.
>
> While crafting the test case for this particular case, I found all the
> existing tools, like dd (iflag=direct) or xfs_io (pwrite -d -i), are all
> reading the content using 4K block, even block size is specified.
>
> Thus unable to reproduce the bug (no split will happen).
>
> Any good tool to cause a large direct IO read?

OK, all these tools (including a new md5sum_direct I crafted) are
working as expected.

The always sectorsized read is from btrfs, not those tools.

In btrfs_dio_iomap_begin(), if the operation is read, we always limit
the size to sectorsize:

	if (!write)
		len = min_t(u64, len, fs_info->sectorsize);

This behavior saved my ass, as all our direct read will only cause one
single sector, thus it will never be split, thus read repair will never
need to both the dip->file_offset mismatch.

Furthermore, write path doesn't need to bother file offset in the split
bio anyway.

So no fixes: tag is needed, I am saved by a weird behavior there.

Thanks,
Qu



>
> Thanks,
> Qu
>
>>
>> Thanks,
>> Qu

[PATCH 2/2] btrfs: fix direct I/O writes for split bios on zoned devices

[Christoph Hellwig]

When a bio is split in btrfs_submit_direct, dip->file_offset contains
the file offset for the first bio.  But this means the start value used
in btrfs_end_dio_bio to record the write location for zone devices is
incorrect for subsequent bios.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 762133fac0df1..c6f71551ba135 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7861,6 +7861,7 @@ static blk_status_t btrfs_submit_bio_start_direct_io(struct inode *inode,
 static void btrfs_end_dio_bio(struct bio *bio)
 {
 	struct btrfs_dio_private *dip = bio->bi_private;
+	struct btrfs_bio *bbio = btrfs_bio(bio);
 	blk_status_t err = bio->bi_status;
 
 	if (err)
@@ -7871,12 +7872,12 @@ static void btrfs_end_dio_bio(struct bio *bio)
 			   bio->bi_iter.bi_size, err);
 
 	if (bio_op(bio) == REQ_OP_READ)
-		err = btrfs_check_read_dio_bio(dip, btrfs_bio(bio), !err);
+		err = btrfs_check_read_dio_bio(dip, bbio, !err);
 
 	if (err)
 		dip->dio_bio->bi_status = err;
 
-	btrfs_record_physical_zoned(dip->inode, dip->file_offset, bio);
+	btrfs_record_physical_zoned(dip->inode, bbio->file_offset, bio);
 
 	bio_put(bio);
 	btrfs_dio_private_put(dip);
-- 
2.30.2

Re: fixes for handling of split direct I/O bios

[Sweet Tea Dorminy]

On 3/24/22 12:06, Christoph Hellwig wrote:
> Hi all,
> 
> this series fixes two problems in the direct I/O code where the
> file_offset field in the dio_private structure is used in a context where
> we really need the file_offset for the given low-level bios and not for
> the bio submitted by the iomap direct I/O as recorded in the dio_private
> structure.  To do so we need a new file_offset in the btrfs_dio
> structure.
> 
> Found by code inspection as part of my bio cleanups.
> 
> Diffstat:
>   extent_io.c |    1 +
>   inode.c     |   18 ++++++++----------
>   volumes.h   |    3 +++
>   3 files changed, 12 insertions(+), 10 deletions(-)

Reviewed-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

I'm pretty new and don't know much about the criteria for cc'ing stable, 
but arguably this makes the check_data_csum() error message not lie 
about the start offset in such cases and it seems like a very low risk 
improvement to me... might it be worth adding a Fixes: tag / might this 
be a reasonable fix for stable?

Re: fixes for handling of split direct I/O bios

[Christoph Hellwig]

On Thu, Mar 24, 2022 at 12:49:26PM -0400, Sweet Tea Dorminy wrote:
> I'm pretty new and don't know much about the criteria for cc'ing stable, 
> but arguably this makes the check_data_csum() error message not lie about 
> the start offset in such cases and it seems like a very low risk 
> improvement to me... might it be worth adding a Fixes: tag / might this be 
> a reasonable fix for stable?

The error message is the least of the problems - if there is an I/O
error or checksum mismatch this bug will make the repair not work.

Re: fixes for handling of split direct I/O bios

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: fixes for handling of split direct I/O bios

[Naohiro Aota]

On Thu, Mar 24, 2022 at 05:06:26PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series fixes two problems in the direct I/O code where the
> file_offset field in the dio_private structure is used in a context where
> we really need the file_offset for the given low-level bios and not for
> the bio submitted by the iomap direct I/O as recorded in the dio_private
> structure.  To do so we need a new file_offset in the btrfs_dio
> structure.
> 
> Found by code inspection as part of my bio cleanups.

Looks good to me. Thank you for catching this.

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

> Diffstat:
>  extent_io.c |    1 +
>  inode.c     |   18 ++++++++----------
>  volumes.h   |    3 +++
>  3 files changed, 12 insertions(+), 10 deletions(-)

Re: fixes for handling of split direct I/O bios

[Christoph Hellwig]

David,

do you plan to pick these up?

On Thu, Mar 24, 2022 at 05:06:26PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series fixes two problems in the direct I/O code where the
> file_offset field in the dio_private structure is used in a context where
> we really need the file_offset for the given low-level bios and not for
> the bio submitted by the iomap direct I/O as recorded in the dio_private
> structure.  To do so we need a new file_offset in the btrfs_dio
> structure.
> 
> Found by code inspection as part of my bio cleanups.
> 
> Diffstat:
>  extent_io.c |    1 +
>  inode.c     |   18 ++++++++----------
>  volumes.h   |    3 +++
>  3 files changed, 12 insertions(+), 10 deletions(-)
---end quoted text---

Re: fixes for handling of split direct I/O bios

[David Sterba]

On Thu, Mar 24, 2022 at 05:06:26PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series fixes two problems in the direct I/O code where the
> file_offset field in the dio_private structure is used in a context where
> we really need the file_offset for the given low-level bios and not for
> the bio submitted by the iomap direct I/O as recorded in the dio_private
> structure.  To do so we need a new file_offset in the btrfs_dio
> structure.
> 
> Found by code inspection as part of my bio cleanups.
> 
> Diffstat:
>  extent_io.c |    1 +
>  inode.c     |   18 ++++++++----------
>  volumes.h   |    3 +++
>  3 files changed, 12 insertions(+), 10 deletions(-)

Added to to misc-next, thanks.