diff mbox series

[v4] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

Message ID 20230713135413.2946622-1-houtao@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series [v4] virtio_pmem: add the missing REQ_OP_WRITE for flush bio | expand

Commit Message

Hou Tao July 13, 2023, 1:54 p.m. UTC 
From: Hou Tao <houtao1@huawei.com>

When doing mkfs.xfs on a pmem device, the following warning was
reported:

 ------------[ cut here ]------------
 WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct
 Modules linked in:
 CPU: 2 PID: 384 Comm: mkfs.xfs Not tainted 6.4.0-rc7+ #154
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 RIP: 0010:submit_bio_noacct+0x340/0x520
 ......
 Call Trace:
  <TASK>
  ? submit_bio_noacct+0xd5/0x520
  submit_bio+0x37/0x60
  async_pmem_flush+0x79/0xa0
  nvdimm_flush+0x17/0x40
  pmem_submit_bio+0x370/0x390
  __submit_bio+0xbc/0x190
  submit_bio_noacct_nocheck+0x14d/0x370
  submit_bio_noacct+0x1ef/0x520
  submit_bio+0x55/0x60
  submit_bio_wait+0x5a/0xc0
  blkdev_issue_flush+0x44/0x60

The root cause is that submit_bio_noacct() needs bio_op() is either
WRITE or ZONE_APPEND for flush bio and async_pmem_flush() doesn't assign
REQ_OP_WRITE when allocating flush bio, so submit_bio_noacct just fail
the flush bio.

Simply fix it by adding the missing REQ_OP_WRITE for flush bio. And we
could fix the flush order issue and do flush optimization later.

Cc: stable@vger.kernel.org # 6.3+
Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
Tested-by: Pankaj Gupta <pankaj.gupta@amd.com>
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
v4:
 * add stable Cc
 * collect Rvb and Tested-by tags

v3: https://lore.kernel.org/linux-block/20230625022633.2753877-1-houtao@huaweicloud.com
 * adjust the overly long lines in both commit message and code

v2: https://lore.kernel.org/linux-block/20230621134340.878461-1-houtao@huaweicloud.com
 * do a minimal fix first (Suggested by Christoph)

v1: https://lore.kernel.org/linux-block/ZJLpYMC8FgtZ0k2k@infradead.org/T/#t

 drivers/nvdimm/nd_virtio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Pankaj Gupta Aug. 4, 2023, 6:39 p.m. UTC | #1 
Gentle ping!

Dan, Vishal for suggestion/review on this patch and request for merging.
+Cc Michael for awareness, as virtio-pmem device is currently broken.

Thanks,
Pankaj

> From: Hou Tao <houtao1@huawei.com>
>
> When doing mkfs.xfs on a pmem device, the following warning was
> reported:
>
>  ------------[ cut here ]------------
>  WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct
>  Modules linked in:
>  CPU: 2 PID: 384 Comm: mkfs.xfs Not tainted 6.4.0-rc7+ #154
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>  RIP: 0010:submit_bio_noacct+0x340/0x520
>  ......
>  Call Trace:
>   <TASK>
>   ? submit_bio_noacct+0xd5/0x520
>   submit_bio+0x37/0x60
>   async_pmem_flush+0x79/0xa0
>   nvdimm_flush+0x17/0x40
>   pmem_submit_bio+0x370/0x390
>   __submit_bio+0xbc/0x190
>   submit_bio_noacct_nocheck+0x14d/0x370
>   submit_bio_noacct+0x1ef/0x520
>   submit_bio+0x55/0x60
>   submit_bio_wait+0x5a/0xc0
>   blkdev_issue_flush+0x44/0x60
>
> The root cause is that submit_bio_noacct() needs bio_op() is either
> WRITE or ZONE_APPEND for flush bio and async_pmem_flush() doesn't assign
> REQ_OP_WRITE when allocating flush bio, so submit_bio_noacct just fail
> the flush bio.
>
> Simply fix it by adding the missing REQ_OP_WRITE for flush bio. And we
> could fix the flush order issue and do flush optimization later.
>
> Cc: stable@vger.kernel.org # 6.3+
> Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
> Tested-by: Pankaj Gupta <pankaj.gupta@amd.com>
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> v4:
>  * add stable Cc
>  * collect Rvb and Tested-by tags
>
> v3: https://lore.kernel.org/linux-block/20230625022633.2753877-1-houtao@huaweicloud.com
>  * adjust the overly long lines in both commit message and code
>
> v2: https://lore.kernel.org/linux-block/20230621134340.878461-1-houtao@huaweicloud.com
>  * do a minimal fix first (Suggested by Christoph)
>
> v1: https://lore.kernel.org/linux-block/ZJLpYMC8FgtZ0k2k@infradead.org/T/#t
>
>  drivers/nvdimm/nd_virtio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> index c6a648fd8744..1f8c667c6f1e 100644
> --- a/drivers/nvdimm/nd_virtio.c
> +++ b/drivers/nvdimm/nd_virtio.c
> @@ -105,7 +105,8 @@ int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
>          * parent bio. Otherwise directly call nd_region flush.
>          */
>         if (bio && bio->bi_iter.bi_sector != -1) {
> -               struct bio *child = bio_alloc(bio->bi_bdev, 0, REQ_PREFLUSH,
> +               struct bio *child = bio_alloc(bio->bi_bdev, 0,
> +                                             REQ_OP_WRITE | REQ_PREFLUSH,
>                                               GFP_ATOMIC);
>
>                 if (!child)
> --
> 2.29.2
>
Verma, Vishal L Aug. 4, 2023, 9:03 p.m. UTC | #2 
On Fri, 2023-08-04 at 20:39 +0200, Pankaj Gupta wrote:
> Gentle ping!
> 
> Dan, Vishal for suggestion/review on this patch and request for merging.
> +Cc Michael for awareness, as virtio-pmem device is currently broken.

Looks good to me,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Dave, will you queue this for 6.6.

> 
> Thanks,
> Pankaj
> 
> > From: Hou Tao <houtao1@huawei.com>
> > 
> > When doing mkfs.xfs on a pmem device, the following warning was
> > reported:
> > 
> >  ------------[ cut here ]------------
> >  WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct
> >  Modules linked in:
> >  CPU: 2 PID: 384 Comm: mkfs.xfs Not tainted 6.4.0-rc7+ #154
> >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> >  RIP: 0010:submit_bio_noacct+0x340/0x520
> >  ......
> >  Call Trace:
> >   <TASK>
> >   ? submit_bio_noacct+0xd5/0x520
> >   submit_bio+0x37/0x60
> >   async_pmem_flush+0x79/0xa0
> >   nvdimm_flush+0x17/0x40
> >   pmem_submit_bio+0x370/0x390
> >   __submit_bio+0xbc/0x190
> >   submit_bio_noacct_nocheck+0x14d/0x370
> >   submit_bio_noacct+0x1ef/0x520
> >   submit_bio+0x55/0x60
> >   submit_bio_wait+0x5a/0xc0
> >   blkdev_issue_flush+0x44/0x60
> > 
> > The root cause is that submit_bio_noacct() needs bio_op() is either
> > WRITE or ZONE_APPEND for flush bio and async_pmem_flush() doesn't assign
> > REQ_OP_WRITE when allocating flush bio, so submit_bio_noacct just fail
> > the flush bio.
> > 
> > Simply fix it by adding the missing REQ_OP_WRITE for flush bio. And we
> > could fix the flush order issue and do flush optimization later.
> > 
> > Cc: stable@vger.kernel.org # 6.3+
> > Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> > Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
> > Tested-by: Pankaj Gupta <pankaj.gupta@amd.com>
> > Signed-off-by: Hou Tao <houtao1@huawei.com>
> > ---
> > v4:
> >  * add stable Cc
> >  * collect Rvb and Tested-by tags
> > 
> > v3: https://lore.kernel.org/linux-block/20230625022633.2753877-1-houtao@huaweicloud.com
> >  * adjust the overly long lines in both commit message and code
> > 
> > v2: https://lore.kernel.org/linux-block/20230621134340.878461-1-houtao@huaweicloud.com
> >  * do a minimal fix first (Suggested by Christoph)
> > 
> > v1: https://lore.kernel.org/linux-block/ZJLpYMC8FgtZ0k2k@infradead.org/T/#t
> > 
> >  drivers/nvdimm/nd_virtio.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> > index c6a648fd8744..1f8c667c6f1e 100644
> > --- a/drivers/nvdimm/nd_virtio.c
> > +++ b/drivers/nvdimm/nd_virtio.c
> > @@ -105,7 +105,8 @@ int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
> >          * parent bio. Otherwise directly call nd_region flush.
> >          */
> >         if (bio && bio->bi_iter.bi_sector != -1) {
> > -               struct bio *child = bio_alloc(bio->bi_bdev, 0, REQ_PREFLUSH,
> > +               struct bio *child = bio_alloc(bio->bi_bdev, 0,
> > +                                             REQ_OP_WRITE | REQ_PREFLUSH,
> >                                               GFP_ATOMIC);
> > 
> >                 if (!child)
> > --
> > 2.29.2
> >
Michael S. Tsirkin Aug. 6, 2023, 3:10 p.m. UTC | #3 
On Fri, Aug 04, 2023 at 09:03:20PM +0000, Verma, Vishal L wrote:
> On Fri, 2023-08-04 at 20:39 +0200, Pankaj Gupta wrote:
> > Gentle ping!
> > 
> > Dan, Vishal for suggestion/review on this patch and request for merging.
> > +Cc Michael for awareness, as virtio-pmem device is currently broken.
> 
> Looks good to me,
> 
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Dave, will you queue this for 6.6.


Generally if you expect me to merge a patch I should be CC'd.


> > 
> > Thanks,
> > Pankaj
> > 
> > > From: Hou Tao <houtao1@huawei.com>
> > > 
> > > When doing mkfs.xfs on a pmem device, the following warning was
> > > reported:
> > > 
> > >  ------------[ cut here ]------------
> > >  WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct
> > >  Modules linked in:
> > >  CPU: 2 PID: 384 Comm: mkfs.xfs Not tainted 6.4.0-rc7+ #154
> > >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> > >  RIP: 0010:submit_bio_noacct+0x340/0x520
> > >  ......
> > >  Call Trace:
> > >   <TASK>
> > >   ? submit_bio_noacct+0xd5/0x520
> > >   submit_bio+0x37/0x60
> > >   async_pmem_flush+0x79/0xa0
> > >   nvdimm_flush+0x17/0x40
> > >   pmem_submit_bio+0x370/0x390
> > >   __submit_bio+0xbc/0x190
> > >   submit_bio_noacct_nocheck+0x14d/0x370
> > >   submit_bio_noacct+0x1ef/0x520
> > >   submit_bio+0x55/0x60
> > >   submit_bio_wait+0x5a/0xc0
> > >   blkdev_issue_flush+0x44/0x60
> > > 
> > > The root cause is that submit_bio_noacct() needs bio_op() is either
> > > WRITE or ZONE_APPEND for flush bio and async_pmem_flush() doesn't assign
> > > REQ_OP_WRITE when allocating flush bio, so submit_bio_noacct just fail
> > > the flush bio.
> > > 
> > > Simply fix it by adding the missing REQ_OP_WRITE for flush bio. And we
> > > could fix the flush order issue and do flush optimization later.
> > > 
> > > Cc: stable@vger.kernel.org # 6.3+
> > > Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> > > Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
> > > Tested-by: Pankaj Gupta <pankaj.gupta@amd.com>
> > > Signed-off-by: Hou Tao <houtao1@huawei.com>
> > > ---
> > > v4:
> > >  * add stable Cc
> > >  * collect Rvb and Tested-by tags
> > > 
> > > v3: https://lore.kernel.org/linux-block/20230625022633.2753877-1-houtao@huaweicloud.com
> > >  * adjust the overly long lines in both commit message and code
> > > 
> > > v2: https://lore.kernel.org/linux-block/20230621134340.878461-1-houtao@huaweicloud.com
> > >  * do a minimal fix first (Suggested by Christoph)
> > > 
> > > v1: https://lore.kernel.org/linux-block/ZJLpYMC8FgtZ0k2k@infradead.org/T/#t
> > > 
> > >  drivers/nvdimm/nd_virtio.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> > > index c6a648fd8744..1f8c667c6f1e 100644
> > > --- a/drivers/nvdimm/nd_virtio.c
> > > +++ b/drivers/nvdimm/nd_virtio.c
> > > @@ -105,7 +105,8 @@ int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
> > >          * parent bio. Otherwise directly call nd_region flush.
> > >          */
> > >         if (bio && bio->bi_iter.bi_sector != -1) {
> > > -               struct bio *child = bio_alloc(bio->bi_bdev, 0, REQ_PREFLUSH,
> > > +               struct bio *child = bio_alloc(bio->bi_bdev, 0,
> > > +                                             REQ_OP_WRITE | REQ_PREFLUSH,
> > >                                               GFP_ATOMIC);
> > > 
> > >                 if (!child)
> > > --
> > > 2.29.2
> > > 
>
Dave Jiang Aug. 7, 2023, 4:42 p.m. UTC | #4 
On 8/4/23 14:03, Verma, Vishal L wrote:
> On Fri, 2023-08-04 at 20:39 +0200, Pankaj Gupta wrote:
>> Gentle ping!
>>
>> Dan, Vishal for suggestion/review on this patch and request for merging.
>> +Cc Michael for awareness, as virtio-pmem device is currently broken.
> 
> Looks good to me,
> 
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Dave, will you queue this for 6.6.

Looks like it's already queued:
https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/commit/?h=libnvdimm-for-next&id=c1dbd8a849183b9c12d257ad3043ecec50db50b3


> 
>>
>> Thanks,
>> Pankaj
>>
>>> From: Hou Tao <houtao1@huawei.com>
>>>
>>> When doing mkfs.xfs on a pmem device, the following warning was
>>> reported:
>>>
>>>   ------------[ cut here ]------------
>>>   WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct
>>>   Modules linked in:
>>>   CPU: 2 PID: 384 Comm: mkfs.xfs Not tainted 6.4.0-rc7+ #154
>>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>>>   RIP: 0010:submit_bio_noacct+0x340/0x520
>>>   ......
>>>   Call Trace:
>>>    <TASK>
>>>    ? submit_bio_noacct+0xd5/0x520
>>>    submit_bio+0x37/0x60
>>>    async_pmem_flush+0x79/0xa0
>>>    nvdimm_flush+0x17/0x40
>>>    pmem_submit_bio+0x370/0x390
>>>    __submit_bio+0xbc/0x190
>>>    submit_bio_noacct_nocheck+0x14d/0x370
>>>    submit_bio_noacct+0x1ef/0x520
>>>    submit_bio+0x55/0x60
>>>    submit_bio_wait+0x5a/0xc0
>>>    blkdev_issue_flush+0x44/0x60
>>>
>>> The root cause is that submit_bio_noacct() needs bio_op() is either
>>> WRITE or ZONE_APPEND for flush bio and async_pmem_flush() doesn't assign
>>> REQ_OP_WRITE when allocating flush bio, so submit_bio_noacct just fail
>>> the flush bio.
>>>
>>> Simply fix it by adding the missing REQ_OP_WRITE for flush bio. And we
>>> could fix the flush order issue and do flush optimization later.
>>>
>>> Cc: stable@vger.kernel.org # 6.3+
>>> Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
>>> Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
>>> Tested-by: Pankaj Gupta <pankaj.gupta@amd.com>
>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>>> ---
>>> v4:
>>>   * add stable Cc
>>>   * collect Rvb and Tested-by tags
>>>
>>> v3: https://lore.kernel.org/linux-block/20230625022633.2753877-1-houtao@huaweicloud.com
>>>   * adjust the overly long lines in both commit message and code
>>>
>>> v2: https://lore.kernel.org/linux-block/20230621134340.878461-1-houtao@huaweicloud.com
>>>   * do a minimal fix first (Suggested by Christoph)
>>>
>>> v1: https://lore.kernel.org/linux-block/ZJLpYMC8FgtZ0k2k@infradead.org/T/#t
>>>
>>>   drivers/nvdimm/nd_virtio.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
>>> index c6a648fd8744..1f8c667c6f1e 100644
>>> --- a/drivers/nvdimm/nd_virtio.c
>>> +++ b/drivers/nvdimm/nd_virtio.c
>>> @@ -105,7 +105,8 @@ int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
>>>           * parent bio. Otherwise directly call nd_region flush.
>>>           */
>>>          if (bio && bio->bi_iter.bi_sector != -1) {
>>> -               struct bio *child = bio_alloc(bio->bi_bdev, 0, REQ_PREFLUSH,
>>> +               struct bio *child = bio_alloc(bio->bi_bdev, 0,
>>> +                                             REQ_OP_WRITE | REQ_PREFLUSH,
>>>                                                GFP_ATOMIC);
>>>
>>>                  if (!child)
>>> --
>>> 2.29.2
>>>
>
Pankaj Gupta Aug. 7, 2023, 5:55 p.m. UTC | #5 
> >> Gentle ping!
> >>
> >> Dan, Vishal for suggestion/review on this patch and request for merging.
> >> +Cc Michael for awareness, as virtio-pmem device is currently broken.
> >
> > Looks good to me,
> >
> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> >
> > Dave, will you queue this for 6.6.
>
> Looks like it's already queued:
> https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/commit/?h=libnvdimm-for-next&id=c1dbd8a849183b9c12d257ad3043ecec50db50b3

Thank you, Dave!

Best regards,
Pankaj
diff mbox series

Patch

diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
index c6a648fd8744..1f8c667c6f1e 100644
--- a/drivers/nvdimm/nd_virtio.c
+++ b/drivers/nvdimm/nd_virtio.c
@@ -105,7 +105,8 @@  int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
 	 * parent bio. Otherwise directly call nd_region flush.
 	 */
 	if (bio && bio->bi_iter.bi_sector != -1) {
-		struct bio *child = bio_alloc(bio->bi_bdev, 0, REQ_PREFLUSH,
+		struct bio *child = bio_alloc(bio->bi_bdev, 0,
+					      REQ_OP_WRITE | REQ_PREFLUSH,
 					      GFP_ATOMIC);
 
 		if (!child)