Message-ID: <1c8296f3.647.1976d2a57fc.Coremail.zhengqixing@huaweicloud.com>
Date: Sat, 14 Jun 2025 14:39:41 +0800 (GMT+08:00)
From: zhengqixing@...weicloud.com
To: "Paul Menzel" <pmenzel@...gen.mpg.de>
Cc: song@...nel.org, yukuai3@...wei.com, linux-raid@...r.kernel.org, 
	linux-kernel@...r.kernel.org, yi.zhang@...wei.com, 
	yangerkun@...wei.com, zhengqixing@...wei.com
Subject: Re: [PATCH] md/raid1,raid10: fix IO handle for REQ_NOWAIT

Hello Paul,


在 2025/6/13 16:02, Paul Menzel 写道:
&gt; Dear Zheng,
&gt;
&gt;
&gt; Thank you for the patch.
&gt;
&gt; Am 12.06.25 um 15:21 schrieb Zheng Qixing:
&gt;&gt; From: Zheng Qixing <zhengqixing@...wei.com>
&gt;&gt;
&gt;&gt; IO with REQ_NOWAIT should not set R1BIO_Uptodate when it fails,
&gt;&gt; and bad blocks should also be cleared when REQ_NOWAIT IO succeeds.
&gt;
&gt; It’d be great if you could add an explanation for the *should*. Why 
&gt; should it not be done?
&gt;
&gt; Do you have a reproducer for this?
&gt;

If we set R1BIO_Uptodate when IO with REQ_NOWAIT fails, the request will 
return a success.

But actually it should return BLK_STS_IOERR or BLK_STS_AGAIN, right?


&gt;&gt; Fixes: 9f346f7d4ea7 ("md/raid1,raid10: don't handle IO error for 
&gt;&gt; REQ_RAHEAD and REQ_NOWAIT")
&gt;&gt; Signed-off-by: Zheng Qixing <zhengqixing@...wei.com>
&gt;&gt; ---
&gt;&gt; &nbsp; drivers/md/raid1.c&nbsp; | 11 ++++++-----
&gt;&gt; &nbsp; drivers/md/raid10.c |&nbsp; 9 +++++----
&gt;&gt; &nbsp; 2 files changed, 11 insertions(+), 9 deletions(-)
&gt;&gt;
&gt;&gt; diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
&gt;&gt; index 19c5a0ce5a40..a1cddd24b178 100644
&gt;&gt; --- a/drivers/md/raid1.c
&gt;&gt; +++ b/drivers/md/raid1.c
&gt;&gt; @@ -455,13 +455,13 @@ static void raid1_end_write_request(struct bio 
&gt;&gt; *bio)
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; struct md_rdev *rdev = conf-&gt;mirrors[mirror].rdev;
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; sector_t lo = r1_bio-&gt;sector;
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; sector_t hi = r1_bio-&gt;sector + r1_bio-&gt;sectors;
&gt;&gt; -&nbsp;&nbsp;&nbsp; bool ignore_error = !raid1_should_handle_error(bio) ||
&gt;&gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (bio-&gt;bi_status &amp;&amp; bio_op(bio) == REQ_OP_DISCARD);
&gt;&gt; +&nbsp;&nbsp;&nbsp; bool discard_error = bio-&gt;bi_status &amp;&amp; bio_op(bio) == 
&gt;&gt; REQ_OP_DISCARD;
&gt;
&gt; Excuse my ignorance. What is the difference between ignore and discard?


REQ_OP_DISCARD is a operation type while REQ_NOWAIT is just a request flag.

These two can be combined together. IO with REQ_NOWAIT can fail early, even

though the storage medium is fine. So, we better handle this type of 
error specially.


I hope this clarifies your doubts.


&gt;
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; /*
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; * 'one mirror IO has finished' event handler:
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; */
&gt;&gt; -&nbsp;&nbsp;&nbsp; if (bio-&gt;bi_status &amp;&amp; !ignore_error) {
&gt;&gt; +&nbsp;&nbsp;&nbsp; if (bio-&gt;bi_status &amp;&amp; !discard_error &amp;&amp;
&gt;&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; raid1_should_handle_error(bio)) {
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; set_bit(WriteErrorSeen,&nbsp;&nbsp;&nbsp; &amp;rdev-&gt;flags);
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; if (!test_and_set_bit(WantReplacement, &amp;rdev-&gt;flags))
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; set_bit(MD_RECOVERY_NEEDED, &amp;
&gt;&gt; @@ -507,12 +507,13 @@ static void raid1_end_write_request(struct bio 
&gt;&gt; *bio)
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; * check this here.
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; */
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; if (test_bit(In_sync, &amp;rdev-&gt;flags) &amp;&amp;
&gt;&gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !test_bit(Faulty, &amp;rdev-&gt;flags))
&gt;&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !test_bit(Faulty, &amp;rdev-&gt;flags) &amp;&amp;
&gt;&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (!bio-&gt;bi_status || discard_error))
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; set_bit(R1BIO_Uptodate, &amp;r1_bio-&gt;state);
&gt;&gt; &nbsp; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; /* Maybe we can clear some bad blocks. */
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; if (rdev_has_badblock(rdev, r1_bio-&gt;sector, 
&gt;&gt; r1_bio-&gt;sectors) &amp;&amp;
&gt;&gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !ignore_error) {
&gt;&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !bio-&gt;bi_status) {
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; r1_bio-&gt;bios[mirror] = IO_MADE_GOOD;
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; set_bit(R1BIO_MadeGood, &amp;r1_bio-&gt;state);
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; }
&gt;&gt; diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
&gt;&gt; index b74780af4c22..1848947b0a6d 100644
&gt;&gt; --- a/drivers/md/raid10.c
&gt;&gt; +++ b/drivers/md/raid10.c
&gt;&gt; @@ -458,8 +458,8 @@ static void raid10_end_write_request(struct bio 
&gt;&gt; *bio)
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; int slot, repl;
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; struct md_rdev *rdev = NULL;
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; struct bio *to_put = NULL;
&gt;&gt; -&nbsp;&nbsp;&nbsp; bool ignore_error = !raid1_should_handle_error(bio) ||
&gt;&gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (bio-&gt;bi_status &amp;&amp; bio_op(bio) == REQ_OP_DISCARD);
&gt;&gt; +&nbsp;&nbsp;&nbsp; bool discard_error = bio-&gt;bi_status &amp;&amp; bio_op(bio) == 
&gt;&gt; REQ_OP_DISCARD;
&gt;&gt; +&nbsp;&nbsp;&nbsp; bool ignore_error = !raid1_should_handle_error(bio) || 
&gt;&gt; discard_error;
&gt;&gt; &nbsp; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; dev = find_bio_disk(conf, r10_bio, bio, &amp;slot, &amp;repl);
&gt;&gt; &nbsp; @@ -522,13 +522,14 @@ static void raid10_end_write_request(struct 
&gt;&gt; bio *bio)
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; * check this here.
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; */
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; if (test_bit(In_sync, &amp;rdev-&gt;flags) &amp;&amp;
&gt;&gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !test_bit(Faulty, &amp;rdev-&gt;flags))
&gt;&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !test_bit(Faulty, &amp;rdev-&gt;flags) &amp;&amp;
&gt;&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (!bio-&gt;bi_status || discard_error))
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; set_bit(R10BIO_Uptodate, &amp;r10_bio-&gt;state);
&gt;&gt; &nbsp; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; /* Maybe we can clear some bad blocks. */
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; if (rdev_has_badblock(rdev, r10_bio-&gt;devs[slot].addr,
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; r10_bio-&gt;sectors) &amp;&amp;
&gt;&gt; -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !ignore_error) {
&gt;&gt; +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; !bio-&gt;bi_status) {
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; bio_put(bio);
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; if (repl)
&gt;&gt; &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; r10_bio-&gt;devs[slot].repl_bio = IO_MADE_GOOD;
&gt;
&gt;
&gt; Kind regards,
&gt;
&gt; Paul


Regards,

Zheng

</zhengqixing@...wei.com></zhengqixing@...wei.com>

Powered by blists - more mailing lists