From: Chen Cheng <[email protected]>
zero_exception() zeroes one chunk of the COW device and takes the start
sector from bio->bi_iter.bi_sector. remap_exception() has already set
that to the COW chunk start plus the offset inside the chunk. That
offset is needed for reads and writes, but for a discard it is wrong:
the zeroing window is shifted, so the head of the target chunk keeps
its old data and the tail lands in the next COW chunk, which holds a
different exception.
io_overlaps_chunk() should have caught this, but it only compares the
length. "maps to entire chunks" in snapshot.rst also means the discard
has to start on a chunk boundary.
A discard that is not a full chunk has a second problem: after
remap_exception() it is sent to the COW device, where it hits a chunk
that still backs a live exception. After a discard the contents are
undefined, but the exception stays in the table and keeps pointing at
that chunk, so a later read returns whatever the discard left instead
of the snapshot's data.
Reproducer:
truncate -s 4M o.img c.img
o=$(losetup -f --show o.img)
c=$(losetup -f --show c.img)
dmsetup create snap --table \
"0 8192 snapshot $o $c P 512 1 discard_zeroes_cow"
# write two chunks, this creates two exceptions
head -c 256K /dev/zero | tr '\000' '\252' > pat
dd if=pat of=/dev/mapper/snap bs=256K seek=1 count=1 conv=notrunc
dd if=pat of=/dev/mapper/snap bs=256K seek=2 count=1 conv=notrunc
# shift the snapshot by 8 sectors, so a discard that is aligned on
# top is unaligned inside the snapshot
dmsetup create top --table "0 8184 linear /dev/mapper/snap 8"
blkdiscard -o 262144 -l 262144 /dev/mapper/top
sync
echo 3 > /proc/sys/vm/drop_caches
# chunk 1
dd if=/dev/mapper/snap bs=512 skip=512 count=8 | od -An -tx1 -N1
# chunk 2
dd if=/dev/mapper/snap bs=512 skip=1024 count=8 | od -An -tx1 -N1
Before: aa 00 <- chunk 2 was zeroed although it was not discarded
After: aa aa <- nothing written, a discard is only a hint
Fix it by requiring the discard to start on a chunk boundary in
io_overlaps_chunk(), and by finishing a discard that is not a full
chunk with bio_endio() instead of passing it down to the COW device.
Fixes: 2e6023850e17 ("dm snapshot: add optional discard support features")
Cc: [email protected]
Signed-off-by: Chen Cheng <[email protected]>
---
drivers/md/dm-snap.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index e49427bdbeef..6968e806eb84 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -1946,12 +1946,12 @@ static void zero_exception(struct dm_snapshot *s,
struct dm_exception *e,
dm_kcopyd_zero(s->kcopyd_client, 1, &dest, 0, zero_callback, bio);
}
static bool io_overlaps_chunk(struct dm_snapshot *s, struct bio *bio)
{
- return bio->bi_iter.bi_size ==
- (s->store->chunk_size << SECTOR_SHIFT);
+ return bio->bi_iter.bi_size == (s->store->chunk_size << SECTOR_SHIFT) &&
+ !(bio->bi_iter.bi_sector & s->store->chunk_mask);
}
static int snapshot_map(struct dm_target *ti, struct bio *bio)
{
struct dm_exception *e;
@@ -2007,15 +2007,17 @@ static int snapshot_map(struct dm_target *ti, struct
bio *bio)
/* If the block is already remapped - use that, else remap it */
e = dm_lookup_exception(&s->complete, chunk);
if (e) {
remap_exception(s, e, bio, chunk);
- if (unlikely(bio_op(bio) == REQ_OP_DISCARD) &&
- io_overlaps_chunk(s, bio)) {
+ if (unlikely(bio_op(bio) == REQ_OP_DISCARD)) {
dm_exception_table_unlock(&lock);
up_read(&s->lock);
- zero_exception(s, e, bio, chunk);
+ if (io_overlaps_chunk(s, bio))
+ zero_exception(s, e, bio, chunk);
+ else
+ bio_endio(bio);
r = DM_MAPIO_SUBMITTED; /* discard is not issued */
goto out;
}
goto out_unlock;
}
--
2.56.0
