| Message ID | 20250304104159.19102-1-xni@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] md/raid10: wait barrier before returning discard request with REQ_NOWAIT | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_14-PR | success | PR summary |
| mdraidci/vmtest-md-6_14-VM_Test-0 | success | Logs for per-patch-testing |
Dear Xiao, Thank you for your patch. A minor thing, I’d add a verb to the summary/title: > Add wait barrier before … Am 04.03.25 um 11:41 schrieb Xiao Ni: > raid10_handle_discard should wait barrier before returning a discard bio > which has REQ_NOWAIT. And there is no need to print warning calltrace > if a discard bio has REQ_NOWAIT flag. Quality engineer usually checks > dmesg and reports error if dmesg has warning/error calltrace. As written in the other thread, please add, why the warning is not useful. Somebody added that warning probably with some reason. > Signed-off-by: Xiao Ni <xni@redhat.com> > --- > drivers/md/raid10.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index 15b9ae5bf84d..7bbc04522f26 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -1631,11 +1631,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio) > if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)) > return -EAGAIN; > > - if (WARN_ON_ONCE(bio->bi_opf & REQ_NOWAIT)) { > + if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) { > bio_wouldblock_error(bio); > return 0; > } > - wait_barrier(conf, false); > > /* > * Check reshape again to avoid reshape happens after checking Kind regards, Paul
On Tue, Mar 4, 2025 at 7:03 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Xiao, > > > Thank you for your patch. A minor thing, I’d add a verb to the > summary/title: Thanks for pointing out this. > > > Add wait barrier before … > > Am 04.03.25 um 11:41 schrieb Xiao Ni: > > raid10_handle_discard should wait barrier before returning a discard bio > > which has REQ_NOWAIT. And there is no need to print warning calltrace > > if a discard bio has REQ_NOWAIT flag. Quality engineer usually checks > > dmesg and reports error if dmesg has warning/error calltrace. > > As written in the other thread, please add, why the warning is not > useful. Somebody added that warning probably with some reason. For me, it's overkilled to print a warning calltrace if one bio has REQ_NOWAIT flag. It's a normal request rather than a dangerous thing happens, right? If we want to print some logs, we can use pr_info rather than WARN_ON_ONCE. Best Regards Xiao > > > Signed-off-by: Xiao Ni <xni@redhat.com> > > --- > > drivers/md/raid10.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > > index 15b9ae5bf84d..7bbc04522f26 100644 > > --- a/drivers/md/raid10.c > > +++ b/drivers/md/raid10.c > > @@ -1631,11 +1631,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio) > > if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)) > > return -EAGAIN; > > > > - if (WARN_ON_ONCE(bio->bi_opf & REQ_NOWAIT)) { > > + if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) { > > bio_wouldblock_error(bio); > > return 0; > > } > > - wait_barrier(conf, false); > > > > /* > > * Check reshape again to avoid reshape happens after checking > > > Kind regards, > > Paul >
Hi, 在 2025/03/04 20:22, Xiao Ni 写道: > On Tue, Mar 4, 2025 at 7:03 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: >> >> Dear Xiao, >> >> >> Thank you for your patch. A minor thing, I’d add a verb to the >> summary/title: > > Thanks for pointing out this. > >> >>> Add wait barrier before … >> >> Am 04.03.25 um 11:41 schrieb Xiao Ni: >>> raid10_handle_discard should wait barrier before returning a discard bio >>> which has REQ_NOWAIT. And there is no need to print warning calltrace >>> if a discard bio has REQ_NOWAIT flag. Quality engineer usually checks >>> dmesg and reports error if dmesg has warning/error calltrace. >> >> As written in the other thread, please add, why the warning is not >> useful. Somebody added that warning probably with some reason. > > For me, it's overkilled to print a warning calltrace if one bio has > REQ_NOWAIT flag. It's a normal request rather than a dangerous thing > happens, right? If we want to print some logs, we can use pr_info > rather than WARN_ON_ONCE. Just take a look at block layer and other drivers, there is no such warn in this case. And I think you can add a fixtag: c9aa889b035f ("md: raid10 add nowait support") This commit just forbid discard with REQ_NOWAIT in raid10. BTW, I think the abouve checking can be removed as well: if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)) return -EAGAIN; Thanks, Kuai > > Best Regards > Xiao > >> >>> Signed-off-by: Xiao Ni <xni@redhat.com> >>> --- >>> drivers/md/raid10.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c >>> index 15b9ae5bf84d..7bbc04522f26 100644 >>> --- a/drivers/md/raid10.c >>> +++ b/drivers/md/raid10.c >>> @@ -1631,11 +1631,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio) >>> if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)) >>> return -EAGAIN; >>> >>> - if (WARN_ON_ONCE(bio->bi_opf & REQ_NOWAIT)) { >>> + if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) { >>> bio_wouldblock_error(bio); >>> return 0; >>> } >>> - wait_barrier(conf, false); >>> >>> /* >>> * Check reshape again to avoid reshape happens after checking >> >> >> Kind regards, >> >> Paul >> > > > . >
On Tue, Mar 4, 2025 at 8:34 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2025/03/04 20:22, Xiao Ni 写道: > > On Tue, Mar 4, 2025 at 7:03 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > >> > >> Dear Xiao, > >> > >> > >> Thank you for your patch. A minor thing, I’d add a verb to the > >> summary/title: > > > > Thanks for pointing out this. > > > >> > >>> Add wait barrier before … > >> > >> Am 04.03.25 um 11:41 schrieb Xiao Ni: > >>> raid10_handle_discard should wait barrier before returning a discard bio > >>> which has REQ_NOWAIT. And there is no need to print warning calltrace > >>> if a discard bio has REQ_NOWAIT flag. Quality engineer usually checks > >>> dmesg and reports error if dmesg has warning/error calltrace. > >> > >> As written in the other thread, please add, why the warning is not > >> useful. Somebody added that warning probably with some reason. > > > > For me, it's overkilled to print a warning calltrace if one bio has > > REQ_NOWAIT flag. It's a normal request rather than a dangerous thing > > happens, right? If we want to print some logs, we can use pr_info > > rather than WARN_ON_ONCE. > > Just take a look at block layer and other drivers, there is no such > warn in this case. And I think you can add a fixtag: > > c9aa889b035f ("md: raid10 add nowait support") > > This commit just forbid discard with REQ_NOWAIT in raid10. Ok. > > BTW, I think the abouve checking can be removed as well: > > if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)) > return -EAGAIN; Now we can't remove it. raid10_handle_discard doesn't consider the reshape situation. So raid10 will use the old way to handle discard if reshape happens. Regards Xiao > > Thanks, > Kuai > > > > > Best Regards > > Xiao > > > >> > >>> Signed-off-by: Xiao Ni <xni@redhat.com> > >>> --- > >>> drivers/md/raid10.c | 3 +-- > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > >>> index 15b9ae5bf84d..7bbc04522f26 100644 > >>> --- a/drivers/md/raid10.c > >>> +++ b/drivers/md/raid10.c > >>> @@ -1631,11 +1631,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio) > >>> if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)) > >>> return -EAGAIN; > >>> > >>> - if (WARN_ON_ONCE(bio->bi_opf & REQ_NOWAIT)) { > >>> + if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) { > >>> bio_wouldblock_error(bio); > >>> return 0; > >>> } > >>> - wait_barrier(conf, false); > >>> > >>> /* > >>> * Check reshape again to avoid reshape happens after checking > >> > >> > >> Kind regards, > >> > >> Paul > >> > > > > > > . > > >
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 15b9ae5bf84d..7bbc04522f26 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -1631,11 +1631,10 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio) if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery)) return -EAGAIN; - if (WARN_ON_ONCE(bio->bi_opf & REQ_NOWAIT)) { + if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) { bio_wouldblock_error(bio); return 0; } - wait_barrier(conf, false); /* * Check reshape again to avoid reshape happens after checking
raid10_handle_discard should wait barrier before returning a discard bio which has REQ_NOWAIT. And there is no need to print warning calltrace if a discard bio has REQ_NOWAIT flag. Quality engineer usually checks dmesg and reports error if dmesg has warning/error calltrace. Signed-off-by: Xiao Ni <xni@redhat.com> --- drivers/md/raid10.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)