From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 Sender: yuru.hong@gmail.com In-Reply-To: <4E4A1069.606@kernel.dk> References: <1313450590-22069-1-git-send-email-egouriou@google.com> <4E4A1069.606@kernel.dk> Date: Mon, 22 Aug 2011 13:04:04 -0400 Message-ID: Subject: Re: [PATCH] stats: fix io_u_plat out-of-bound accesses (round 2) From: Yu-Ju Hong Content-Type: multipart/alternative; boundary=20cf300e4f4bfcbcfa04ab1b1004 To: Jens Axboe Cc: Eric Gouriou , Zhu Yanhai , fio@vger.kernel.org, Nauman Rafique List-ID: --20cf300e4f4bfcbcfa04ab1b1004 Content-Type: text/plain; charset=ISO-8859-1 Hi Jens, Zhu, Sorry for all the troubles I caused for not checking the differences between the Google internal version and the external version of fio. I couldn't respond last week because I was out of town and did not have Internet access nor my laptop last week. Also, my google.com account had been suspended after I finished my internship, so I am currently using my gmail.com account (which explains why the email got bounced). Thank you for discovering and fixing the problem, and sorry again! Eric, Thank you very much for the follow-up! Regards, Yu-Ju On Tue, Aug 16, 2011 at 2:38 AM, Jens Axboe wrote: > On 2011-08-16 01:23, Eric Gouriou wrote: > > Commit 833491908a1afd67 introduced the ability to report completion > > latency percentiles. It also caused a memory corruption when running > > with multiple threads due to out of bound accesses in show_run_stats(). > > The major index of the io_u_plat two-dimensional array is meant > > to be DDIR_ value in {DDIR_READ, DDIR_WRITE} (i.e., {0, 1}). The > > code in show_run_stats() incorrectly wrote into the array using > > a major index with values {0, 1, 2}. Commit 0a0b49007cbce8d1 fixed > > the out of bound accesses by increasing the size of the major > > dimension of the io_u_plat array from 2 to 3. > > > > This patch reverts the size change from 0a0b49007cbce8d1 in favor > > of avoiding the out-of-bound accesses in show_run_stats(). > > This makes more sense, I didn't look carefully enough at this, the 3rd > index is usually for trim. > > > Jens, Zhu, > > > > Yu-Ju is unlikely to be checking fio email traffic this week, > > hence my follow-up. The error was introduced while porting > > the patch between different versions of fio. The internal version > > was tested appropriately but not the upstream version. > > > > Apologies for the trouble. > > No worries, thanks for following up on this so quickly. My group replies > bounced on Yu-Ju's email. > > -- > Jens Axboe > > --20cf300e4f4bfcbcfa04ab1b1004 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Hi Jens, Zhu,

Sorry for all the troubles I caused for no= t checking the differences between the Google internal version and the exte= rnal version of fio. I couldn't respond last week because I was out of = town and did not have Internet access nor my laptop last week. Also, my google.com account had been suspended after = I finished my internship, so I am currently using my gmail.com account (which explains why the email got bounced). Th= ank you for discovering and fixing the problem, and sorry again!

Eric,

Thank you very much for = the follow-up!

Regards,
Yu-Ju

=
On Tue, Aug 16, 2011 at 2:38 AM, Jen= s Axboe <axboe@kern= el.dk> wrote:
On 2011-08-16 01:23, Eric Gouriou wrote:
> Commit 833491908a1afd67 introduced the ability to report completion > latency percentiles. It also caused a memory corruption when running > with multiple threads due to out of bound accesses in show_run_stats()= .
> The major index of the io_u_plat two-dimensional array is meant
> to be DDIR_ value in {DDIR_READ, DDIR_WRITE} (i.e., {0, 1}). The
> code in show_run_stats() incorrectly wrote into the array using
> a major index with values {0, 1, 2}. Commit 0a0b49007cbce8d1 fixed
> the out of bound accesses by increasing the size of the major
> dimension of the io_u_plat array from 2 to 3.
>
> This patch reverts the size change from 0a0b49007cbce8d1 in favor
> of avoiding the out-of-bound accesses in show_run_stats().

This makes more sense, I didn't look carefully enough at this, th= e 3rd
index is usually for trim.

> Jens, Zhu,
>
> Yu-Ju is unlikely to be checking fio email traffic this week,
> hence my follow-up. The error was introduced while porting
> the patch between different versions of fio. The internal version
> was tested appropriately but not the upstream version.
>
> Apologies for the trouble.

No worries, thanks for following up on this so quickly. My group repl= ies
bounced on Yu-Ju's email.

--
Jens Axboe


--20cf300e4f4bfcbcfa04ab1b1004-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [95.166.99.235] ([95.166.99.235]:45863 "EHLO kernel.dk" rhost-flags-FAIL-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751143Ab1HPGif (ORCPT ); Tue, 16 Aug 2011 02:38:35 -0400 Message-ID: <4E4A1069.606@kernel.dk> Date: Tue, 16 Aug 2011 08:38:33 +0200 From: Jens Axboe MIME-Version: 1.0 Subject: Re: [PATCH] stats: fix io_u_plat out-of-bound accesses (round 2) References: <1313450590-22069-1-git-send-email-egouriou@google.com> In-Reply-To: <1313450590-22069-1-git-send-email-egouriou@google.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: fio-owner@vger.kernel.org List-Id: fio@vger.kernel.org To: Eric Gouriou Cc: Zhu Yanhai , Yu-Ju Hong , fio@vger.kernel.org, Nauman Rafique On 2011-08-16 01:23, Eric Gouriou wrote: > Commit 833491908a1afd67 introduced the ability to report completion > latency percentiles. It also caused a memory corruption when running > with multiple threads due to out of bound accesses in show_run_stats(). > The major index of the io_u_plat two-dimensional array is meant > to be DDIR_ value in {DDIR_READ, DDIR_WRITE} (i.e., {0, 1}). The > code in show_run_stats() incorrectly wrote into the array using > a major index with values {0, 1, 2}. Commit 0a0b49007cbce8d1 fixed > the out of bound accesses by increasing the size of the major > dimension of the io_u_plat array from 2 to 3. > > This patch reverts the size change from 0a0b49007cbce8d1 in favor > of avoiding the out-of-bound accesses in show_run_stats(). This makes more sense, I didn't look carefully enough at this, the 3rd index is usually for trim. > Jens, Zhu, > > Yu-Ju is unlikely to be checking fio email traffic this week, > hence my follow-up. The error was introduced while porting > the patch between different versions of fio. The internal version > was tested appropriately but not the upstream version. > > Apologies for the trouble. No worries, thanks for following up on this so quickly. My group replies bounced on Yu-Ju's email. -- Jens Axboe From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Eric Gouriou Subject: [PATCH] stats: fix io_u_plat out-of-bound accesses (round 2) Date: Mon, 15 Aug 2011 16:23:10 -0700 Message-Id: <1313450590-22069-1-git-send-email-egouriou@google.com> To: Jens Axboe , Zhu Yanhai Cc: Yu-Ju Hong , fio@vger.kernel.org, Nauman Rafique , Eric Gouriou List-ID: Commit 833491908a1afd67 introduced the ability to report completion latency percentiles. It also caused a memory corruption when running with multiple threads due to out of bound accesses in show_run_stats(). The major index of the io_u_plat two-dimensional array is meant to be DDIR_ value in {DDIR_READ, DDIR_WRITE} (i.e., {0, 1}). The code in show_run_stats() incorrectly wrote into the array using a major index with values {0, 1, 2}. Commit 0a0b49007cbce8d1 fixed the out of bound accesses by increasing the size of the major dimension of the io_u_plat array from 2 to 3. This patch reverts the size change from 0a0b49007cbce8d1 in favor of avoiding the out-of-bound accesses in show_run_stats(). Signed-off-by: Eric Gouriou --- Jens, Zhu, Yu-Ju is unlikely to be checking fio email traffic this week, hence my follow-up. The error was introduced while porting the patch between different versions of fio. The internal version was tested appropriately but not the upstream version. Apologies for the trouble. Regards - Eric --- fio.h | 2 +- stat.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/fio.h b/fio.h index c741162..6c57496 100644 --- a/fio.h +++ b/fio.h @@ -217,7 +217,7 @@ struct thread_stat { unsigned int io_u_complete[FIO_IO_U_MAP_NR]; unsigned int io_u_lat_u[FIO_IO_U_LAT_U_NR]; unsigned int io_u_lat_m[FIO_IO_U_LAT_M_NR]; - unsigned int io_u_plat[3][FIO_IO_U_PLAT_NR]; + unsigned int io_u_plat[2][FIO_IO_U_PLAT_NR]; unsigned long total_io_u[3]; unsigned long short_io_u[3]; unsigned long total_submit; diff --git a/stat.c b/stat.c index ee6ee51..ae3c71a 100644 --- a/stat.c +++ b/stat.c @@ -773,11 +773,12 @@ void show_run_stats(void) for (k = 0; k <= 2; k++) { - int m; - ts->total_io_u[k] += td->ts.total_io_u[k]; ts->short_io_u[k] += td->ts.short_io_u[k]; + } + for (k = 0; k <= DDIR_WRITE; k++) { + int m; for (m = 0; m < FIO_IO_U_PLAT_NR; m++) ts->io_u_plat[k][m] += td->ts.io_u_plat[k][m]; } -- 1.7.3.1