On Tue, Dec 03, 2019 at 02:56:08PM -0500, Stephen Rust wrote:
> Hi Ming,
>
> Thanks very much for the patch.
>
> > BTW, you may try the attached test patch. If the issue can be fixed by
> > this patch, that means it is really caused by un-aligned buffer, and
> > the iser driver needs to be fixed.
>
> I have tried the patch, and re-run the test. Results are mixed.
>
> To recap, our test writes the last bytes of an iser attached iscsi
> device. The target device is a LIO iblock, backed by a brd ramdisk.
> The client does a simple `dd`, doing a seek to "size - offset" of the
> device, and writing a buffer of "length" which is equivalent to the
> offset.
>
> For example, to test a write at a 512 offset, seek to device "size -
> 512", and write a length of data 512 bytes.
>
> WITHOUT the patch, writing data at the following offsets from the end
> of the device failed to write all the correct data (rather, the write
> succeeded, but reading the data back it was invalid):
>
> - failed: 512,1024, 2048, 4096, 8192
>
> Anything larger worked fine.
>
> WITH the patch applied, writing data up to an offset of 4096 all now
> worked and verified correctly. However, offsets between 4096 and 8192
> all still failed. I started at 512, and incremented by 512 all the way
> up to 16384. The following offsets all failed to verify the write:
>
> - failed: 4608, 5120, 5632, 6144, 6656, 7168, 7680, 8192
>
> Anything larger continues to work fine with the patch.
>
> As an example, for the failed 8192 case, the `bpftrace lio.bt` trace shows:
>
> 8192 76
> 4096 0
> 4096 0
> 8192 76
> 4096 0
> 4096 0
The following delta change against last patch should fix the issue
with >4096 bvec length:
diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 9ea1894c820d..49e37a7dda63 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -308,7 +308,7 @@ static blk_qc_t brd_make_request(struct request_queue *q, struct bio *bio)
if (err)
goto io_error;
sector += secs;
- offset_in_sec = len - (secs << SECTOR_SHIFT);
+ offset_in_sec += len - (secs << SECTOR_SHIFT);
}
bio_endio(bio);
However, the change on brd is a workaround just for confirming the
issue.
Thanks,
Ming
