[PATCH] raid0: data corruption when using trim

[PATCH] raid0: data corruption when using trim

[Seunguk Shin]

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 <seunguk.shin@samsung.com>
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(-)

diff --git a/block/bio.c b/block/bio.c
index 2a00d34..df7589d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -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
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index efb654e..fd1318b 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -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;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 5e963a6..8344bb5 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -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)
-- 
1.9.1



--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] raid0: data corruption when using trim

[Martin K. Petersen]

>>>>> "Seunguk" == Seunguk Shin <seunguk.shin@samsung.com> 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!

-- 
Martin K. Petersen	Oracle Linux Engineering

commit 779e6b55da74108460baa8194d82806c4d7db523
Author: Martin K. Petersen <martin.petersen@oracle.com>
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 <martin.petersen@oracle.com>
    Reported-by: Seunguk Shin <seunguk.shin@samsung.com>
    Cc: Jens Axboe <axboe@fb.com>
    Cc: Kent Overstreet <kent.overstreet@gmail.com>
    Cc: <stable@vger.kernel.org> # v3.14+

diff --git a/block/bio.c b/block/bio.c
index 2a00d349cd68..616b0e6f910a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -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;
 

Re: [PATCH] raid0: data corruption when using trim

[Piergiorgio Sartor]

On Mon, Jul 20, 2015 at 08:33:35AM -0400, Martin K. Petersen wrote:
> >>>>> "Seunguk" == Seunguk Shin <seunguk.shin@samsung.com> 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,

-- 

piergiorgio

Re: [PATCH] raid0: data corruption when using trim

[Martin K. Petersen]

>>>>> "Piergiorgio" == Piergiorgio Sartor <piergiorgio.sartor@nexgo.de> 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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] raid0: data corruption when using trim

[Piergiorgio Sartor]

On Mon, Jul 20, 2015 at 02:26:10PM -0400, Martin K. Petersen wrote:
> >>>>> "Piergiorgio" == Piergiorgio Sartor <piergiorgio.sartor@nexgo.de> 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,

-- 

piergiorgio

RE: [PATCH] raid0: data corruption when using trim

[Seunguk Shin]

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: linux-raid-owner@vger.kernel.org
[mailto:linux-raid-owner@vger.kernel.org] On Behalf Of Martin K. Petersen
Sent: Monday, July 20, 2015 9:34 PM
To: Seunguk Shin
Cc: neilb@suse.de; linux-raid@vger.kernel.org
Subject: Re: [PATCH] raid0: data corruption when using trim

>>>>> "Seunguk" == Seunguk Shin <seunguk.shin@samsung.com> 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!

-- 
Martin K. Petersen	Oracle Linux Engineering

commit 779e6b55da74108460baa8194d82806c4d7db523
Author: Martin K. Petersen <martin.petersen@oracle.com>
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 <martin.petersen@oracle.com>
    Reported-by: Seunguk Shin <seunguk.shin@samsung.com>
    Cc: Jens Axboe <axboe@fb.com>
    Cc: Kent Overstreet <kent.overstreet@gmail.com>
    Cc: <stable@vger.kernel.org> # v3.14+

diff --git a/block/bio.c b/block/bio.c
index 2a00d349cd68..616b0e6f910a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -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 majordomo@vger.kernel.org More majordomo info at
http://vger.kernel.org/majordomo-info.html

Re: [PATCH] raid0: data corruption when using trim

[Martin K. Petersen]

>>>>> "Piergiorgio" == Piergiorgio Sartor <piergiorgio.sartor@nexgo.de> writes:

Piergiorgio> There is LVM on top, I wonder if this makes a difference.

It does not.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] raid0: data corruption when using trim

[Martin K. Petersen]

>>>>> "Seunguk" == Seunguk Shin <seunguk.shin@samsung.com> 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

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH] raid0: data corruption when using trim

[Seunguk Shin]

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

Re: [PATCH] raid0: data corruption when using trim

[Martin K. Petersen]

>>>>> "Seunguk" == Seunguk Shin <seunguk.shin@samsung.com> 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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] raid0: data corruption when using trim

[Gionatan Danti]

>
> 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.

-- 
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8

Re: [PATCH] raid0: data corruption when using trim

[Martin K. Petersen]

>>>>> "Gionatan" == Gionatan Danti <g.danti@assyoma.it> 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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] raid0: data corruption when using trim

[Gionatan Danti]

>
> 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! :)

-- 
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8

Re: [PATCH] raid0: data corruption when using trim

[Gionatan Danti]

> 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.

-- 
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8

Re: [PATCH] raid0: data corruption when using trim

[Gionatan Danti]

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.


On 24/07/15 14:19, Seunguk Shin wrote:
> 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 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.
>>
>> --
>> Danti Gionatan
>> Supporto Tecnico
>> Assyoma S.r.l. - www.assyoma.it
>> email: g.danti@assyoma.it - info@assyoma.it GPG public key ID: FF5F32A8
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org More majordomo info at
>> http://vger.kernel.org/majordomo-info.html
>

-- 
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8

Re: [PATCH] raid0: data corruption when using trim

[Martin K. Petersen]

>>>>> "Gionatan" == Gionatan Danti <g.danti@assyoma.it> writes:

Gionatan> Hi, any idea on why the bug affects/manifests only on specific
Gionatan> SATA SSDs?

Timing and a very heavy discard load.

-- 
Martin K. Petersen	Oracle Linux Engineering