[RFC 0/2] virtio-pmem: Asynchronous flush

[RFC 0/2] virtio-pmem: Asynchronous flush

[Pankaj Gupta]

 Jeff reported preflush order issue with the existing implementation
 of virtio pmem preflush. Dan suggested[1] to implement asynchronous flush
 for virtio pmem using work queue as done in md/RAID. This patch series
 intends to solve the preflush ordering issue and also makes the flush
 asynchronous from the submitting thread POV.

 Submitting this patch series for feeback and is in WIP. I have
 done basic testing and currently doing more testing.

Pankaj Gupta (2):
  pmem: make nvdimm_flush asynchronous
  virtio_pmem: Async virtio-pmem flush

 drivers/nvdimm/nd_virtio.c   | 66 ++++++++++++++++++++++++++----------
 drivers/nvdimm/pmem.c        | 15 ++++----
 drivers/nvdimm/region_devs.c |  3 +-
 drivers/nvdimm/virtio_pmem.c |  9 +++++
 drivers/nvdimm/virtio_pmem.h | 12 +++++++
 5 files changed, 78 insertions(+), 27 deletions(-)

[1] https://marc.info/?l=linux-kernel&m=157446316409937&w=2
-- 
2.20.1

[RFC 1/2] pmem: make nvdimm_flush asynchronous

[Pankaj Gupta]

 This patch converts nvdimm_flush to return when 
 asynchronous flush is in process.

Signed-off-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
---
 drivers/nvdimm/pmem.c        | 15 ++++++++-------
 drivers/nvdimm/region_devs.c |  3 +--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 4ffc6f7ca131..747ffaee513b 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -192,8 +192,10 @@ static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
 	struct pmem_device *pmem = q->queuedata;
 	struct nd_region *nd_region = to_region(pmem);
 
-	if (bio->bi_opf & REQ_PREFLUSH)
-		ret = nvdimm_flush(nd_region, bio);
+	if ((bio->bi_opf & REQ_PREFLUSH) &&
+		nvdimm_flush(nd_region, bio)) {
+		return BLK_QC_T_NONE;
+	}
 
 	do_acct = nd_iostat_start(bio, &start);
 	bio_for_each_segment(bvec, bio, iter) {
@@ -207,11 +209,10 @@ static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
 	if (do_acct)
 		nd_iostat_end(bio, start);
 
-	if (bio->bi_opf & REQ_FUA)
-		ret = nvdimm_flush(nd_region, bio);
-
-	if (ret)
-		bio->bi_status = errno_to_blk_status(ret);
+	if (bio->bi_opf & REQ_FUA) {
+		nvdimm_flush(nd_region, bio);
+		return BLK_QC_T_NONE;
+	}
 
 	bio_endio(bio);
 	return BLK_QC_T_NONE;
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index a19e535830d9..1caa13f1523f 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1091,8 +1091,7 @@ int nvdimm_flush(struct nd_region *nd_region, struct bio *bio)
 	if (!nd_region->flush)
 		rc = generic_nvdimm_flush(nd_region);
 	else {
-		if (nd_region->flush(nd_region, bio))
-			rc = -EIO;
+		rc = nd_region->flush(nd_region, bio);
 	}
 
 	return rc;
-- 
2.20.1

[RFC 2/2] virtio_pmem: Async virtio-pmem flush

[Pankaj Gupta]

 This patch enables asynchronous flush for virtio pmem
 using work queue. Also, it coalesce the flush requests
 when a flush is already in process.

Signed-off-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
---
 drivers/nvdimm/nd_virtio.c   | 62 ++++++++++++++++++++++++++----------
 drivers/nvdimm/virtio_pmem.c |  9 ++++++
 drivers/nvdimm/virtio_pmem.h | 14 ++++++++
 3 files changed, 68 insertions(+), 17 deletions(-)

diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
index 10351d5b49fa..ef53d0a0d134 100644
--- a/drivers/nvdimm/nd_virtio.c
+++ b/drivers/nvdimm/nd_virtio.c
@@ -97,29 +97,57 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
 	return err;
 };
 
+static void submit_async_flush(struct work_struct *ws);
+
 /* 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(GFP_ATOMIC, 0);
-
-		if (!child)
-			return -ENOMEM;
-		bio_copy_dev(child, bio);
-		child->bi_opf = REQ_PREFLUSH;
-		child->bi_iter.bi_sector = -1;
-		bio_chain(child, bio);
-		submit_bio(child);
-		return 0;
+	struct virtio_device *vdev = nd_region->provider_data;
+	struct virtio_pmem *vpmem  = vdev->priv;
+	ktime_t start = ktime_get_boottime();
+
+	spin_lock_irq(&vpmem->lock);
+	wait_event_lock_irq(vpmem->sb_wait,
+			    !vpmem->flush_bio ||
+			    ktime_after(vpmem->last_flush, start),
+			    vpmem->lock);
+
+	if (!ktime_after(vpmem->last_flush, start)) {
+		WARN_ON(vpmem->flush_bio);
+		vpmem->flush_bio = bio;
+		bio = NULL;
 	}
-	if (virtio_pmem_flush(nd_region))
-		return -EIO;
+	spin_unlock_irq(&vpmem->lock);
 
+	if (!bio) {
+		INIT_WORK(&vpmem->flush_work, submit_async_flush);
+		queue_work(vpmem->pmem_wq, &vpmem->flush_work);
+		return 1;
+	}
+
+	bio->bi_opf &= ~REQ_PREFLUSH;
 	return 0;
 };
 EXPORT_SYMBOL_GPL(async_pmem_flush);
+
+static void submit_async_flush(struct work_struct *ws)
+{
+	struct virtio_pmem *vpmem = container_of(ws, struct virtio_pmem, flush_work);
+	struct bio *bio = vpmem->flush_bio;
+
+	vpmem->start_flush = ktime_get_boottime();
+	bio->bi_status = errno_to_blk_status(virtio_pmem_flush(vpmem->nd_region));
+	vpmem->last_flush = vpmem->start_flush;
+	vpmem->flush_bio = NULL;
+	wake_up(&vpmem->sb_wait);
+
+	/* Submit parent bio only for PREFLUSH */
+	if (bio && (bio->bi_opf & REQ_PREFLUSH)) {
+		bio->bi_opf &= ~REQ_PREFLUSH;
+		submit_bio(bio);
+	} else if (bio && (bio->bi_opf & REQ_FUA)) {
+		bio->bi_opf &= ~REQ_FUA;
+		bio_endio(bio);
+	}
+}
 MODULE_LICENSE("GPL");
diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
index 5e3d07b47e0c..4ab135d820fd 100644
--- a/drivers/nvdimm/virtio_pmem.c
+++ b/drivers/nvdimm/virtio_pmem.c
@@ -24,6 +24,7 @@ static int init_vq(struct virtio_pmem *vpmem)
 		return PTR_ERR(vpmem->req_vq);
 
 	spin_lock_init(&vpmem->pmem_lock);
+	spin_lock_init(&vpmem->lock);
 	INIT_LIST_HEAD(&vpmem->req_list);
 
 	return 0;
@@ -58,6 +59,12 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
 		goto out_err;
 	}
 
+	vpmem->pmem_wq = alloc_workqueue("vpmem_wq", WQ_MEM_RECLAIM, 0);
+	if (!vpmem->pmem_wq) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+	init_waitqueue_head(&vpmem->sb_wait);
 	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
 			start, &vpmem->start);
 	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
@@ -90,10 +97,12 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
 		goto out_nd;
 	}
 	nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
+	vpmem->nd_region = nd_region;
 	return 0;
 out_nd:
 	nvdimm_bus_unregister(vpmem->nvdimm_bus);
 out_vq:
+	destroy_workqueue(vpmem->pmem_wq);
 	vdev->config->del_vqs(vdev);
 out_err:
 	return err;
diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
index 0dddefe594c4..9d3615a324bf 100644
--- a/drivers/nvdimm/virtio_pmem.h
+++ b/drivers/nvdimm/virtio_pmem.h
@@ -35,9 +35,23 @@ struct virtio_pmem {
 	/* Virtio pmem request queue */
 	struct virtqueue *req_vq;
 
+	struct bio *flush_bio;
+	/* last_flush is when the last completed flush was started */
+	ktime_t last_flush, start_flush;
+
+	/* work queue for deferred flush */
+	struct work_struct flush_work;
+	struct workqueue_struct *pmem_wq;
+
+	/* Synchronize flush wait queue data */
+	spinlock_t lock;
+	/* for waiting for previous flush to complete */
+	wait_queue_head_t sb_wait;
+
 	/* nvdimm bus registers virtio pmem device */
 	struct nvdimm_bus *nvdimm_bus;
 	struct nvdimm_bus_descriptor nd_desc;
+	struct nd_region *nd_region;
 
 	/* List to store deferred work if virtqueue is full */
 	struct list_head req_list;
-- 
2.20.1

Re: [RFC 0/2] virtio-pmem: Asynchronous flush

[David Hildenbrand]

On 20.04.20 15:19, Pankaj Gupta wrote:
>   Jeff reported preflush order issue with the existing implementation
>   of virtio pmem preflush. Dan suggested[1] to implement asynchronous flush
>   for virtio pmem using work queue as done in md/RAID. This patch series
>   intends to solve the preflush ordering issue and also makes the flush
>   asynchronous from the submitting thread POV.
> 
>   Submitting this patch series for feeback and is in WIP. I have
>   done basic testing and currently doing more testing.
> 
> Pankaj Gupta (2):
>    pmem: make nvdimm_flush asynchronous
>    virtio_pmem: Async virtio-pmem flush
> 
>   drivers/nvdimm/nd_virtio.c   | 66 ++++++++++++++++++++++++++----------
>   drivers/nvdimm/pmem.c        | 15 ++++----
>   drivers/nvdimm/region_devs.c |  3 +-
>   drivers/nvdimm/virtio_pmem.c |  9 +++++
>   drivers/nvdimm/virtio_pmem.h | 12 +++++++
>   5 files changed, 78 insertions(+), 27 deletions(-)
> 
> [1] https://marc.info/?l=linux-kernel&m=157446316409937&w=2
> 

Just wondering, was there any follow up of this or are we still waiting 
for feedback? :)

-- 
Thanks,

David / dhildenb

Re: [RFC 0/2] virtio-pmem: Asynchronous flush

[Pankaj Gupta]

Hi David,

> >   Jeff reported preflush order issue with the existing implementation
> >   of virtio pmem preflush. Dan suggested[1] to implement asynchronous flush
> >   for virtio pmem using work queue as done in md/RAID. This patch series
> >   intends to solve the preflush ordering issue and also makes the flush
> >   asynchronous from the submitting thread POV.
> >
> >   Submitting this patch series for feeback and is in WIP. I have
> >   done basic testing and currently doing more testing.
> >
> > Pankaj Gupta (2):
> >    pmem: make nvdimm_flush asynchronous
> >    virtio_pmem: Async virtio-pmem flush
> >
> >   drivers/nvdimm/nd_virtio.c   | 66 ++++++++++++++++++++++++++----------
> >   drivers/nvdimm/pmem.c        | 15 ++++----
> >   drivers/nvdimm/region_devs.c |  3 +-
> >   drivers/nvdimm/virtio_pmem.c |  9 +++++
> >   drivers/nvdimm/virtio_pmem.h | 12 +++++++
> >   5 files changed, 78 insertions(+), 27 deletions(-)
> >
> > [1] https://marc.info/?l=linux-kernel&m=157446316409937&w=2
> >
>
> Just wondering, was there any follow up of this or are we still waiting
> for feedback? :)

Thank you for bringing this up.

My apologies I could not followup on this. I have another version in my local
tree but could not post it as I was not sure if I solved the problem
correctly. I will
clean it up and post for feedback as soon as I can.

P.S: Due to serious personal/family health issues I am not able to
devote much time
on this with other professional commitments. I feel bad that I have
this unfinished task.
Just in last one year things have not been stable for me & my family
and still not getting :(

Best regards,
Pankaj

Re: [RFC 0/2] virtio-pmem: Asynchronous flush

[Dan Williams]

On Thu, Mar 11, 2021 at 8:21 PM Pankaj Gupta
<pankaj.gupta@cloud.ionos.com> wrote:
>
> Hi David,
>
> > >   Jeff reported preflush order issue with the existing implementation
> > >   of virtio pmem preflush. Dan suggested[1] to implement asynchronous flush
> > >   for virtio pmem using work queue as done in md/RAID. This patch series
> > >   intends to solve the preflush ordering issue and also makes the flush
> > >   asynchronous from the submitting thread POV.
> > >
> > >   Submitting this patch series for feeback and is in WIP. I have
> > >   done basic testing and currently doing more testing.
> > >
> > > Pankaj Gupta (2):
> > >    pmem: make nvdimm_flush asynchronous
> > >    virtio_pmem: Async virtio-pmem flush
> > >
> > >   drivers/nvdimm/nd_virtio.c   | 66 ++++++++++++++++++++++++++----------
> > >   drivers/nvdimm/pmem.c        | 15 ++++----
> > >   drivers/nvdimm/region_devs.c |  3 +-
> > >   drivers/nvdimm/virtio_pmem.c |  9 +++++
> > >   drivers/nvdimm/virtio_pmem.h | 12 +++++++
> > >   5 files changed, 78 insertions(+), 27 deletions(-)
> > >
> > > [1] https://marc.info/?l=linux-kernel&m=157446316409937&w=2
> > >
> >
> > Just wondering, was there any follow up of this or are we still waiting
> > for feedback? :)
>
> Thank you for bringing this up.
>
> My apologies I could not followup on this. I have another version in my local
> tree but could not post it as I was not sure if I solved the problem
> correctly. I will
> clean it up and post for feedback as soon as I can.
>
> P.S: Due to serious personal/family health issues I am not able to
> devote much time
> on this with other professional commitments. I feel bad that I have
> this unfinished task.
> Just in last one year things have not been stable for me & my family
> and still not getting :(

No worries Pankaj. Take care of yourself and your family. The
community can handle this for you. I'm open to coaching somebody
through what's involved to get this fix landed.

Re: [RFC 0/2] virtio-pmem: Asynchronous flush

[David Hildenbrand]

On 12.03.21 07:02, Dan Williams wrote:
> On Thu, Mar 11, 2021 at 8:21 PM Pankaj Gupta
> <pankaj.gupta@cloud.ionos.com> wrote:
>>
>> Hi David,
>>
>>>>    Jeff reported preflush order issue with the existing implementation
>>>>    of virtio pmem preflush. Dan suggested[1] to implement asynchronous flush
>>>>    for virtio pmem using work queue as done in md/RAID. This patch series
>>>>    intends to solve the preflush ordering issue and also makes the flush
>>>>    asynchronous from the submitting thread POV.
>>>>
>>>>    Submitting this patch series for feeback and is in WIP. I have
>>>>    done basic testing and currently doing more testing.
>>>>
>>>> Pankaj Gupta (2):
>>>>     pmem: make nvdimm_flush asynchronous
>>>>     virtio_pmem: Async virtio-pmem flush
>>>>
>>>>    drivers/nvdimm/nd_virtio.c   | 66 ++++++++++++++++++++++++++----------
>>>>    drivers/nvdimm/pmem.c        | 15 ++++----
>>>>    drivers/nvdimm/region_devs.c |  3 +-
>>>>    drivers/nvdimm/virtio_pmem.c |  9 +++++
>>>>    drivers/nvdimm/virtio_pmem.h | 12 +++++++
>>>>    5 files changed, 78 insertions(+), 27 deletions(-)
>>>>
>>>> [1] https://marc.info/?l=linux-kernel&m=157446316409937&w=2
>>>>
>>>
>>> Just wondering, was there any follow up of this or are we still waiting
>>> for feedback? :)
>>
>> Thank you for bringing this up.
>>
>> My apologies I could not followup on this. I have another version in my local
>> tree but could not post it as I was not sure if I solved the problem
>> correctly. I will
>> clean it up and post for feedback as soon as I can.
>>
>> P.S: Due to serious personal/family health issues I am not able to
>> devote much time
>> on this with other professional commitments. I feel bad that I have
>> this unfinished task.
>> Just in last one year things have not been stable for me & my family
>> and still not getting :(
> 
> No worries Pankaj. Take care of yourself and your family. The
> community can handle this for you. I'm open to coaching somebody
> through what's involved to get this fix landed.

Absolutely, no need to worry for now - take care of yourself and your 
loved ones! I was merely stumbling over this series while cleaning up my 
inbox, wondering if this is still stuck waiting for review/feedback. No 
need to rush anything or be stressed.

In case I have time to look into this in the future, I'd coordinate in 
this thread (especially, asking for feedback again so I know where this 
series stands)!

-- 
Thanks,

David / dhildenb