raid0: data corruption when using trim
Subsystems (2)
PATCH 1/1 raid0: data corruption when using trim
Hi,
There was report at Algolia's blog website regarding some problems using software raid and trim in SSD.
https://blog.algolia.com/when-solid-state-drives-are-not-that-solid/
It turns out that there is misunderstanding between raid driver and scsi/ata driver.
The raid driver lets split bios share bio vector of source bio. Usually, there is no problem, because the raid layer ensures that the source bio is not freed before the split bios.
But, in case of trim, there are some problems.
The scsi/ata needs some payloads that include start address and size of device to trim.
So, the scsi/ata driver allocates a page and stores that pointer on bio->bi_io_vec->bv_page.
(sd_setup_discard_cmnd)
Because split bios share the source bio's bi_io_vec,
the pointer to the allocated page in scsi/ata driver is overwritten. It leads to memory leakage and data corruption
because the overwritten pointer has wrong address and size to trim on device.
So, we add some codes that make sure not to share bio vector if the source bio is discard.
The linear and raid10 also have same problem
because they're using similar scheme to split the source bio.
This problem exists at the very first time when the trim is enabled on raid0.
Before the new bio_split() (20d0189b), we can patch more easily by modifying the condition to split bio vector in bio_split function from (bi->bi_vec != 0) to ((bi->bi_vec != 0) || (bi->bi_rw & REQ_DISCARD)).
Thank you.
Seunguk Shin.
[“0001-PATCH-raid0-data-corruption-when-using-trim.patch”]
From ca7dbe01fcd2ef2f8cea1a38de5aca5c866c585d Mon Sep 17 00:00:00 2001
From: Seunguk Shin <[email protected]>
Date: Sat, 18 Jul 2015 20:13:44 +0900
Subject: [PATCH] [PATCH] raid0: data corruption when using trim
When we are using software raid and tirm, there is data corruption.
The raid driver lets split bios share bio vector of source bio. The scsi/ata driver allocates a page and stores that pointer on bio->bi_io_vec->bv_page
(sd_setup_discard_cmnd) because the scsi/ata needs some payloads that include start address and size of device to trim. Because split bios share the source bio's bi_io_vec,
the pointer to the allocated page in scsi/ata driver is overwritten.
This patch splits bio vector if bio is discard.
---
block/bio.c | 6 ------
drivers/md/raid0.c | 13 +++++++++++++include/linux/bio.h | 6 ++++++
3 files changed, 19 insertions(+), 6 deletions(-)
block/bio.c +0 -0
@@ -32,12 +32,6 @@
#include <trace/events/block.h>
/*
- * Test patch to inline a certain number of bi_io_vec's inside the bio
- * itself, to shrink a bio data allocation from two mempool calls to one
- */
-#define BIO_INLINE_VECS 4
-
-/*
* if you change this list, also change bvec_alloc or things will
* break badly! cannot be bigger than what you can fit into an
* unsigned short
drivers/md/raid0.c +0 -0
@@ -529,6 +529,19 @@ static void raid0_make_request(struct mddev *mddev,
struct bio *bio)
if (sectors < bio_sectors(bio)) {
split = bio_split(bio, sectors, GFP_NOIO, fs_bio_set);
+ if (unlikely(split->bi_rw & REQ_DISCARD)) { + struct bio_vec *bvl = NULL; + unsigned long idx = BIO_POOL_NONE; +
+ bvl = bvec_alloc(GFP_NOIO, BIO_INLINE_VECS + 1,
+ &idx, fs_bio_set->bvec_pool);
+ split->bi_flags |= 1 << BIO_OWNS_VEC;
+ split->bi_flags &= ~(BIO_POOL_NONE <<
+BIO_POOL_OFFSET);
+ split->bi_flags |= idx << BIO_POOL_OFFSET;
+ split->bi_max_vecs = BIO_INLINE_VECS + 1;
+ split->bi_io_vec = bvl; + }
bio_chain(split, bio);
} else {
split = bio;
include/linux/bio.h +0 -0
@@ -40,6 +40,12 @@
#define BIO_BUG_ON
#endif
+/*
+ * Test patch to inline a certain number of bi_io_vec's inside the bio
+ * itself, to shrink a bio data allocation from two mempool calls to one
+ */
+#define BIO_INLINE_VECS 4
+
#define BIO_MAX_PAGES 256
#define BIO_MAX_SIZE (BIO_MAX_PAGES << PAGE_CACHE_SHIFT) #define BIO_MAX_SECTORS (BIO_MAX_SIZE >> 9)
Signature · 8 lines
1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Seunguk" == Seunguk Shin <[email protected]> writes:
Seunguk,
Thanks for tracking this down. Instead of explicitly coding around the issue in raid0/raid10/linear I would prefer to fix bio_split(). It seems like a deficiency in the interface that it does not handle this transparently.
Do you have a reproducible test case? If so it would be great if you could try the following patch and let us know the results.
Thank you!
Signature · 20 lines
Martin K. Petersen Oracle Linux Engineering commit 779e6b55da74108460baa8194d82806c4d7db523 Author: Martin K. Petersen <[email protected]> Date: Mon Jul 20 08:05:30 2015 -0400 block: Do a full clone when splitting discard bios Commit 20d0189b1012 "block: Introduce new bio_split()" permits sharing the bio_vec between the two resulting bios. That's fine for read/write requests where the bio_vec is immutable. For discards, however, we need to be able to attach a payload and update the bio_vec so the page can get mapped to a scatterlist entry. Therefore the bio_vec can not be shared when splitting discards and we must do a full clone. Signed-off-by: Martin K. Petersen <[email protected]> Reported-by: Seunguk Shin <[email protected]> Cc: Jens Axboe <[email protected]> Cc: Kent Overstreet <[email protected]> Cc: <[email protected]> # v3.14+
block/bio.c +9 -1
@@ -1842,7 +1842,15 @@ struct bio *bio_split(struct bio *bio, int sectors,BUG_ON (sectors <=0 );BUG_ON (sectors >=bio_sectors (bio)); - split =bio_clone_fast (bio, gfp, bs); +/* +* Discards need a mutable bio_vec to accommodate the payload +* required by the DSM TRIM and UNMAP commands. +*/ +if (bio->bi_rw &REQ_DISCARD ) + split =bio_clone_bioset (bio, gfp, bs); +else + split =bio_clone_fast (bio, gfp, bs); +if (!split)return NULL ;
Quoted 6 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Seunguk" == Seunguk Shin <[email protected]> writes:
Seunguk,
Thanks for tracking this down. Instead of explicitly coding around the issue in raid0/raid10/linear I would prefer to fix
bio_split(). It seems
Does this mean we should disable any trimming
on RAID-10 until further notice?
Thanks,
bye,
Signature · 1 line
piergiorgio
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Piergiorgio" == Piergiorgio Sartor <[email protected]> writes:
Piergiorgio> Does this mean we should disable any trimming on RAID-10 Piergiorgio> until further notice?
If you are using SATA SSDs, absolutely.
This bug has been around for a long time and I'm surprised we haven't heard of it until now. But it's very specific to the way linear, raid0 and raid10 interface with the block layer. I'm guessing that most SSD users deploy raid1 which is not affected.
The problem is also there if you have a storage array that prefers the UNMAP command. But it's even less likely that you'd be using software RAID in that case.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 6 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Piergiorgio" == Piergiorgio Sartor <[email protected]> writes:
Piergiorgio> Does this mean we should disable any trimming on RAID-10 Piergiorgio> until further notice?
If you are using SATA SSDs, absolutely.
There is LVM on top, I wonder if this
makes a difference.
Anyway thanks,
bye,
Signature · 1 line
piergiorgio
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Piergiorgio" == Piergiorgio Sartor <[email protected]> writes:
Piergiorgio> There is LVM on top, I wonder if this makes a difference.
It does not.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 5 lines
Piergiorgio> There is LVM on top, I wonder if this makes a difference.
It does not.
Hi all,
it is my understanding that, when backed by SSDs (and configured for passing down TRIM requests), ThinLVM are prime condidate for trigger the bug.
But what about ThinLVM + MDRAID10 on top of normal (spinning HDD)? Generally SATA HDDs do not support TRIM/UNMAP, but when using ThinLVM, the thin-provision mapper advertise TRIM support, and indeed an "fstrim -v /" succeeds.
I *think* that, as this TRIM command is processed way above the ATA layer (it is processed inside the device-mapper code), the bug should not happen here. Is it correct? Are we safe with ThinLVM + MDRAID10 + HDDs?
Thanks.
Signature · 5 lines
Danti Gionatan Supporto Tecnico Assyoma S.r.l. - www.assyoma.it email: [email protected] - [email protected] GPG public key ID: FF5F32A8
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Gionatan" == Gionatan Danti <[email protected]> writes:
Gionatan> I *think* that, as this TRIM command is processed way above Gionatan> the ATA layer (it is processed inside the device-mapper code), Gionatan> the bug should not happen here. Is it correct? Are we safe Gionatan> with ThinLVM + MDRAID10 + HDDs?
The problem only occurs if you are using MD linear/raid0/raid10 on top of a device that implements either DSM TRIM (SATA) or UNMAP (SCSI). If the device prefers WRITE SAME w/ UNMAP or if it does not support discards at all there is no problem.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 6 lines
The problem only occurs if you are using MD linear/raid0/raid10 on top of a device that implements either DSM TRIM (SATA) or UNMAP (SCSI). If the device prefers WRITE SAME w/ UNMAP or if it does not support discards at all there is no problem.
Thank you for confirmation! :)
Signature · 5 lines
Danti Gionatan Supporto Tecnico Assyoma S.r.l. - www.assyoma.it email: [email protected] - [email protected] GPG public key ID: FF5F32A8
Martin,
Thank you for new patch.
The function bio_split says "The newly allocated bio will point to @bio's bi_io_vec; it is the caller's responsibility to ensure that @bio is not freed before the split." in its comments. I'm not sure whether some caller uses this limitation or not, so I modifid the raid function. If you want to modify bio_split function, you have to modify the comment also.
The patch you shared has no problem, I think. I'll test it and share the result.
I used the script from the algolia blog to reproduce the symptoms. They share it on Github. (https://github.com/algolia/trimtester)
Thank you.
Seunguk Shin
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Martin K. Petersen
Sent: Monday, July 20, 2015 9:34 PM
To: Seunguk Shin
Cc: [email protected]; [email protected]
Subject: Re: [PATCH] raid0: data corruption when using trim
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Seunguk" == Seunguk Shin <[email protected]> writes:
Seunguk,
Thanks for tracking this down. Instead of explicitly coding around the issue in raid0/raid10/linear I would prefer to fix bio_split(). It seems like a deficiency in the interface that it does not handle this transparently.
Do you have a reproducible test case? If so it would be great if you could try the following patch and let us know the results.
Thank you!
Signature · 20 lines
Martin K. Petersen Oracle Linux Engineering commit 779e6b55da74108460baa8194d82806c4d7db523 Author: Martin K. Petersen <[email protected]> Date: Mon Jul 20 08:05:30 2015 -0400 block: Do a full clone when splitting discard bios Commit 20d0189b1012 "block: Introduce new bio_split()" permits sharing the bio_vec between the two resulting bios. That's fine for read/write requests where the bio_vec is immutable. For discards, however, we need to be able to attach a payload and update the bio_vec so the page can get mapped to a scatterlist entry. Therefore the bio_vec can not be shared when splitting discards and we must do a full clone. Signed-off-by: Martin K. Petersen <[email protected]> Reported-by: Seunguk Shin <[email protected]> Cc: Jens Axboe <[email protected]> Cc: Kent Overstreet <[email protected]> Cc: <[email protected]> # v3.14+
block/bio.c +9 -1
@@ -1842,7 +1842,15 @@ struct bio *bio_split(struct bio *bio, int sectors,BUG_ON (sectors <=0 );BUG_ON (sectors >=bio_sectors (bio)); - split =bio_clone_fast (bio, gfp, bs); +/* +* Discards need a mutable bio_vec to accommodate the payload +* required by the DSM TRIM and UNMAP commands. +*/ +if (bio->bi_rw &REQ_DISCARD ) + split =bio_clone_bioset (bio, gfp, bs); +else + split =bio_clone_fast (bio, gfp, bs); +if (!split)return NULL ;
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Seunguk" == Seunguk Shin <[email protected]> writes:
Seunguk,
Seunguk> The function bio_split says "The newly allocated bio will point Seunguk> to @bio's bi_io_vec; it is the caller's responsibility to Seunguk> ensure that @bio is not freed before the split." in its Seunguk> comments.
I already put an explanatory comment above the cloning code. But I'll elaborate in the function description as well.
Seunguk> I'm not sure whether some caller uses this limitation or not, Seunguk> so I modifid the raid function.
We generally make sure our generic functions handle non-read/write commands correctly so that callers do not have to keep track of internal block layer implementation details/idiosyncrasies.
bio_split() exists mainly to provide linear/raid0/raid10 with a fast path for reads and writes. But the function needs to be able to fall back to a full clone in the discard case. The mutable bio_vec requirement is purely an artifact of how things are implemented further down the stack. MD should not have to deal with that.
Seunguk> The patch you shared has no problem, I think. I'll test it and Seunguk> share the result.
That would be much appreciated!
Thanks,
Martin
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Martine,
I have tested the script from Algolia with your patch, I cannot see the same symptoms I saw with original kernel. I think the patch works and there is no problem.
Thank you,
Seunguk Shin
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Seunguk" == Seunguk Shin <[email protected]> writes:
Seunguk,
Seunguk> I have tested the script from Algolia with your patch, I cannot Seunguk> see the same symptoms I saw with original kernel. I think the Seunguk> patch works and there is no problem.
Thank you for your diligent work on this issue! I just submitted the patch.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 9 lines
Martine,
I have tested the script from Algolia with your patch, I cannot see the same symptoms I saw with original kernel. I think the patch works and there is no problem.
Thank you,
Seunguk Shin
Hi, any idea on why the bug affects/manifests only on specific SATA SSDs? It is timing related, or it depends on how the controller manage TRIM requests?
Thanks.
Signature · 5 lines
Danti Gionatan Supporto Tecnico Assyoma S.r.l. - www.assyoma.it email: [email protected] - [email protected] GPG public key ID: FF5F32A8
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Seunguk,
thank you for your further explanation.
Does it means that only driver with Queued TRIM are affected, or it affect all TRIM-capable driver?
What I wounder is that, if it is only a timing-triggered problem, why it is only apparent on specific SSDs? I would expect it to manifest on other devices also, albeit with different frequency.
Sorry if some questions sound dumb, I'm only trying to understand. Thanks.
Quoted 33 lines · Seunguk Shin
Gionatan,
Because it is related with timing.
1st trim is issued and before complete it, 2nd trim is started (allocates memory but is not issued to device)
Then, when 1st trim is completed, it frees 2nd trim's memory, because they share the pointer due to bug.
If 3rd trim is started before 2nd trim is issued to device, 3rd trim can allocate the same memory with 2nd trim, because it is freed when 1st trim is completed.
Only this case makes the corruption.Thank you,
Seunguk ShinQuoted 16 lines
Hi, any idea on why the bug affects/manifests only on specific SATA SSDs? It is timing related, or it depends on how the controller manage TRIM requests?
Thanks.
--
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: [email protected] - [email protected] GPG public key ID: FF5F32A8 --
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html
Signature · 5 lines
Danti Gionatan Supporto Tecnico Assyoma S.r.l. - www.assyoma.it email: [email protected] - [email protected] GPG public key ID: FF5F32A8
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Gionatan" == Gionatan Danti <[email protected]> writes:
Gionatan> Hi, any idea on why the bug affects/manifests only on specific Gionatan> SATA SSDs?
Timing and a very heavy discard load.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering