[PATCH] virtio_pmem: do flush synchronously

[PATCH] virtio_pmem: do flush synchronously

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

The following warning was reported when doing fsync on a pmem device:

 ------------[ cut here ]------------
 WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct+0x340/0x520
 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>
  ? asm_exc_invalid_op+0x1b/0x20
  ? submit_bio_noacct+0x340/0x520
  ? 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.

The reason for allocating a new flush bio is to execute the flush
command asynchrously and doesn't want to block the original submit_bio()
invocation. However the original submit_bio() will be blocked anyway,
because the nested submit_bio() for the flush bio just places the flush
bio in current->bio_list and the original submit_bio() only returns
after submitting all bio in bio_list.

So just removing the allocation of new flush bio and do synchronous
flush directly.

Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
Hi Jens & Dan,

I found Pankaj was working on the optimization of virtio-pmem flush bio
[0], but considering the last status update was 1/12/2022, so could you
please pick the patch up for v6.7 and we can do the flush optimization
later ?

[0]: https://lore.kernel.org/lkml/20220111161937.56272-1-pankaj.gupta.linux@gmail.com/T/

 drivers/nvdimm/nd_virtio.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
index c6a648fd8744..a7d510f446e0 100644
--- a/drivers/nvdimm/nd_virtio.c
+++ b/drivers/nvdimm/nd_virtio.c
@@ -100,22 +100,6 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
 /* The asynchronous flush callback function */
 int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
 {
-	/*
-	 * Create child bio for asynchronous flush and chain with
-	 * 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,
-					      GFP_ATOMIC);
-
-		if (!child)
-			return -ENOMEM;
-		bio_clone_blkg_association(child, bio);
-		child->bi_iter.bi_sector = -1;
-		bio_chain(child, bio);
-		submit_bio(child);
-		return 0;
-	}
 	if (virtio_pmem_flush(nd_region))
 		return -EIO;
 
-- 
2.29.2

Re: [PATCH] virtio_pmem: do flush synchronously

[Christoph Hellwig]

I think the proper minimal fix is to pass in a REQ_WRITE in addition to
REQ_PREFLUSH.  We can than have a discussion on the merits of this
weird async pmem flush scheme separately.

[PATCH v2] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

The following warning was reported when doing fsync on a pmem device:

 ------------[ cut here ]------------
 WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct+0x340/0x520
 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>
  ? asm_exc_invalid_op+0x1b/0x20
  ? submit_bio_noacct+0x340/0x520
  ? 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.

Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
v2:
  * do a minimal fix first (Suggested by Christoph)
v1: https://lore.kernel.org/linux-block/ZJLpYMC8FgtZ0k2k@infradead.org/T/#t

Hi Jens & Dan,

I found Pankaj was working on the fix and optimization of virtio-pmem
flush bio [0], but considering the last status update was 1/12/2022, so
could you please pick the patch up for v6.4 and we can do the flush fix
and optimization later ?

[0]: https://lore.kernel.org/lkml/20220111161937.56272-1-pankaj.gupta.linux@gmail.com/T/

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

diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
index c6a648fd8744..97098099f8a3 100644
--- a/drivers/nvdimm/nd_virtio.c
+++ b/drivers/nvdimm/nd_virtio.c
@@ -105,7 +105,7 @@ 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

Re: [PATCH v2] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Christoph Hellwig]

Please avoid the overly long line.  With that fixe this looks good
to me.

[PATCH v3] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

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

 ------------[ 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>
  ? asm_exc_invalid_op+0x1b/0x20
  ? submit_bio_noacct+0x340/0x520
  ? 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.

Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
v3:
 * 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

Hi Jens & Dan,

I found Pankaj was working on the optimization of virtio-pmem flush bio
[0], but considering the last status update was 1/12/2022, so could you
please pick the patch up for v6.4 and we can do the flush optimization
later ?

[0]: https://lore.kernel.org/lkml/20220111161937.56272-1-pankaj.gupta.linux@gmail.com/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

Re: [PATCH v3] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Chaitanya Kulkarni]

On 6/24/2023 7:26 PM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> When doing mkfs.xfs on a pmem device, the following warning was
> reported and :
> 
>   ------------[ 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>
>    ? asm_exc_invalid_op+0x1b/0x20
>    ? submit_bio_noacct+0x340/0x520
>    ? 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.
> 
> Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> v3:
>   * 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
> 
> Hi Jens & Dan,
> 
> I found Pankaj was working on the optimization of virtio-pmem flush bio
> [0], but considering the last status update was 1/12/2022, so could you
> please pick the patch up for v6.4 and we can do the flush optimization
> later ?
> 
> [0]: https://lore.kernel.org/lkml/20220111161937.56272-1-pankaj.gupta.linux@gmail.com/T/
> 

I've failed to understand why we should wait for [0] ...

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v3] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Pankaj Gupta]

> I've failed to understand why we should wait for [0] ...

Sorry, missed to answer before. The optimization was to make the
virtio-pmem interface completely
asynchronous and coalesce the flush requests. As this uses the
asynchronous behavior of virtio bus
there is no stall of guest vCPU, so this optimization intends to solve below:

- Guest IO submitting thread can do other other operations till the
host flush completes.
- As all the guest flush work on one virtio-pmem device. If there is
single flush in queue all the subsequent flush would wait enabling
request coalescing.

Currently, its solved by splitting the bio request to parent & child.
But this results in preorder issue (As PREFLUSH happens after the
first fsync, because of the way how bio_submit() works). I think the
preflush order issue will still remain after this patch. But currently
this interface is broken in mainline. So, this patch fixes the issue.


Thanks,
Pankaj

Re: [PATCH v3] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Pankaj Gupta]

> From: Hou Tao <houtao1@huawei.com>
>
> When doing mkfs.xfs on a pmem device, the following warning was
> reported and :
>
>  ------------[ 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>
>   ? asm_exc_invalid_op+0x1b/0x20
>   ? submit_bio_noacct+0x340/0x520
>   ? 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.
>
> Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
> Signed-off-by: Hou Tao <houtao1@huawei.com>

With 6.3+ stable Cc, Feel free to add:

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
Tested-by: Pankaj Gupta <pankaj.gupta@amd.com>

Thanks,
Pankaj

Re: [PATCH v3] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Pankaj Gupta]

+Cc Vishal,

> > Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
> > Signed-off-by: Hou Tao <houtao1@huawei.com>
>
> With 6.3+ stable Cc, Feel free to add:

Hi Dan, Vishal,

Do you have any suggestion on this patch? Or can we queue this please?

Hi Hou,

Could you please send a new version with Cc stable or as per any
suggestions from maintainers.

Thanks,
Pankaj

Re: [PATCH v3] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Hou Tao]

Hi Pankaj,

On 7/13/2023 4:23 PM, Pankaj Gupta wrote:
> +Cc Vishal,
>
>>> Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> With 6.3+ stable Cc, Feel free to add:
> Hi Dan, Vishal,
>
> Do you have any suggestion on this patch? Or can we queue this please?
>
> Hi Hou,
>
> Could you please send a new version with Cc stable or as per any
> suggestions from maintainers.
Will add Cc stable for v6.3 in v4.
>
> Thanks,
> Pankaj
> .

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

[Hou Tao]

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

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

[Pankaj Gupta]

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
>

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

[Verma, Vishal L]

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

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

[Michael S. Tsirkin]

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

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

[Dave Jiang]

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

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

[Pankaj Gupta]

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

Re: [PATCH v2] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Pankaj Gupta]

> The following warning was reported when doing fsync on a pmem device:
>
>  ------------[ cut here ]------------
>  WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct+0x340/0x520
>  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>
>   ? asm_exc_invalid_op+0x1b/0x20
>   ? submit_bio_noacct+0x340/0x520
>   ? 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.
>
> Fixes: b4a6bb3a67aa ("block: add a sanity check for non-write flush/fua bios")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> v2:
>   * do a minimal fix first (Suggested by Christoph)
> v1: https://lore.kernel.org/linux-block/ZJLpYMC8FgtZ0k2k@infradead.org/T/#t
>
> Hi Jens & Dan,
>
> I found Pankaj was working on the fix and optimization of virtio-pmem
> flush bio [0], but considering the last status update was 1/12/2022, so
> could you please pick the patch up for v6.4 and we can do the flush fix
> and optimization later ?
>
> [0]: https://lore.kernel.org/lkml/20220111161937.56272-1-pankaj.gupta.linux@gmail.com/T/
>
>  drivers/nvdimm/nd_virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> index c6a648fd8744..97098099f8a3 100644
> --- a/drivers/nvdimm/nd_virtio.c
> +++ b/drivers/nvdimm/nd_virtio.c
> @@ -105,7 +105,7 @@ 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)

Fix looks good to me. Will give a run soon.

Yes, [0] needs to be completed. Curious to know if you guys using
virtio-pmem device?

Thanks,
Pankaj

Re: [PATCH v2] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Hou Tao]

Hi Pankaj,

On 6/22/2023 4:35 PM, Pankaj Gupta wrote:
>> The following warning was reported when doing fsync on a pmem device:
>>
>>  ------------[ cut here ]------------
>>  WARNING: CPU: 2 PID: 384 at block/blk-core.c:751 submit_bio_noacct+0x340/0x520
SNIP
>> Hi Jens & Dan,
>>
>> I found Pankaj was working on the fix and optimization of virtio-pmem
>> flush bio [0], but considering the last status update was 1/12/2022, so
>> could you please pick the patch up for v6.4 and we can do the flush fix
>> and optimization later ?
>>
>> [0]: https://lore.kernel.org/lkml/20220111161937.56272-1-pankaj.gupta.linux@gmail.com/T/
>>
>>  drivers/nvdimm/nd_virtio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
>> index c6a648fd8744..97098099f8a3 100644
>> --- a/drivers/nvdimm/nd_virtio.c
>> +++ b/drivers/nvdimm/nd_virtio.c
>> @@ -105,7 +105,7 @@ 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)
> Fix looks good to me. Will give a run soon.
>
> Yes, [0] needs to be completed. Curious to know if you guys using
> virtio-pmem device?
Sorry about missing the question. We are plan to use DAX to do page
cache offload and now we are just do experiment with virtio-pmem and
nd-pmem.

> Thanks,
> Pankaj

Re: [PATCH v2] virtio_pmem: add the missing REQ_OP_WRITE for flush bio

[Pankaj Gupta]

> > Yes, [0] needs to be completed. Curious to know if you guys using
> > virtio-pmem device?
> Sorry about missing the question. We are plan to use DAX to do page
> cache offload and now we are just do experiment with virtio-pmem and
> nd-pmem.

Sounds good. Thank you for answering!

Best regards,
Pankaj