zram: take device and not only bvec offset into account

[PATCH] zram: take device and not only bvec offset into account

Posted by Christoph Hellwig 2 years, 1 month ago

Commit af8b04c63708 ("zram: simplify bvec iteration in
__zram_make_request") changed the bio iteration in zram to rely on the
implicit capping to page boundaries in bio_for_each_segment.  But it
failed to care for the fact zram not only care about the page alignment
of the bio payload, but also the page alignment into the device.  For
buffered I/O and swap those are the same, but for direct I/O or kernel
internal I/O like XFS log buffer writes they can differ.

Fix this by open coding bio_for_each_segment and limiting the bvec len
so that it never crosses over a page alignment boundary in the device
in addition to the payload boundary already taken care of by
bio_iter_iovec.

Fixes: af8b04c63708 ("zram: simplify bvec iteration in __zram_make_request")
Reported-by: Dusty Mabe <dusty@dustymabe.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/zram/zram_drv.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 5676e6dd5b1672..06673c6ca25555 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1870,15 +1870,16 @@ static void zram_bio_discard(struct zram *zram, struct bio *bio)
 
 static void zram_bio_read(struct zram *zram, struct bio *bio)
 {
-	struct bvec_iter iter;
-	struct bio_vec bv;
-	unsigned long start_time;
+	unsigned long start_time = bio_start_io_acct(bio);
+	struct bvec_iter iter = bio->bi_iter;
 
-	start_time = bio_start_io_acct(bio);
-	bio_for_each_segment(bv, bio, iter) {
+	do {
 		u32 index = iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
 		u32 offset = (iter.bi_sector & (SECTORS_PER_PAGE - 1)) <<
 				SECTOR_SHIFT;
+		struct bio_vec bv = bio_iter_iovec(bio, iter);
+
+		bv.bv_len = min_t(u32, bv.bv_len, PAGE_SIZE - offset);
 
 		if (zram_bvec_read(zram, &bv, index, offset, bio) < 0) {
 			atomic64_inc(&zram->stats.failed_reads);
@@ -1890,22 +1891,26 @@ static void zram_bio_read(struct zram *zram, struct bio *bio)
 		zram_slot_lock(zram, index);
 		zram_accessed(zram, index);
 		zram_slot_unlock(zram, index);
-	}
+
+		bio_advance_iter_single(bio, &iter, bv.bv_len);
+	} while (iter.bi_size);
+
 	bio_end_io_acct(bio, start_time);
 	bio_endio(bio);
 }
 
 static void zram_bio_write(struct zram *zram, struct bio *bio)
 {
-	struct bvec_iter iter;
-	struct bio_vec bv;
-	unsigned long start_time;
+	unsigned long start_time = bio_start_io_acct(bio);
+	struct bvec_iter iter = bio->bi_iter;
 
-	start_time = bio_start_io_acct(bio);
-	bio_for_each_segment(bv, bio, iter) {
+	do {
 		u32 index = iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
 		u32 offset = (iter.bi_sector & (SECTORS_PER_PAGE - 1)) <<
 				SECTOR_SHIFT;
+		struct bio_vec bv = bio_iter_iovec(bio, iter);
+
+		bv.bv_len = min_t(u32, bv.bv_len, PAGE_SIZE - offset);
 
 		if (zram_bvec_write(zram, &bv, index, offset, bio) < 0) {
 			atomic64_inc(&zram->stats.failed_writes);
@@ -1916,7 +1921,10 @@ static void zram_bio_write(struct zram *zram, struct bio *bio)
 		zram_slot_lock(zram, index);
 		zram_accessed(zram, index);
 		zram_slot_unlock(zram, index);
-	}
+
+		bio_advance_iter_single(bio, &iter, bv.bv_len);
+	} while (iter.bi_size);
+
 	bio_end_io_acct(bio, start_time);
 	bio_endio(bio);
 }
-- 
2.39.2

Re: [PATCH] zram: take device and not only bvec offset into account

Posted by Jens Axboe 2 years, 1 month ago

On Sat, 05 Aug 2023 07:55:37 +0200, Christoph Hellwig wrote:
> Commit af8b04c63708 ("zram: simplify bvec iteration in
> __zram_make_request") changed the bio iteration in zram to rely on the
> implicit capping to page boundaries in bio_for_each_segment.  But it
> failed to care for the fact zram not only care about the page alignment
> of the bio payload, but also the page alignment into the device.  For
> buffered I/O and swap those are the same, but for direct I/O or kernel
> internal I/O like XFS log buffer writes they can differ.
> 
> [...]

Applied, thanks!

[1/1] zram: take device and not only bvec offset into account
      commit: 95848dcb9d676738411a8ff70a9704039f1b3982

Best regards,
-- 
Jens Axboe

Re: [PATCH] zram: take device and not only bvec offset into account

Posted by Sergey Senozhatsky 2 years, 1 month ago

On (23/08/05 07:55), Christoph Hellwig wrote:
> Commit af8b04c63708 ("zram: simplify bvec iteration in
> __zram_make_request") changed the bio iteration in zram to rely on the
> implicit capping to page boundaries in bio_for_each_segment.  But it
> failed to care for the fact zram not only care about the page alignment
> of the bio payload, but also the page alignment into the device.  For
> buffered I/O and swap those are the same, but for direct I/O or kernel
> internal I/O like XFS log buffer writes they can differ.
> 
> Fix this by open coding bio_for_each_segment and limiting the bvec len
> so that it never crosses over a page alignment boundary in the device
> in addition to the payload boundary already taken care of by
> bio_iter_iovec.
> 
> Fixes: af8b04c63708 ("zram: simplify bvec iteration in __zram_make_request")
> Reported-by: Dusty Mabe <dusty@dustymabe.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH] zram: take device and not only bvec offset into account

Posted by Christoph Hellwig 2 years, 1 month ago

On Sat, Aug 05, 2023 at 04:46:45PM +0900, Sergey Senozhatsky wrote:
> > Fixes: af8b04c63708 ("zram: simplify bvec iteration in __zram_make_request")
> > Reported-by: Dusty Mabe <dusty@dustymabe.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Acked-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Btw, are there any interesting test suites you want me to run on
a > 4K page size system now that I do have this setup available?

Re: [PATCH] zram: take device and not only bvec offset into account

Posted by Sergey Senozhatsky 2 years, 1 month ago

On (23/08/05 10:13), Christoph Hellwig wrote:
> On Sat, Aug 05, 2023 at 04:46:45PM +0900, Sergey Senozhatsky wrote:
> > > Fixes: af8b04c63708 ("zram: simplify bvec iteration in __zram_make_request")
> > > Reported-by: Dusty Mabe <dusty@dustymabe.com>
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > Acked-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> Btw, are there any interesting test suites you want me to run on
> a > 4K page size system now that I do have this setup available?

I don't really have any special tests. I used to run fio, but switched
to a shell script that:

1) configures zram0 and adds zram1 as writeback
2) mkfs.ext4 on zram0, cp linux tar.gz, compile (in parallel)
3) deferred recompress (idle and size based)
4) idle writeback
5) re-reads all writtenback pages

I test on a system with 4K pages, tho, I probably need to get an image
with larger PAGE_SIZE.

Re: [PATCH] zram: take device and not only bvec offset into account

Posted by Dusty Mabe 2 years, 1 month ago


On 8/5/23 04:13, Christoph Hellwig wrote:
> On Sat, Aug 05, 2023 at 04:46:45PM +0900, Sergey Senozhatsky wrote:
>>> Fixes: af8b04c63708 ("zram: simplify bvec iteration in __zram_make_request")
>>> Reported-by: Dusty Mabe <dusty@dustymabe.com>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> Acked-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> Btw, are there any interesting test suites you want me to run on
> a > 4K page size system now that I do have this setup available?

The patch is passing tests for me. I ran the Fedora CoreOS root reprovision tests
(which are the tests that caught this bug to begin with) and the trivial reproducer:

```
#!/bin/bash
set -eux -o pipefail
modprobe zram num_devices=0
read dev < /sys/class/zram-control/hot_add
echo 10G > /sys/block/zram"${dev}"/disksize
mkfs.xfs /dev/zram"${dev}"
mkdir -p /tmp/foo
mount -t xfs /dev/zram"${dev}" /tmp/foo
```

Dusty