LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v9 0/7] virtio pmem driver 
@ 2019-05-14 14:54 Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 1/7] libnvdimm: nd_region flush callback support Pankaj Gupta
                   ` (6 more replies)
  0 siblings, 7 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

 Hi Dan,

 Proposing the patch series to be merged via nvdimm tree
 as kindly agreed by you. We have ack/review on XFS, EXT4 &
 VIRTIO patches. 

 Incorporated all the changes suggested in v8. This version
 added a new patch 4 for dax for device mapper change and some
 minor style changes in patch 2. Kept all the reviews. Request
 to please merge the series.

 ---

 This patch series has implementation for "virtio pmem". 
 "virtio pmem" is fake persistent memory(nvdimm) in guest 
 which allows to bypass the guest page cache. This also
 implements a VIRTIO based asynchronous flush mechanism.  
 
 Sharing guest kernel driver in this patchset with the 
 changes suggested in v4. Tested with Qemu side device 
 emulation [6] for virtio-pmem. Documented the impact of
 possible page cache side channel attacks with suggested
 countermeasures.

 Details of project idea for 'virtio pmem' flushing interface 
 is shared [3] & [4].

 Implementation is divided into two parts:
 New virtio pmem guest driver and qemu code changes for new 
 virtio pmem paravirtualized device.

1. Guest virtio-pmem kernel driver
---------------------------------
   - Reads persistent memory range from paravirt device and 
     registers with 'nvdimm_bus'.  
   - 'nvdimm/pmem' driver uses this information to allocate 
     persistent memory region and setup filesystem operations 
     to the allocated memory. 
   - virtio pmem driver implements asynchronous flushing 
     interface to flush from guest to host.

2. Qemu virtio-pmem device
---------------------------------
   - Creates virtio pmem device and exposes a memory range to 
     KVM guest. 
   - At host side this is file backed memory which acts as 
     persistent memory. 
   - Qemu side flush uses aio thread pool API's and virtio 
     for asynchronous guest multi request handling. 

   David Hildenbrand CCed also posted a modified version[7] of 
   qemu virtio-pmem code based on updated Qemu memory device API. 

 Virtio-pmem security implications and countermeasures:
 -----------------------------------------------------

 In previous posting of kernel driver, there was discussion [9]
 on possible implications of page cache side channel attacks with 
 virtio pmem. After thorough analysis of details of known side 
 channel attacks, below are the suggestions:

 - Depends entirely on how host backing image file is mapped 
   into guest address space. 

 - virtio-pmem device emulation, by default shared mapping is used
   to map host backing file. It is recommended to use separate
   backing file at host side for every guest. This will prevent
   any possibility of executing common code from multiple guests
   and any chance of inferring guest local data based based on 
   execution time.

 - If backing file is required to be shared among multiple guests 
   it is recommended to don't support host page cache eviction 
   commands from the guest driver. This will avoid any possibility
   of inferring guest local data or host data from another guest. 

 - Proposed device specification [8] for virtio-pmem device with 
   details of possible security implications and suggested 
   countermeasures for device emulation.

 Virtio-pmem errors handling:
 ----------------------------------------
  Checked behaviour of virtio-pmem for below types of errors
  Need suggestions on expected behaviour for handling these errors?

  - Hardware Errors: Uncorrectable recoverable Errors: 
  a] virtio-pmem: 
    - As per current logic if error page belongs to Qemu process, 
      host MCE handler isolates(hwpoison) that page and send SIGBUS. 
      Qemu SIGBUS handler injects exception to KVM guest. 
    - KVM guest then isolates the page and send SIGBUS to guest 
      userspace process which has mapped the page. 
  
  b] Existing implementation for ACPI pmem driver: 
    - Handles such errors with MCE notifier and creates a list 
      of bad blocks. Read/direct access DAX operation return EIO 
      if accessed memory page fall in bad block list.
    - It also starts backgound scrubbing.  
    - Similar functionality can be reused in virtio-pmem with MCE 
      notifier but without scrubbing(no ACPI/ARS)? Need inputs to 
      confirm if this behaviour is ok or needs any change?

Changes from PATCH v8: [1] 
 - Set device mapper synchronous if all target devices support - Dan
 - Move virtio_pmem.h to nvdimm directory  - Dan
 - Style, indentation & better error messages in patch 2 - DavidH
 - Added MST's ack in patch 2.

Changes from PATCH v7: [2]
 - Corrected pending request queue logic (patch 2) - Jakub Staroń
 - Used unsigned long flags for passing DAXDEV_F_SYNC (patch 3) - Dan
 - Fixed typo =>  vma 'flag' to 'vm_flag' (patch 4)
 - Added rob in patch 6 & patch 2

Changes from PATCH v6: 
 - Corrected comment format in patch 5 & patch 6. [Dave]
 - Changed variable declaration indentation in patch 6 [Darrick]
 - Add Reviewed-by tag by 'Jan Kara' in patch 4 & patch 5

Changes from PATCH v5: 
  Changes suggested in by - [Cornelia, Yuval]
- Remove assignment chaining in virtio driver
- Better error message and remove not required free
- Check nd_region before use

  Changes suggested by - [Jan Kara]
- dax_synchronous() for !CONFIG_DAX
- Correct 'daxdev_mapping_supported' comment and non-dax implementation

  Changes suggested by - [Dan Williams]
- Pass meaningful flag 'DAXDEV_F_SYNC' to alloc_dax
- Gate nvdimm_flush instead of additional async parameter
- Move block chaining logic to flush callback than common nvdimm_flush
- Use NULL flush callback for generic flush for better readability [Dan, Jan]

- Use virtio device id 27 from 25(already used) - [MST]

Changes from PATCH v4:
- Factor out MAP_SYNC supported functionality to a common helper
				[Dave, Darrick, Jan]
- Comment, indentation and virtqueue_kick failure handle - Yuval Shaia

Changes from PATCH v3: 
- Use generic dax_synchronous() helper to check for DAXDEV_SYNC 
  flag - [Dan, Darrick, Jan]
- Add 'is_nvdimm_async' function
- Document page cache side channel attacks implications & 
  countermeasures - [Dave Chinner, Michael]

Changes from PATCH v2: 
- Disable MAP_SYNC for ext4 & XFS filesystems - [Dan] 
- Use name 'virtio pmem' in place of 'fake dax' 

Changes from PATCH v1: 
- 0-day build test for build dependency on libnvdimm 

 Changes suggested by - [Dan Williams]
- Split the driver into two parts virtio & pmem  
- Move queuing of async block request to block layer
- Add "sync" parameter in nvdimm_flush function
- Use indirect call for nvdimm_flush
- Don’t move declarations to common global header e.g nd.h
- nvdimm_flush() return 0 or -EIO if it fails
- Teach nsio_rw_bytes() that the flush can fail
- Rename nvdimm_flush() to generic_nvdimm_flush()
- Use 'nd_region->provider_data' for long dereferencing
- Remove virtio_pmem_freeze/restore functions
- Remove BSD license text with SPDX license text

- Add might_sleep() in virtio_pmem_flush - [Luiz]
- Make spin_lock_irqsave() narrow

Pankaj Gupta (7):
   libnvdimm: nd_region flush callback support
   virtio-pmem: Add virtio-pmem guest driver
   libnvdimm: add nd_region buffered dax_dev flag
   dax: check synchronous mapping is supported
   dm: dm: Enable synchronous dax
   ext4: disable map_sync for virtio pmem
   xfs: disable map_sync for virtio pmem

[1] https://lkml.org/lkml/2019/5/10/447
[2] https://lkml.org/lkml/2019/4/26/36
[3] https://www.spinics.net/lists/kvm/msg149761.html
[4] https://www.spinics.net/lists/kvm/msg153095.html  
[5] https://lkml.org/lkml/2018/8/31/413
[6] https://marc.info/?l=linux-kernel&m=153572228719237&w=2 
[7] https://marc.info/?l=qemu-devel&m=153555721901824&w=2
[8] https://lists.oasis-open.org/archives/virtio-dev/201903/msg00083.html
[9] https://lkml.org/lkml/2019/1/9/1191

 drivers/acpi/nfit/core.c         |    4 -
 drivers/dax/bus.c                |    2 
 drivers/dax/super.c              |   19 +++++
 drivers/md/dm-table.c            |   14 ++++
 drivers/md/dm.c                  |    3 
 drivers/nvdimm/Makefile          |    1 
 drivers/nvdimm/claim.c           |    6 +
 drivers/nvdimm/nd.h              |    1 
 drivers/nvdimm/nd_virtio.c       |  126 +++++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/pmem.c            |   18 +++--
 drivers/nvdimm/region_devs.c     |   33 +++++++++-
 drivers/nvdimm/virtio_pmem.c     |  122 +++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/virtio_pmem.h     |   60 ++++++++++++++++++
 drivers/virtio/Kconfig           |   11 +++
 fs/ext4/file.c                   |   10 +--
 fs/xfs/xfs_file.c                |    9 +-
 include/linux/dax.h              |   26 +++++++-
 include/linux/libnvdimm.h        |    9 ++
 include/uapi/linux/virtio_ids.h  |    1 
 include/uapi/linux/virtio_pmem.h |   10 +++
 20 files changed, 460 insertions(+), 25 deletions(-)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v9 1/7] libnvdimm: nd_region flush callback support
  2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
@ 2019-05-14 14:54 ` Pankaj Gupta
  2019-05-15 21:07   ` Dan Williams
  2019-05-14 14:54 ` [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver Pankaj Gupta
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

This patch adds functionality to perform flush from guest
to host over VIRTIO. We are registering a callback based
on 'nd_region' type. virtio_pmem driver requires this special
flush function. For rest of the region types we are registering
existing flush function. Report error returned by host fsync
failure to userspace.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 drivers/acpi/nfit/core.c     |  4 ++--
 drivers/nvdimm/claim.c       |  6 ++++--
 drivers/nvdimm/nd.h          |  1 +
 drivers/nvdimm/pmem.c        | 13 ++++++++-----
 drivers/nvdimm/region_devs.c | 26 ++++++++++++++++++++++++--
 include/linux/libnvdimm.h    |  8 +++++++-
 6 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 5a389a4f4f65..08dde76cf459 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2434,7 +2434,7 @@ static void write_blk_ctl(struct nfit_blk *nfit_blk, unsigned int bw,
 		offset = to_interleave_offset(offset, mmio);
 
 	writeq(cmd, mmio->addr.base + offset);
-	nvdimm_flush(nfit_blk->nd_region);
+	nvdimm_flush(nfit_blk->nd_region, NULL);
 
 	if (nfit_blk->dimm_flags & NFIT_BLK_DCR_LATCH)
 		readq(mmio->addr.base + offset);
@@ -2483,7 +2483,7 @@ static int acpi_nfit_blk_single_io(struct nfit_blk *nfit_blk,
 	}
 
 	if (rw)
-		nvdimm_flush(nfit_blk->nd_region);
+		nvdimm_flush(nfit_blk->nd_region, NULL);
 
 	rc = read_blk_stat(nfit_blk, lane) ? -EIO : 0;
 	return rc;
diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
index fb667bf469c7..13510bae1e6f 100644
--- a/drivers/nvdimm/claim.c
+++ b/drivers/nvdimm/claim.c
@@ -263,7 +263,7 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns,
 	struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
 	unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
 	sector_t sector = offset >> 9;
-	int rc = 0;
+	int rc = 0, ret = 0;
 
 	if (unlikely(!size))
 		return 0;
@@ -301,7 +301,9 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns,
 	}
 
 	memcpy_flushcache(nsio->addr + offset, buf, size);
-	nvdimm_flush(to_nd_region(ndns->dev.parent));
+	ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL);
+	if (ret)
+		rc = ret;
 
 	return rc;
 }
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index a5ac3b240293..0c74d2428bd7 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -159,6 +159,7 @@ struct nd_region {
 	struct badblocks bb;
 	struct nd_interleave_set *nd_set;
 	struct nd_percpu_lane __percpu *lane;
+	int (*flush)(struct nd_region *nd_region, struct bio *bio);
 	struct nd_mapping mapping[0];
 };
 
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index bc2f700feef8..f719245da170 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -192,6 +192,7 @@ static blk_status_t pmem_do_bvec(struct pmem_device *pmem, struct page *page,
 
 static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
 {
+	int ret = 0;
 	blk_status_t rc = 0;
 	bool do_acct;
 	unsigned long start;
@@ -201,7 +202,7 @@ static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
 	struct nd_region *nd_region = to_region(pmem);
 
 	if (bio->bi_opf & REQ_PREFLUSH)
-		nvdimm_flush(nd_region);
+		ret = nvdimm_flush(nd_region, bio);
 
 	do_acct = nd_iostat_start(bio, &start);
 	bio_for_each_segment(bvec, bio, iter) {
@@ -216,7 +217,10 @@ static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
 		nd_iostat_end(bio, start);
 
 	if (bio->bi_opf & REQ_FUA)
-		nvdimm_flush(nd_region);
+		ret = nvdimm_flush(nd_region, bio);
+
+	if (ret)
+		bio->bi_status = errno_to_blk_status(ret);
 
 	bio_endio(bio);
 	return BLK_QC_T_NONE;
@@ -469,7 +473,6 @@ static int pmem_attach_disk(struct device *dev,
 	}
 	dax_write_cache(dax_dev, nvdimm_has_cache(nd_region));
 	pmem->dax_dev = dax_dev;
-
 	gendev = disk_to_dev(disk);
 	gendev->groups = pmem_attribute_groups;
 
@@ -527,14 +530,14 @@ static int nd_pmem_remove(struct device *dev)
 		sysfs_put(pmem->bb_state);
 		pmem->bb_state = NULL;
 	}
-	nvdimm_flush(to_nd_region(dev->parent));
+	nvdimm_flush(to_nd_region(dev->parent), NULL);
 
 	return 0;
 }
 
 static void nd_pmem_shutdown(struct device *dev)
 {
-	nvdimm_flush(to_nd_region(dev->parent));
+	nvdimm_flush(to_nd_region(dev->parent), NULL);
 }
 
 static void nd_pmem_notify(struct device *dev, enum nvdimm_event event)
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index b4ef7d9ff22e..e5b59708865e 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -295,7 +295,9 @@ static ssize_t deep_flush_store(struct device *dev, struct device_attribute *att
 		return rc;
 	if (!flush)
 		return -EINVAL;
-	nvdimm_flush(nd_region);
+	rc = nvdimm_flush(nd_region, NULL);
+	if (rc)
+		return rc;
 
 	return len;
 }
@@ -1085,6 +1087,11 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
 	dev->of_node = ndr_desc->of_node;
 	nd_region->ndr_size = resource_size(ndr_desc->res);
 	nd_region->ndr_start = ndr_desc->res->start;
+	if (ndr_desc->flush)
+		nd_region->flush = ndr_desc->flush;
+	else
+		nd_region->flush = NULL;
+
 	nd_device_register(dev);
 
 	return nd_region;
@@ -1125,11 +1132,24 @@ struct nd_region *nvdimm_volatile_region_create(struct nvdimm_bus *nvdimm_bus,
 }
 EXPORT_SYMBOL_GPL(nvdimm_volatile_region_create);
 
+int nvdimm_flush(struct nd_region *nd_region, struct bio *bio)
+{
+	int rc = 0;
+
+	if (!nd_region->flush)
+		rc = generic_nvdimm_flush(nd_region);
+	else {
+		if (nd_region->flush(nd_region, bio))
+			rc = -EIO;
+	}
+
+	return rc;
+}
 /**
  * nvdimm_flush - flush any posted write queues between the cpu and pmem media
  * @nd_region: blk or interleaved pmem region
  */
-void nvdimm_flush(struct nd_region *nd_region)
+int generic_nvdimm_flush(struct nd_region *nd_region)
 {
 	struct nd_region_data *ndrd = dev_get_drvdata(&nd_region->dev);
 	int i, idx;
@@ -1153,6 +1173,8 @@ void nvdimm_flush(struct nd_region *nd_region)
 		if (ndrd_get_flush_wpq(ndrd, i, 0))
 			writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));
 	wmb();
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(nvdimm_flush);
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index feb342d026f2..a5f369ec3726 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -65,6 +65,9 @@ enum {
 	 */
 	ND_REGION_PERSIST_MEMCTRL = 2,
 
+	/* Platform provides asynchronous flush mechanism */
+	ND_REGION_ASYNC = 3,
+
 	/* mark newly adjusted resources as requiring a label update */
 	DPA_RESOURCE_ADJUSTED = 1 << 0,
 };
@@ -121,6 +124,7 @@ struct nd_mapping_desc {
 	int position;
 };
 
+struct nd_region;
 struct nd_region_desc {
 	struct resource *res;
 	struct nd_mapping_desc *mapping;
@@ -133,6 +137,7 @@ struct nd_region_desc {
 	int target_node;
 	unsigned long flags;
 	struct device_node *of_node;
+	int (*flush)(struct nd_region *nd_region, struct bio *bio);
 };
 
 struct device;
@@ -260,7 +265,8 @@ unsigned long nd_blk_memremap_flags(struct nd_blk_region *ndbr);
 unsigned int nd_region_acquire_lane(struct nd_region *nd_region);
 void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane);
 u64 nd_fletcher64(void *addr, size_t len, bool le);
-void nvdimm_flush(struct nd_region *nd_region);
+int nvdimm_flush(struct nd_region *nd_region, struct bio *bio);
+int generic_nvdimm_flush(struct nd_region *nd_region);
 int nvdimm_has_flush(struct nd_region *nd_region);
 int nvdimm_has_cache(struct nd_region *nd_region);
 int nvdimm_in_overwrite(struct nvdimm *nvdimm);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 1/7] libnvdimm: nd_region flush callback support Pankaj Gupta
@ 2019-05-14 14:54 ` Pankaj Gupta
  2019-05-14 15:09   ` Randy Dunlap
                     ` (2 more replies)
  2019-05-14 14:54 ` [PATCH v9 3/7] libnvdimm: add dax_dev sync flag Pankaj Gupta
                   ` (4 subsequent siblings)
  6 siblings, 3 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

This patch adds virtio-pmem driver for KVM guest.

Guest reads the persistent memory range information from
Qemu over VIRTIO and registers it on nvdimm_bus. It also
creates a nd_region object with the persistent memory
range information so that existing 'nvdimm/pmem' driver
can reserve this into system memory map. This way
'virtio-pmem' driver uses existing functionality of pmem
driver to register persistent memory compatible for DAX
capable filesystems.

This also provides function to perform guest flush over
VIRTIO from 'pmem' driver when userspace performs flush
on DAX memory range.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/nvdimm/Makefile          |   1 +
 drivers/nvdimm/nd_virtio.c       | 126 +++++++++++++++++++++++++++++++
 drivers/nvdimm/virtio_pmem.c     | 122 ++++++++++++++++++++++++++++++
 drivers/nvdimm/virtio_pmem.h     |  60 +++++++++++++++
 drivers/virtio/Kconfig           |  11 +++
 include/uapi/linux/virtio_ids.h  |   1 +
 include/uapi/linux/virtio_pmem.h |  10 +++
 7 files changed, 331 insertions(+)
 create mode 100644 drivers/nvdimm/nd_virtio.c
 create mode 100644 drivers/nvdimm/virtio_pmem.c
 create mode 100644 drivers/nvdimm/virtio_pmem.h
 create mode 100644 include/uapi/linux/virtio_pmem.h

diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
index 6f2a088afad6..cefe233e0b52 100644
--- a/drivers/nvdimm/Makefile
+++ b/drivers/nvdimm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_ND_BTT) += nd_btt.o
 obj-$(CONFIG_ND_BLK) += nd_blk.o
 obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
 obj-$(CONFIG_OF_PMEM) += of_pmem.o
+obj-$(CONFIG_VIRTIO_PMEM) += virtio_pmem.o nd_virtio.o
 
 nd_pmem-y := pmem.o
 
diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
new file mode 100644
index 000000000000..dd95cfecfe4c
--- /dev/null
+++ b/drivers/nvdimm/nd_virtio.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * virtio_pmem.c: Virtio pmem Driver
+ *
+ * Discovers persistent memory range information
+ * from host and provides a virtio based flushing
+ * interface.
+ */
+#include "virtio_pmem.h"
+#include "nd.h"
+
+ /* The interrupt handler */
+void host_ack(struct virtqueue *vq)
+{
+	struct virtio_pmem *vpmem = vq->vdev->priv;
+	struct virtio_pmem_request *req, *req_buf;
+	unsigned long flags;
+	unsigned int len;
+
+	spin_lock_irqsave(&vpmem->pmem_lock, flags);
+	while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
+		req->done = true;
+		wake_up(&req->host_acked);
+
+		if (!list_empty(&vpmem->req_list)) {
+			req_buf = list_first_entry(&vpmem->req_list,
+					struct virtio_pmem_request, list);
+			req_buf->wq_buf_avail = true;
+			wake_up(&req_buf->wq_buf);
+			list_del(&req_buf->list);
+		}
+	}
+	spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
+}
+EXPORT_SYMBOL_GPL(host_ack);
+
+ /* The request submission function */
+int virtio_pmem_flush(struct nd_region *nd_region)
+{
+	struct virtio_device *vdev = nd_region->provider_data;
+	struct virtio_pmem *vpmem = vdev->priv;
+	struct scatterlist *sgs[2], sg, ret;
+	struct virtio_pmem_request *req;
+	unsigned long flags;
+	int err, err1;
+
+	might_sleep();
+	req = kmalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	req->done = false;
+	strcpy(req->name, "FLUSH");
+	init_waitqueue_head(&req->host_acked);
+	init_waitqueue_head(&req->wq_buf);
+	INIT_LIST_HEAD(&req->list);
+	sg_init_one(&sg, req->name, strlen(req->name));
+	sgs[0] = &sg;
+	sg_init_one(&ret, &req->ret, sizeof(req->ret));
+	sgs[1] = &ret;
+
+	spin_lock_irqsave(&vpmem->pmem_lock, flags);
+	 /*
+	  * If virtqueue_add_sgs returns -ENOSPC then req_vq virtual
+	  * queue does not have free descriptor. We add the request
+	  * to req_list and wait for host_ack to wake us up when free
+	  * slots are available.
+	  */
+	while ((err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req,
+					GFP_ATOMIC)) == -ENOSPC) {
+
+		dev_err(&vdev->dev, "failed to send command to virtio pmem" \
+			"device, no free slots in the virtqueue\n");
+		req->wq_buf_avail = false;
+		list_add_tail(&req->list, &vpmem->req_list);
+		spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
+
+		/* A host response results in "host_ack" getting called */
+		wait_event(req->wq_buf, req->wq_buf_avail);
+		spin_lock_irqsave(&vpmem->pmem_lock, flags);
+	}
+	err1 = virtqueue_kick(vpmem->req_vq);
+	spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
+
+	/*
+	 * virtqueue_add_sgs failed with error different than -ENOSPC, we can't
+	 * do anything about that.
+	 */
+	if (err || !err1) {
+		dev_info(&vdev->dev, "failed to send command to virtio pmem device\n");
+		err = -EIO;
+	} else {
+		/* A host repsonse results in "host_ack" getting called */
+		wait_event(req->host_acked, req->done);
+		err = req->ret;
+	}
+
+	kfree(req);
+	return err;
+};
+
+/* 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;
+	}
+	if (virtio_pmem_flush(nd_region))
+		return -EIO;
+
+	return 0;
+};
+EXPORT_SYMBOL_GPL(async_pmem_flush);
+MODULE_LICENSE("GPL");
diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
new file mode 100644
index 000000000000..73a1cd085eae
--- /dev/null
+++ b/drivers/nvdimm/virtio_pmem.c
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * virtio_pmem.c: Virtio pmem Driver
+ *
+ * Discovers persistent memory range information
+ * from host and registers the virtual pmem device
+ * with libnvdimm core.
+ */
+#include "virtio_pmem.h"
+#include "nd.h"
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+ /* Initialize virt queue */
+static int init_vq(struct virtio_pmem *vpmem)
+{
+	/* single vq */
+	vpmem->req_vq = virtio_find_single_vq(vpmem->vdev,
+						host_ack, "flush_queue");
+	if (IS_ERR(vpmem->req_vq))
+		return PTR_ERR(vpmem->req_vq);
+
+	spin_lock_init(&vpmem->pmem_lock);
+	INIT_LIST_HEAD(&vpmem->req_list);
+
+	return 0;
+};
+
+static int virtio_pmem_probe(struct virtio_device *vdev)
+{
+	struct nd_region_desc ndr_desc = {};
+	int nid = dev_to_node(&vdev->dev);
+	struct nd_region *nd_region;
+	struct virtio_pmem *vpmem;
+	struct resource res;
+	int err = 0;
+
+	if (!vdev->config->get) {
+		dev_err(&vdev->dev, "%s failure: config access disabled\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem), GFP_KERNEL);
+	if (!vpmem) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	vpmem->vdev = vdev;
+	vdev->priv = vpmem;
+	err = init_vq(vpmem);
+	if (err) {
+		dev_err(&vdev->dev, "failed to initialize virtio pmem vq's\n");
+		goto out_err;
+	}
+
+	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
+			start, &vpmem->start);
+	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
+			size, &vpmem->size);
+
+	res.start = vpmem->start;
+	res.end   = vpmem->start + vpmem->size-1;
+	vpmem->nd_desc.provider_name = "virtio-pmem";
+	vpmem->nd_desc.module = THIS_MODULE;
+
+	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
+						&vpmem->nd_desc);
+	if (!vpmem->nvdimm_bus) {
+		dev_err(&vdev->dev, "failed to register device with nvdimm_bus\n");
+		err = -ENXIO;
+		goto out_vq;
+	}
+
+	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
+
+	ndr_desc.res = &res;
+	ndr_desc.numa_node = nid;
+	ndr_desc.flush = async_pmem_flush;
+	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
+	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
+	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
+	if (!nd_region) {
+		dev_err(&vdev->dev, "failed to create nvdimm region\n");
+		err = -ENXIO;
+		goto out_nd;
+	}
+	nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
+	return 0;
+out_nd:
+	nvdimm_bus_unregister(vpmem->nvdimm_bus);
+out_vq:
+	vdev->config->del_vqs(vdev);
+out_err:
+	return err;
+}
+
+static void virtio_pmem_remove(struct virtio_device *vdev)
+{
+	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
+
+	nvdimm_bus_unregister(nvdimm_bus);
+	vdev->config->del_vqs(vdev);
+	vdev->config->reset(vdev);
+}
+
+static struct virtio_driver virtio_pmem_driver = {
+	.driver.name		= KBUILD_MODNAME,
+	.driver.owner		= THIS_MODULE,
+	.id_table		= id_table,
+	.probe			= virtio_pmem_probe,
+	.remove			= virtio_pmem_remove,
+};
+
+module_virtio_driver(virtio_pmem_driver);
+MODULE_DEVICE_TABLE(virtio, id_table);
+MODULE_DESCRIPTION("Virtio pmem driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
new file mode 100644
index 000000000000..ab1da877575d
--- /dev/null
+++ b/drivers/nvdimm/virtio_pmem.h
@@ -0,0 +1,60 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * virtio_pmem.h: virtio pmem Driver
+ *
+ * Discovers persistent memory range information
+ * from host and provides a virtio based flushing
+ * interface.
+ **/
+
+#ifndef _LINUX_VIRTIO_PMEM_H
+#define _LINUX_VIRTIO_PMEM_H
+
+#include <linux/virtio_ids.h>
+#include <linux/module.h>
+#include <linux/virtio_config.h>
+#include <uapi/linux/virtio_pmem.h>
+#include <linux/libnvdimm.h>
+#include <linux/spinlock.h>
+
+struct virtio_pmem_request {
+	/* Host return status corresponding to flush request */
+	int ret;
+
+	/* command name*/
+	char name[16];
+
+	/* Wait queue to process deferred work after ack from host */
+	wait_queue_head_t host_acked;
+	bool done;
+
+	/* Wait queue to process deferred work after virt queue buffer avail */
+	wait_queue_head_t wq_buf;
+	bool wq_buf_avail;
+	struct list_head list;
+};
+
+struct virtio_pmem {
+	struct virtio_device *vdev;
+
+	/* Virtio pmem request queue */
+	struct virtqueue *req_vq;
+
+	/* nvdimm bus registers virtio pmem device */
+	struct nvdimm_bus *nvdimm_bus;
+	struct nvdimm_bus_descriptor nd_desc;
+
+	/* List to store deferred work if virtqueue is full */
+	struct list_head req_list;
+
+	/* Synchronize virtqueue data */
+	spinlock_t pmem_lock;
+
+	/* Memory region information */
+	uint64_t start;
+	uint64_t size;
+};
+
+void host_ack(struct virtqueue *vq);
+int async_pmem_flush(struct nd_region *nd_region, struct bio *bio);
+#endif
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 35897649c24f..94bad084ebab 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -42,6 +42,17 @@ config VIRTIO_PCI_LEGACY
 
 	  If unsure, say Y.
 
+config VIRTIO_PMEM
+	tristate "Support for virtio pmem driver"
+	depends on VIRTIO
+	depends on LIBNVDIMM
+	help
+	This driver provides access to virtio-pmem devices, storage devices
+	that are mapped into the physical address space - similar to NVDIMMs
+	 - with a virtio-based flushing interface.
+
+	If unsure, say M.
+
 config VIRTIO_BALLOON
 	tristate "Virtio balloon driver"
 	depends on VIRTIO
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 6d5c3b2d4f4d..32b2f94d1f58 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -43,5 +43,6 @@
 #define VIRTIO_ID_INPUT        18 /* virtio input */
 #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
 #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
+#define VIRTIO_ID_PMEM         27 /* virtio pmem */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/include/uapi/linux/virtio_pmem.h b/include/uapi/linux/virtio_pmem.h
new file mode 100644
index 000000000000..fa3f7d52717a
--- /dev/null
+++ b/include/uapi/linux/virtio_pmem.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _UAPI_LINUX_VIRTIO_PMEM_H
+#define _UAPI_LINUX_VIRTIO_PMEM_H
+
+struct virtio_pmem_config {
+	__le64 start;
+	__le64 size;
+};
+#endif
-- 
2.20.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v9 3/7] libnvdimm: add dax_dev sync flag
  2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 1/7] libnvdimm: nd_region flush callback support Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver Pankaj Gupta
@ 2019-05-14 14:54 ` Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 4/7] dm: enable synchronous dax Pankaj Gupta
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

This patch adds 'DAXDEV_SYNC' flag which is set
for nd_region doing synchronous flush. This later
is used to disable MAP_SYNC functionality for
ext4 & xfs filesystem for devices don't support
synchronous flush.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 drivers/dax/bus.c            |  2 +-
 drivers/dax/super.c          | 19 ++++++++++++++++++-
 drivers/md/dm.c              |  3 ++-
 drivers/nvdimm/pmem.c        |  5 ++++-
 drivers/nvdimm/region_devs.c |  7 +++++++
 include/linux/dax.h          |  9 +++++++--
 include/linux/libnvdimm.h    |  1 +
 7 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 2109cfe80219..5f184e751c82 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -388,7 +388,7 @@ struct dev_dax *__devm_create_dev_dax(struct dax_region *dax_region, int id,
 	 * No 'host' or dax_operations since there is no access to this
 	 * device outside of mmap of the resulting character device.
 	 */
-	dax_dev = alloc_dax(dev_dax, NULL, NULL);
+	dax_dev = alloc_dax(dev_dax, NULL, NULL, DAXDEV_F_SYNC);
 	if (!dax_dev)
 		goto err;
 
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index bbd57ca0634a..93b3718b5cfa 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -186,6 +186,8 @@ enum dax_device_flags {
 	DAXDEV_ALIVE,
 	/* gate whether dax_flush() calls the low level flush routine */
 	DAXDEV_WRITE_CACHE,
+	/* flag to check if device supports synchronous flush */
+	DAXDEV_SYNC,
 };
 
 /**
@@ -354,6 +356,18 @@ bool dax_write_cache_enabled(struct dax_device *dax_dev)
 }
 EXPORT_SYMBOL_GPL(dax_write_cache_enabled);
 
+bool dax_synchronous(struct dax_device *dax_dev)
+{
+	return test_bit(DAXDEV_SYNC, &dax_dev->flags);
+}
+EXPORT_SYMBOL_GPL(dax_synchronous);
+
+void set_dax_synchronous(struct dax_device *dax_dev)
+{
+	set_bit(DAXDEV_SYNC, &dax_dev->flags);
+}
+EXPORT_SYMBOL_GPL(set_dax_synchronous);
+
 bool dax_alive(struct dax_device *dax_dev)
 {
 	lockdep_assert_held(&dax_srcu);
@@ -508,7 +522,7 @@ static void dax_add_host(struct dax_device *dax_dev, const char *host)
 }
 
 struct dax_device *alloc_dax(void *private, const char *__host,
-		const struct dax_operations *ops)
+		const struct dax_operations *ops, unsigned long flags)
 {
 	struct dax_device *dax_dev;
 	const char *host;
@@ -531,6 +545,9 @@ struct dax_device *alloc_dax(void *private, const char *__host,
 	dax_add_host(dax_dev, host);
 	dax_dev->ops = ops;
 	dax_dev->private = private;
+	if (flags & DAXDEV_F_SYNC)
+		set_dax_synchronous(dax_dev);
+
 	return dax_dev;
 
  err_dev:
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 043f0761e4a0..ee007b75d9fd 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1969,7 +1969,8 @@ static struct mapped_device *alloc_dev(int minor)
 	sprintf(md->disk->disk_name, "dm-%d", minor);
 
 	if (IS_ENABLED(CONFIG_DAX_DRIVER)) {
-		dax_dev = alloc_dax(md, md->disk->disk_name, &dm_dax_ops);
+		dax_dev = alloc_dax(md, md->disk->disk_name, &dm_dax_ops,
+							 DAXDEV_F_SYNC);
 		if (!dax_dev)
 			goto bad;
 	}
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 0279eb1da3ef..bdbd2b414d3d 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -365,6 +365,7 @@ static int pmem_attach_disk(struct device *dev,
 	struct gendisk *disk;
 	void *addr;
 	int rc;
+	unsigned long flags = 0UL;
 
 	pmem = devm_kzalloc(dev, sizeof(*pmem), GFP_KERNEL);
 	if (!pmem)
@@ -462,7 +463,9 @@ static int pmem_attach_disk(struct device *dev,
 	nvdimm_badblocks_populate(nd_region, &pmem->bb, &bb_res);
 	disk->bb = &pmem->bb;
 
-	dax_dev = alloc_dax(pmem, disk->disk_name, &pmem_dax_ops);
+	if (is_nvdimm_sync(nd_region))
+		flags = DAXDEV_F_SYNC;
+	dax_dev = alloc_dax(pmem, disk->disk_name, &pmem_dax_ops, flags);
 	if (!dax_dev) {
 		put_disk(disk);
 		return -ENOMEM;
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index b4ef7d9ff22e..f3ea5369d20a 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1197,6 +1197,13 @@ int nvdimm_has_cache(struct nd_region *nd_region)
 }
 EXPORT_SYMBOL_GPL(nvdimm_has_cache);
 
+bool is_nvdimm_sync(struct nd_region *nd_region)
+{
+	return is_nd_pmem(&nd_region->dev) &&
+		!test_bit(ND_REGION_ASYNC, &nd_region->flags);
+}
+EXPORT_SYMBOL_GPL(is_nvdimm_sync);
+
 struct conflict_context {
 	struct nd_region *nd_region;
 	resource_size_t start, size;
diff --git a/include/linux/dax.h b/include/linux/dax.h
index 0dd316a74a29..2b106752b1b8 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -7,6 +7,9 @@
 #include <linux/radix-tree.h>
 #include <asm/pgtable.h>
 
+/* Flag for synchronous flush */
+#define DAXDEV_F_SYNC (1UL << 0)
+
 typedef unsigned long dax_entry_t;
 
 struct iomap_ops;
@@ -32,18 +35,20 @@ extern struct attribute_group dax_attribute_group;
 #if IS_ENABLED(CONFIG_DAX)
 struct dax_device *dax_get_by_host(const char *host);
 struct dax_device *alloc_dax(void *private, const char *host,
-		const struct dax_operations *ops);
+		const struct dax_operations *ops, unsigned long flags);
 void put_dax(struct dax_device *dax_dev);
 void kill_dax(struct dax_device *dax_dev);
 void dax_write_cache(struct dax_device *dax_dev, bool wc);
 bool dax_write_cache_enabled(struct dax_device *dax_dev);
+bool dax_synchronous(struct dax_device *dax_dev);
+void set_dax_synchronous(struct dax_device *dax_dev);
 #else
 static inline struct dax_device *dax_get_by_host(const char *host)
 {
 	return NULL;
 }
 static inline struct dax_device *alloc_dax(void *private, const char *host,
-		const struct dax_operations *ops)
+		const struct dax_operations *ops, unsigned long flags)
 {
 	/*
 	 * Callers should check IS_ENABLED(CONFIG_DAX) to know if this
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index feb342d026f2..3238a206e563 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -264,6 +264,7 @@ void nvdimm_flush(struct nd_region *nd_region);
 int nvdimm_has_flush(struct nd_region *nd_region);
 int nvdimm_has_cache(struct nd_region *nd_region);
 int nvdimm_in_overwrite(struct nvdimm *nvdimm);
+bool is_nvdimm_sync(struct nd_region *nd_region);
 
 static inline int nvdimm_ctl(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
 		unsigned int buf_len, int *cmd_rc)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v9 4/7] dm: enable synchronous dax
  2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
                   ` (2 preceding siblings ...)
  2019-05-14 14:54 ` [PATCH v9 3/7] libnvdimm: add dax_dev sync flag Pankaj Gupta
@ 2019-05-14 14:54 ` Pankaj Gupta
  2019-05-15 20:48   ` Dan Williams
  2019-05-14 14:54 ` [PATCH v9 5/7] dax: check synchronous mapping is supported Pankaj Gupta
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

 This patch sets dax device 'DAXDEV_SYNC' flag if all the target
 devices of device mapper support synchrononous DAX. If device
 mapper consists of both synchronous and asynchronous dax devices,
 we don't set 'DAXDEV_SYNC' flag. 

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 drivers/md/dm-table.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index cde3b49b2a91..1cce626ff576 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -886,10 +886,17 @@ static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
 	return bdev_dax_supported(dev->bdev, PAGE_SIZE);
 }
 
+static int device_synchronous(struct dm_target *ti, struct dm_dev *dev,
+			       sector_t start, sector_t len, void *data)
+{
+	return dax_synchronous(dev->dax_dev);
+}
+
 static bool dm_table_supports_dax(struct dm_table *t)
 {
 	struct dm_target *ti;
 	unsigned i;
+	bool dax_sync = true;
 
 	/* Ensure that all targets support DAX. */
 	for (i = 0; i < dm_table_get_num_targets(t); i++) {
@@ -901,7 +908,14 @@ static bool dm_table_supports_dax(struct dm_table *t)
 		if (!ti->type->iterate_devices ||
 		    !ti->type->iterate_devices(ti, device_supports_dax, NULL))
 			return false;
+
+		/* Check devices support synchronous DAX */
+		if (dax_sync &&
+		    !ti->type->iterate_devices(ti, device_synchronous, NULL))
+			dax_sync = false;
 	}
+	if (dax_sync)
+		set_dax_synchronous(t->md->dax_dev);
 
 	return true;
 }
-- 
2.20.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v9 5/7] dax: check synchronous mapping is supported
  2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
                   ` (3 preceding siblings ...)
  2019-05-14 14:54 ` [PATCH v9 4/7] dm: enable synchronous dax Pankaj Gupta
@ 2019-05-14 14:54 ` Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 6/7] ext4: disable map_sync for async flush Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 7/7] xfs: " Pankaj Gupta
  6 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

This patch introduces 'daxdev_mapping_supported' helper
which checks if 'MAP_SYNC' is supported with filesystem
mapping. It also checks if corresponding dax_device is
synchronous. Virtio pmem device is asynchronous and
does not not support VM_SYNC.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 include/linux/dax.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/dax.h b/include/linux/dax.h
index 2b106752b1b8..267251a394fa 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -42,6 +42,18 @@ void dax_write_cache(struct dax_device *dax_dev, bool wc);
 bool dax_write_cache_enabled(struct dax_device *dax_dev);
 bool dax_synchronous(struct dax_device *dax_dev);
 void set_dax_synchronous(struct dax_device *dax_dev);
+/*
+ * Check if given mapping is supported by the file / underlying device.
+ */
+static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
+					    struct dax_device *dax_dev)
+{
+	if (!(vma->vm_flags & VM_SYNC))
+		return true;
+	if (!IS_DAX(file_inode(vma->vm_file)))
+		return false;
+	return dax_synchronous(dax_dev);
+}
 #else
 static inline struct dax_device *dax_get_by_host(const char *host)
 {
@@ -69,6 +81,11 @@ static inline bool dax_write_cache_enabled(struct dax_device *dax_dev)
 {
 	return false;
 }
+static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
+				struct dax_device *dax_dev)
+{
+	return !(vma->vm_flags & VM_SYNC);
+}
 #endif
 
 struct writeback_control;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v9 6/7] ext4: disable map_sync for async flush
  2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
                   ` (4 preceding siblings ...)
  2019-05-14 14:54 ` [PATCH v9 5/7] dax: check synchronous mapping is supported Pankaj Gupta
@ 2019-05-14 14:54 ` Pankaj Gupta
  2019-05-14 14:54 ` [PATCH v9 7/7] xfs: " Pankaj Gupta
  6 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

Dont support 'MAP_SYNC' with non-DAX files and DAX files
with asynchronous dax_device. Virtio pmem provides
asynchronous host page cache flush mechanism. We don't
support 'MAP_SYNC' with virtio pmem and ext4.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 98ec11f69cd4..dee549339e13 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -360,15 +360,17 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
 static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct inode *inode = file->f_mapping->host;
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	struct dax_device *dax_dev = sbi->s_daxdev;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(sbi)))
 		return -EIO;
 
 	/*
-	 * We don't support synchronous mappings for non-DAX files. At least
-	 * until someone comes with a sensible use case.
+	 * We don't support synchronous mappings for non-DAX files and
+	 * for DAX files if underneath dax_device is not synchronous.
 	 */
-	if (!IS_DAX(file_inode(file)) && (vma->vm_flags & VM_SYNC))
+	if (!daxdev_mapping_supported(vma, dax_dev))
 		return -EOPNOTSUPP;
 
 	file_accessed(file);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v9 7/7] xfs: disable map_sync for async flush
  2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
                   ` (5 preceding siblings ...)
  2019-05-14 14:54 ` [PATCH v9 6/7] ext4: disable map_sync for async flush Pankaj Gupta
@ 2019-05-14 14:54 ` " Pankaj Gupta
  6 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 14:54 UTC (permalink / raw)
  To: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron, pagupta

Dont support 'MAP_SYNC' with non-DAX files and DAX files
with asynchronous dax_device. Virtio pmem provides
asynchronous host page cache flush mechanism. We don't
support 'MAP_SYNC' with virtio pmem and xfs.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_file.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index a7ceae90110e..f17652cca5ff 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1203,11 +1203,14 @@ xfs_file_mmap(
 	struct file	*filp,
 	struct vm_area_struct *vma)
 {
+	struct dax_device 	*dax_dev;
+
+	dax_dev = xfs_find_daxdev_for_inode(file_inode(filp));
 	/*
-	 * We don't support synchronous mappings for non-DAX files. At least
-	 * until someone comes with a sensible use case.
+	 * We don't support synchronous mappings for non-DAX files and
+	 * for DAX files if underneath dax_device is not synchronous.
 	 */
-	if (!IS_DAX(file_inode(filp)) && (vma->vm_flags & VM_SYNC))
+	if (!daxdev_mapping_supported(vma, dax_dev))
 		return -EOPNOTSUPP;
 
 	file_accessed(filp);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-14 14:54 ` [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver Pankaj Gupta
@ 2019-05-14 15:09   ` Randy Dunlap
  2019-05-14 15:25     ` [Qemu-devel] " Pankaj Gupta
  2019-05-15 20:46   ` David Hildenbrand
  2019-05-17  0:12   ` Jakub Staroń
  2 siblings, 1 reply; 25+ messages in thread
From: Randy Dunlap @ 2019-05-14 15:09 UTC (permalink / raw)
  To: Pankaj Gupta, linux-nvdimm, linux-kernel, virtualization, kvm,
	linux-fsdevel, linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, jstaron

On 5/14/19 7:54 AM, Pankaj Gupta wrote:
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 35897649c24f..94bad084ebab 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -42,6 +42,17 @@ config VIRTIO_PCI_LEGACY
>  
>  	  If unsure, say Y.
>  
> +config VIRTIO_PMEM
> +	tristate "Support for virtio pmem driver"
> +	depends on VIRTIO
> +	depends on LIBNVDIMM
> +	help
> +	This driver provides access to virtio-pmem devices, storage devices
> +	that are mapped into the physical address space - similar to NVDIMMs
> +	 - with a virtio-based flushing interface.
> +
> +	If unsure, say M.

<beep>
from Documentation/process/coding-style.rst:
"Lines under a ``config`` definition
are indented with one tab, while help text is indented an additional two
spaces."

> +
>  config VIRTIO_BALLOON
>  	tristate "Virtio balloon driver"
>  	depends on VIRTIO

thanks.
-- 
~Randy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-14 15:09   ` Randy Dunlap
@ 2019-05-14 15:25     ` " Pankaj Gupta
  2019-05-15 20:46       ` Dan Williams
  0 siblings, 1 reply; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-14 15:25 UTC (permalink / raw)
  To: Randy Dunlap, dan j williams
  Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs, jack, mst,
	jasowang, david, lcapitulino, adilger kernel, zwisler, aarcange,
	dave jiang, jstaron, darrick wong, vishal l verma, david, willy,
	hch, jmoyer, nilal, lenb, kilobyte, riel, yuval shaia, stefanha,
	pbonzini, dan j williams, kwolf, tytso, xiaoguangrong eric,
	cohuck, rjw, imammedo


> On 5/14/19 7:54 AM, Pankaj Gupta wrote:
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 35897649c24f..94bad084ebab 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -42,6 +42,17 @@ config VIRTIO_PCI_LEGACY
> >  
> >  	  If unsure, say Y.
> >  
> > +config VIRTIO_PMEM
> > +	tristate "Support for virtio pmem driver"
> > +	depends on VIRTIO
> > +	depends on LIBNVDIMM
> > +	help
> > +	This driver provides access to virtio-pmem devices, storage devices
> > +	that are mapped into the physical address space - similar to NVDIMMs
> > +	 - with a virtio-based flushing interface.
> > +
> > +	If unsure, say M.
> 
> <beep>
> from Documentation/process/coding-style.rst:
> "Lines under a ``config`` definition
> are indented with one tab, while help text is indented an additional two
> spaces."

ah... I changed help text and 'checkpatch' did not say anything :( .

Will wait for Dan, If its possible to add two spaces to help text while applying
the series.

Thanks,
Pankaj
  

> 
> > +
> >  config VIRTIO_BALLOON
> >  	tristate "Virtio balloon driver"
> >  	depends on VIRTIO
> 
> thanks.
> --
> ~Randy
> 
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-14 14:54 ` [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver Pankaj Gupta
  2019-05-14 15:09   ` Randy Dunlap
@ 2019-05-15 20:46   ` David Hildenbrand
  2019-05-15 20:52     ` David Hildenbrand
                       ` (2 more replies)
  2019-05-17  0:12   ` Jakub Staroń
  2 siblings, 3 replies; 25+ messages in thread
From: David Hildenbrand @ 2019-05-15 20:46 UTC (permalink / raw)
  To: Pankaj Gupta, linux-nvdimm, linux-kernel, virtualization, kvm,
	linux-fsdevel, linux-acpi, qemu-devel, linux-ext4, linux-xfs,
	mst
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, jasowang,
	willy, rjw, hch, lenb, jack, tytso, adilger.kernel, darrick.wong,
	lcapitulino, kwolf, imammedo, jmoyer, nilal, riel, stefanha,
	aarcange, david, cohuck, xiaoguangrong.eric, pbonzini, kilobyte,
	yuval.shaia, jstaron

> +	vpmem->vdev = vdev;
> +	vdev->priv = vpmem;
> +	err = init_vq(vpmem);
> +	if (err) {
> +		dev_err(&vdev->dev, "failed to initialize virtio pmem vq's\n");
> +		goto out_err;
> +	}
> +
> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> +			start, &vpmem->start);
> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> +			size, &vpmem->size);
> +
> +	res.start = vpmem->start;
> +	res.end   = vpmem->start + vpmem->size-1;

nit: " - 1;"

> +	vpmem->nd_desc.provider_name = "virtio-pmem";
> +	vpmem->nd_desc.module = THIS_MODULE;
> +
> +	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> +						&vpmem->nd_desc);
> +	if (!vpmem->nvdimm_bus) {
> +		dev_err(&vdev->dev, "failed to register device with nvdimm_bus\n");
> +		err = -ENXIO;
> +		goto out_vq;
> +	}
> +
> +	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
> +
> +	ndr_desc.res = &res;
> +	ndr_desc.numa_node = nid;
> +	ndr_desc.flush = async_pmem_flush;
> +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> +	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> +	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> +	if (!nd_region) {
> +		dev_err(&vdev->dev, "failed to create nvdimm region\n");
> +		err = -ENXIO;
> +		goto out_nd;
> +	}
> +	nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
> +	return 0;
> +out_nd:
> +	nvdimm_bus_unregister(vpmem->nvdimm_bus);
> +out_vq:
> +	vdev->config->del_vqs(vdev);
> +out_err:
> +	return err;
> +}
> +
> +static void virtio_pmem_remove(struct virtio_device *vdev)
> +{
> +	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> +
> +	nvdimm_bus_unregister(nvdimm_bus);
> +	vdev->config->del_vqs(vdev);
> +	vdev->config->reset(vdev);
> +}
> +
> +static struct virtio_driver virtio_pmem_driver = {
> +	.driver.name		= KBUILD_MODNAME,
> +	.driver.owner		= THIS_MODULE,
> +	.id_table		= id_table,
> +	.probe			= virtio_pmem_probe,
> +	.remove			= virtio_pmem_remove,
> +};
> +
> +module_virtio_driver(virtio_pmem_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio pmem driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
> new file mode 100644
> index 000000000000..ab1da877575d
> --- /dev/null
> +++ b/drivers/nvdimm/virtio_pmem.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * virtio_pmem.h: virtio pmem Driver
> + *
> + * Discovers persistent memory range information
> + * from host and provides a virtio based flushing
> + * interface.
> + **/
> +
> +#ifndef _LINUX_VIRTIO_PMEM_H
> +#define _LINUX_VIRTIO_PMEM_H
> +
> +#include <linux/virtio_ids.h>
> +#include <linux/module.h>
> +#include <linux/virtio_config.h>
> +#include <uapi/linux/virtio_pmem.h>
> +#include <linux/libnvdimm.h>
> +#include <linux/spinlock.h>
> +
> +struct virtio_pmem_request {
> +	/* Host return status corresponding to flush request */
> +	int ret;
> +
> +	/* command name*/
> +	char name[16];

So ... why are we sending string commands and expect native-endianess
integers and don't define a proper request/response structure + request
types in include/uapi/linux/virtio_pmem.h like

struct virtio_pmem_resp {
	__virtio32 ret;
}

#define VIRTIO_PMEM_REQ_TYPE_FLUSH	1
struct virtio_pmem_req {
	__virtio16 type;
}

... and this way we also define a proper endianess format for exchange
and keep it extensible

@MST, what's your take on this?


-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-14 15:25     ` [Qemu-devel] " Pankaj Gupta
@ 2019-05-15 20:46       ` Dan Williams
  0 siblings, 0 replies; 25+ messages in thread
From: Dan Williams @ 2019-05-15 20:46 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: Randy Dunlap, linux-nvdimm, Linux Kernel Mailing List,
	virtualization, KVM list, linux-fsdevel, Linux ACPI,
	Qemu Developers, linux-ext4, linux-xfs, Jan Kara,
	Michael S. Tsirkin, Jason Wang, david, lcapitulino,
	adilger kernel, Ross Zwisler, Andrea Arcangeli, dave jiang,
	jstaron, darrick wong, vishal l verma, David Hildenbrand,
	Matthew Wilcox, Christoph Hellwig, jmoyer, Nitesh Narayan Lal,
	Len Brown, Adam Borowski, Rik van Riel, yuval shaia,
	Stefan Hajnoczi, Paolo Bonzini, Kevin Wolf, Theodore Ts'o,
	xiaoguangrong eric, cohuck, Rafael J. Wysocki, Igor Mammedov

On Tue, May 14, 2019 at 8:25 AM Pankaj Gupta <pagupta@redhat.com> wrote:
>
>
> > On 5/14/19 7:54 AM, Pankaj Gupta wrote:
> > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > index 35897649c24f..94bad084ebab 100644
> > > --- a/drivers/virtio/Kconfig
> > > +++ b/drivers/virtio/Kconfig
> > > @@ -42,6 +42,17 @@ config VIRTIO_PCI_LEGACY
> > >
> > >       If unsure, say Y.
> > >
> > > +config VIRTIO_PMEM
> > > +   tristate "Support for virtio pmem driver"
> > > +   depends on VIRTIO
> > > +   depends on LIBNVDIMM
> > > +   help
> > > +   This driver provides access to virtio-pmem devices, storage devices
> > > +   that are mapped into the physical address space - similar to NVDIMMs
> > > +    - with a virtio-based flushing interface.
> > > +
> > > +   If unsure, say M.
> >
> > <beep>
> > from Documentation/process/coding-style.rst:
> > "Lines under a ``config`` definition
> > are indented with one tab, while help text is indented an additional two
> > spaces."
>
> ah... I changed help text and 'checkpatch' did not say anything :( .
>
> Will wait for Dan, If its possible to add two spaces to help text while applying
> the series.

I'm inclined to handle this with a fixup appended to the end of the
series just so the patchwork-bot does not get confused by the content
changing from what was sent to the list.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 4/7] dm: enable synchronous dax
  2019-05-14 14:54 ` [PATCH v9 4/7] dm: enable synchronous dax Pankaj Gupta
@ 2019-05-15 20:48   ` Dan Williams
  0 siblings, 0 replies; 25+ messages in thread
From: Dan Williams @ 2019-05-15 20:48 UTC (permalink / raw)
  To: Mike Snitzer
  Cc: linux-nvdimm, Linux Kernel Mailing List, virtualization,
	KVM list, linux-fsdevel, Linux ACPI, Qemu Developers, linux-ext4,
	linux-xfs, Ross Zwisler, Vishal L Verma, Dave Jiang,
	Michael S. Tsirkin, Jason Wang, Matthew Wilcox,
	Rafael J. Wysocki, Christoph Hellwig, Len Brown, Jan Kara,
	Theodore Ts'o, Andreas Dilger, Darrick J. Wong, lcapitulino,
	Kevin Wolf, Igor Mammedov, jmoyer, Nitesh Narayan Lal,
	Rik van Riel, Stefan Hajnoczi, Andrea Arcangeli,
	David Hildenbrand, david, cohuck, Xiao Guangrong, Paolo Bonzini,
	Adam Borowski, yuval shaia, jstaron, Pankaj Gupta,
	device-mapper development

[ add Mike and dm-devel ]

Mike, any concerns with the below addition to the device-mapper-dax
implementation?

On Tue, May 14, 2019 at 7:58 AM Pankaj Gupta <pagupta@redhat.com> wrote:
>
>  This patch sets dax device 'DAXDEV_SYNC' flag if all the target
>  devices of device mapper support synchrononous DAX. If device
>  mapper consists of both synchronous and asynchronous dax devices,
>  we don't set 'DAXDEV_SYNC' flag.
>
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  drivers/md/dm-table.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index cde3b49b2a91..1cce626ff576 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -886,10 +886,17 @@ static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
>         return bdev_dax_supported(dev->bdev, PAGE_SIZE);
>  }
>
> +static int device_synchronous(struct dm_target *ti, struct dm_dev *dev,
> +                              sector_t start, sector_t len, void *data)
> +{
> +       return dax_synchronous(dev->dax_dev);
> +}
> +
>  static bool dm_table_supports_dax(struct dm_table *t)
>  {
>         struct dm_target *ti;
>         unsigned i;
> +       bool dax_sync = true;
>
>         /* Ensure that all targets support DAX. */
>         for (i = 0; i < dm_table_get_num_targets(t); i++) {
> @@ -901,7 +908,14 @@ static bool dm_table_supports_dax(struct dm_table *t)
>                 if (!ti->type->iterate_devices ||
>                     !ti->type->iterate_devices(ti, device_supports_dax, NULL))
>                         return false;
> +
> +               /* Check devices support synchronous DAX */
> +               if (dax_sync &&
> +                   !ti->type->iterate_devices(ti, device_synchronous, NULL))
> +                       dax_sync = false;
>         }
> +       if (dax_sync)
> +               set_dax_synchronous(t->md->dax_dev);
>
>         return true;
>  }
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-15 20:46   ` David Hildenbrand
@ 2019-05-15 20:52     ` David Hildenbrand
  2019-05-16  7:54       ` Pankaj Gupta
  2019-05-16  7:49     ` Pankaj Gupta
  2019-05-16 13:59     ` Michael S. Tsirkin
  2 siblings, 1 reply; 25+ messages in thread
From: David Hildenbrand @ 2019-05-15 20:52 UTC (permalink / raw)
  To: Pankaj Gupta, linux-nvdimm, linux-kernel, virtualization, kvm,
	linux-fsdevel, linux-acpi, qemu-devel, linux-ext4, linux-xfs,
	mst
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, jasowang,
	willy, rjw, hch, lenb, jack, tytso, adilger.kernel, darrick.wong,
	lcapitulino, kwolf, imammedo, jmoyer, nilal, riel, stefanha,
	aarcange, david, cohuck, xiaoguangrong.eric, pbonzini, kilobyte,
	yuval.shaia, jstaron

On 15.05.19 22:46, David Hildenbrand wrote:
>> +	vpmem->vdev = vdev;
>> +	vdev->priv = vpmem;
>> +	err = init_vq(vpmem);
>> +	if (err) {
>> +		dev_err(&vdev->dev, "failed to initialize virtio pmem vq's\n");
>> +		goto out_err;
>> +	}
>> +
>> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>> +			start, &vpmem->start);
>> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>> +			size, &vpmem->size);
>> +
>> +	res.start = vpmem->start;
>> +	res.end   = vpmem->start + vpmem->size-1;
> 
> nit: " - 1;"
> 
>> +	vpmem->nd_desc.provider_name = "virtio-pmem";
>> +	vpmem->nd_desc.module = THIS_MODULE;
>> +
>> +	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
>> +						&vpmem->nd_desc);
>> +	if (!vpmem->nvdimm_bus) {
>> +		dev_err(&vdev->dev, "failed to register device with nvdimm_bus\n");
>> +		err = -ENXIO;
>> +		goto out_vq;
>> +	}
>> +
>> +	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
>> +
>> +	ndr_desc.res = &res;
>> +	ndr_desc.numa_node = nid;
>> +	ndr_desc.flush = async_pmem_flush;
>> +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
>> +	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
>> +	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
>> +	if (!nd_region) {
>> +		dev_err(&vdev->dev, "failed to create nvdimm region\n");
>> +		err = -ENXIO;
>> +		goto out_nd;
>> +	}
>> +	nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
>> +	return 0;
>> +out_nd:
>> +	nvdimm_bus_unregister(vpmem->nvdimm_bus);
>> +out_vq:
>> +	vdev->config->del_vqs(vdev);
>> +out_err:
>> +	return err;
>> +}
>> +
>> +static void virtio_pmem_remove(struct virtio_device *vdev)
>> +{
>> +	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
>> +
>> +	nvdimm_bus_unregister(nvdimm_bus);
>> +	vdev->config->del_vqs(vdev);
>> +	vdev->config->reset(vdev);
>> +}
>> +
>> +static struct virtio_driver virtio_pmem_driver = {
>> +	.driver.name		= KBUILD_MODNAME,
>> +	.driver.owner		= THIS_MODULE,
>> +	.id_table		= id_table,
>> +	.probe			= virtio_pmem_probe,
>> +	.remove			= virtio_pmem_remove,
>> +};
>> +
>> +module_virtio_driver(virtio_pmem_driver);
>> +MODULE_DEVICE_TABLE(virtio, id_table);
>> +MODULE_DESCRIPTION("Virtio pmem driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
>> new file mode 100644
>> index 000000000000..ab1da877575d
>> --- /dev/null
>> +++ b/drivers/nvdimm/virtio_pmem.h
>> @@ -0,0 +1,60 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * virtio_pmem.h: virtio pmem Driver
>> + *
>> + * Discovers persistent memory range information
>> + * from host and provides a virtio based flushing
>> + * interface.
>> + **/
>> +
>> +#ifndef _LINUX_VIRTIO_PMEM_H
>> +#define _LINUX_VIRTIO_PMEM_H
>> +
>> +#include <linux/virtio_ids.h>
>> +#include <linux/module.h>
>> +#include <linux/virtio_config.h>
>> +#include <uapi/linux/virtio_pmem.h>
>> +#include <linux/libnvdimm.h>
>> +#include <linux/spinlock.h>
>> +
>> +struct virtio_pmem_request {
>> +	/* Host return status corresponding to flush request */
>> +	int ret;
>> +
>> +	/* command name*/
>> +	char name[16];
> 
> So ... why are we sending string commands and expect native-endianess
> integers and don't define a proper request/response structure + request
> types in include/uapi/linux/virtio_pmem.h like
> 
> struct virtio_pmem_resp {
> 	__virtio32 ret;
> }

FWIW, I wonder if we should even properly translate return values and
define types like

VIRTIO_PMEM_RESP_TYPE_OK	0
VIRTIO_PMEM_RESP_TYPE_EIO	1

..

> 
> #define VIRTIO_PMEM_REQ_TYPE_FLUSH	1
> struct virtio_pmem_req {
> 	__virtio16 type;
> }
> 
> ... and this way we also define a proper endianess format for exchange
> and keep it extensible
> 
> @MST, what's your take on this?
> 
> 


-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 1/7] libnvdimm: nd_region flush callback support
  2019-05-14 14:54 ` [PATCH v9 1/7] libnvdimm: nd_region flush callback support Pankaj Gupta
@ 2019-05-15 21:07   ` Dan Williams
  2019-05-16  6:28     ` Pankaj Gupta
  0 siblings, 1 reply; 25+ messages in thread
From: Dan Williams @ 2019-05-15 21:07 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: linux-nvdimm, Linux Kernel Mailing List, virtualization,
	KVM list, linux-fsdevel, Linux ACPI, Qemu Developers, linux-ext4,
	linux-xfs, Ross Zwisler, Vishal L Verma, Dave Jiang,
	Michael S. Tsirkin, Jason Wang, Matthew Wilcox,
	Rafael J. Wysocki, Christoph Hellwig, Len Brown, Jan Kara,
	Theodore Ts'o, Andreas Dilger, Darrick J. Wong, lcapitulino,
	Kevin Wolf, Igor Mammedov, jmoyer, Nitesh Narayan Lal,
	Rik van Riel, Stefan Hajnoczi, Andrea Arcangeli,
	David Hildenbrand, david, cohuck, Xiao Guangrong, Paolo Bonzini,
	Adam Borowski, yuval shaia, jstaron

On Tue, May 14, 2019 at 7:55 AM Pankaj Gupta <pagupta@redhat.com> wrote:
>
> This patch adds functionality to perform flush from guest
> to host over VIRTIO. We are registering a callback based
> on 'nd_region' type. virtio_pmem driver requires this special
> flush function. For rest of the region types we are registering
> existing flush function. Report error returned by host fsync
> failure to userspace.
>
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  drivers/acpi/nfit/core.c     |  4 ++--
>  drivers/nvdimm/claim.c       |  6 ++++--
>  drivers/nvdimm/nd.h          |  1 +
>  drivers/nvdimm/pmem.c        | 13 ++++++++-----
>  drivers/nvdimm/region_devs.c | 26 ++++++++++++++++++++++++--
>  include/linux/libnvdimm.h    |  8 +++++++-
>  6 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 5a389a4f4f65..08dde76cf459 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2434,7 +2434,7 @@ static void write_blk_ctl(struct nfit_blk *nfit_blk, unsigned int bw,
>                 offset = to_interleave_offset(offset, mmio);
>
>         writeq(cmd, mmio->addr.base + offset);
> -       nvdimm_flush(nfit_blk->nd_region);
> +       nvdimm_flush(nfit_blk->nd_region, NULL);
>
>         if (nfit_blk->dimm_flags & NFIT_BLK_DCR_LATCH)
>                 readq(mmio->addr.base + offset);
> @@ -2483,7 +2483,7 @@ static int acpi_nfit_blk_single_io(struct nfit_blk *nfit_blk,
>         }
>
>         if (rw)
> -               nvdimm_flush(nfit_blk->nd_region);
> +               nvdimm_flush(nfit_blk->nd_region, NULL);
>
>         rc = read_blk_stat(nfit_blk, lane) ? -EIO : 0;
>         return rc;
> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
> index fb667bf469c7..13510bae1e6f 100644
> --- a/drivers/nvdimm/claim.c
> +++ b/drivers/nvdimm/claim.c
> @@ -263,7 +263,7 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns,
>         struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
>         unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
>         sector_t sector = offset >> 9;
> -       int rc = 0;
> +       int rc = 0, ret = 0;
>
>         if (unlikely(!size))
>                 return 0;
> @@ -301,7 +301,9 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns,
>         }
>
>         memcpy_flushcache(nsio->addr + offset, buf, size);
> -       nvdimm_flush(to_nd_region(ndns->dev.parent));
> +       ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL);
> +       if (ret)
> +               rc = ret;
>
>         return rc;
>  }
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index a5ac3b240293..0c74d2428bd7 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -159,6 +159,7 @@ struct nd_region {
>         struct badblocks bb;
>         struct nd_interleave_set *nd_set;
>         struct nd_percpu_lane __percpu *lane;
> +       int (*flush)(struct nd_region *nd_region, struct bio *bio);

So this triggers:

In file included from drivers/nvdimm/e820.c:7:
./include/linux/libnvdimm.h:140:51: warning: ‘struct bio’ declared
inside parameter list will not be visible outside of this definition
or declaration
  int (*flush)(struct nd_region *nd_region, struct bio *bio);
                                                   ^~~
I was already feeling uneasy about trying to squeeze this into v5.2,
but this warning and the continued drip of comments leads me to
conclude that this driver would do well to wait one more development
cycle. Lets close out the final fixups and let this driver soak in
-next. Then for the v5.3 cycle I'll redouble my efforts towards the
goal of closing patch acceptance at the -rc6 / -rc7 development
milestone.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 1/7] libnvdimm: nd_region flush callback support
  2019-05-15 21:07   ` Dan Williams
@ 2019-05-16  6:28     ` Pankaj Gupta
  0 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-16  6:28 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm, Linux Kernel Mailing List, virtualization,
	KVM list, linux-fsdevel, Linux ACPI, Qemu Developers, linux-ext4,
	linux-xfs, Ross Zwisler, Vishal L Verma, Dave Jiang,
	Michael S. Tsirkin, Jason Wang, Matthew Wilcox,
	Rafael J. Wysocki, Christoph Hellwig, Len Brown, Jan Kara,
	Theodore Ts'o, Andreas Dilger, Darrick J. Wong, lcapitulino,
	Kevin Wolf, Igor Mammedov, jmoyer, Nitesh Narayan Lal,
	Rik van Riel, Stefan Hajnoczi, Andrea Arcangeli,
	David Hildenbrand, david, cohuck, Xiao Guangrong, Paolo Bonzini,
	Adam Borowski, yuval shaia, jstaron


> >
> > This patch adds functionality to perform flush from guest
> > to host over VIRTIO. We are registering a callback based
> > on 'nd_region' type. virtio_pmem driver requires this special
> > flush function. For rest of the region types we are registering
> > existing flush function. Report error returned by host fsync
> > failure to userspace.
> >
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  drivers/acpi/nfit/core.c     |  4 ++--
> >  drivers/nvdimm/claim.c       |  6 ++++--
> >  drivers/nvdimm/nd.h          |  1 +
> >  drivers/nvdimm/pmem.c        | 13 ++++++++-----
> >  drivers/nvdimm/region_devs.c | 26 ++++++++++++++++++++++++--
> >  include/linux/libnvdimm.h    |  8 +++++++-
> >  6 files changed, 46 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 5a389a4f4f65..08dde76cf459 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -2434,7 +2434,7 @@ static void write_blk_ctl(struct nfit_blk *nfit_blk,
> > unsigned int bw,
> >                 offset = to_interleave_offset(offset, mmio);
> >
> >         writeq(cmd, mmio->addr.base + offset);
> > -       nvdimm_flush(nfit_blk->nd_region);
> > +       nvdimm_flush(nfit_blk->nd_region, NULL);
> >
> >         if (nfit_blk->dimm_flags & NFIT_BLK_DCR_LATCH)
> >                 readq(mmio->addr.base + offset);
> > @@ -2483,7 +2483,7 @@ static int acpi_nfit_blk_single_io(struct nfit_blk
> > *nfit_blk,
> >         }
> >
> >         if (rw)
> > -               nvdimm_flush(nfit_blk->nd_region);
> > +               nvdimm_flush(nfit_blk->nd_region, NULL);
> >
> >         rc = read_blk_stat(nfit_blk, lane) ? -EIO : 0;
> >         return rc;
> > diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
> > index fb667bf469c7..13510bae1e6f 100644
> > --- a/drivers/nvdimm/claim.c
> > +++ b/drivers/nvdimm/claim.c
> > @@ -263,7 +263,7 @@ static int nsio_rw_bytes(struct nd_namespace_common
> > *ndns,
> >         struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
> >         unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512);
> >         sector_t sector = offset >> 9;
> > -       int rc = 0;
> > +       int rc = 0, ret = 0;
> >
> >         if (unlikely(!size))
> >                 return 0;
> > @@ -301,7 +301,9 @@ static int nsio_rw_bytes(struct nd_namespace_common
> > *ndns,
> >         }
> >
> >         memcpy_flushcache(nsio->addr + offset, buf, size);
> > -       nvdimm_flush(to_nd_region(ndns->dev.parent));
> > +       ret = nvdimm_flush(to_nd_region(ndns->dev.parent), NULL);
> > +       if (ret)
> > +               rc = ret;
> >
> >         return rc;
> >  }
> > diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> > index a5ac3b240293..0c74d2428bd7 100644
> > --- a/drivers/nvdimm/nd.h
> > +++ b/drivers/nvdimm/nd.h
> > @@ -159,6 +159,7 @@ struct nd_region {
> >         struct badblocks bb;
> >         struct nd_interleave_set *nd_set;
> >         struct nd_percpu_lane __percpu *lane;
> > +       int (*flush)(struct nd_region *nd_region, struct bio *bio);
> 
> So this triggers:
> 
> In file included from drivers/nvdimm/e820.c:7:
> ./include/linux/libnvdimm.h:140:51: warning: ‘struct bio’ declared
> inside parameter list will not be visible outside of this definition
> or declaration
>   int (*flush)(struct nd_region *nd_region, struct bio *bio);
>                                                    ^~~

Sorry! for this. Fixed now.

> I was already feeling uneasy about trying to squeeze this into v5.2,
> but this warning and the continued drip of comments leads me to
> conclude that this driver would do well to wait one more development
> cycle. Lets close out the final fixups and let this driver soak in
> -next. Then for the v5.3 cycle I'll redouble my efforts towards the
> goal of closing patch acceptance at the -rc6 / -rc7 development
> milestone.

o.k. Will wait for Mike's ACK on device mapper patch and send the v10
with final fix-ups. Thank you for your help.

Best regards,
Pankaj



> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-15 20:46   ` David Hildenbrand
  2019-05-15 20:52     ` David Hildenbrand
@ 2019-05-16  7:49     ` Pankaj Gupta
  2019-05-16 13:59     ` Michael S. Tsirkin
  2 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-16  7:49 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs, mst,
	dan j williams, zwisler, vishal l verma, dave jiang, jasowang,
	willy, rjw, hch, lenb, jack, tytso, adilger kernel, darrick wong,
	lcapitulino, kwolf, imammedo, jmoyer, nilal, riel, stefanha,
	aarcange, david, cohuck, xiaoguangrong eric, pbonzini, kilobyte,
	yuval shaia, jstaron


> 
> > +	vpmem->vdev = vdev;
> > +	vdev->priv = vpmem;
> > +	err = init_vq(vpmem);
> > +	if (err) {
> > +		dev_err(&vdev->dev, "failed to initialize virtio pmem vq's\n");
> > +		goto out_err;
> > +	}
> > +
> > +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > +			start, &vpmem->start);
> > +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > +			size, &vpmem->size);
> > +
> > +	res.start = vpmem->start;
> > +	res.end   = vpmem->start + vpmem->size-1;
> 
> nit: " - 1;"

Sure.

> 
> > +	vpmem->nd_desc.provider_name = "virtio-pmem";
> > +	vpmem->nd_desc.module = THIS_MODULE;
> > +
> > +	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > +						&vpmem->nd_desc);
> > +	if (!vpmem->nvdimm_bus) {
> > +		dev_err(&vdev->dev, "failed to register device with nvdimm_bus\n");
> > +		err = -ENXIO;
> > +		goto out_vq;
> > +	}
> > +
> > +	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
> > +
> > +	ndr_desc.res = &res;
> > +	ndr_desc.numa_node = nid;
> > +	ndr_desc.flush = async_pmem_flush;
> > +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > +	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > +	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> > +	if (!nd_region) {
> > +		dev_err(&vdev->dev, "failed to create nvdimm region\n");
> > +		err = -ENXIO;
> > +		goto out_nd;
> > +	}
> > +	nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
> > +	return 0;
> > +out_nd:
> > +	nvdimm_bus_unregister(vpmem->nvdimm_bus);
> > +out_vq:
> > +	vdev->config->del_vqs(vdev);
> > +out_err:
> > +	return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > +	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > +	nvdimm_bus_unregister(nvdimm_bus);
> > +	vdev->config->del_vqs(vdev);
> > +	vdev->config->reset(vdev);
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > +	.driver.name		= KBUILD_MODNAME,
> > +	.driver.owner		= THIS_MODULE,
> > +	.id_table		= id_table,
> > +	.probe			= virtio_pmem_probe,
> > +	.remove			= virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
> > new file mode 100644
> > index 000000000000..ab1da877575d
> > --- /dev/null
> > +++ b/drivers/nvdimm/virtio_pmem.h
> > @@ -0,0 +1,60 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * virtio_pmem.h: virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and provides a virtio based flushing
> > + * interface.
> > + **/
> > +
> > +#ifndef _LINUX_VIRTIO_PMEM_H
> > +#define _LINUX_VIRTIO_PMEM_H
> > +
> > +#include <linux/virtio_ids.h>
> > +#include <linux/module.h>
> > +#include <linux/virtio_config.h>
> > +#include <uapi/linux/virtio_pmem.h>
> > +#include <linux/libnvdimm.h>
> > +#include <linux/spinlock.h>
> > +
> > +struct virtio_pmem_request {
> > +	/* Host return status corresponding to flush request */
> > +	int ret;
> > +
> > +	/* command name*/
> > +	char name[16];
> 
> So ... why are we sending string commands and expect native-endianess
> integers and don't define a proper request/response structure + request
> types in include/uapi/linux/virtio_pmem.h like
> 
> struct virtio_pmem_resp {
> 	__virtio32 ret;
> }
> 
> #define VIRTIO_PMEM_REQ_TYPE_FLUSH	1
> struct virtio_pmem_req {
> 	__virtio16 type;
> }
> 
> ... and this way we also define a proper endianess format for exchange
> and keep it extensible

Done the suggested change.

Thank you,
Pankaj

> 
> @MST, what's your take on this?
> 
> 
> --
> 
> Thanks,
> 
> David / dhildenb
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-15 20:52     ` David Hildenbrand
@ 2019-05-16  7:54       ` Pankaj Gupta
  0 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-16  7:54 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs, mst,
	dan j williams, zwisler, vishal l verma, dave jiang, jasowang,
	willy, rjw, hch, lenb, jack, tytso, adilger kernel, darrick wong,
	lcapitulino, kwolf, imammedo, jmoyer, nilal, riel, stefanha,
	aarcange, david, cohuck, xiaoguangrong eric, pbonzini, kilobyte,
	yuval shaia, jstaron


> >> +	vpmem->vdev = vdev;
> >> +	vdev->priv = vpmem;
> >> +	err = init_vq(vpmem);
> >> +	if (err) {
> >> +		dev_err(&vdev->dev, "failed to initialize virtio pmem vq's\n");
> >> +		goto out_err;
> >> +	}
> >> +
> >> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> >> +			start, &vpmem->start);
> >> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> >> +			size, &vpmem->size);
> >> +
> >> +	res.start = vpmem->start;
> >> +	res.end   = vpmem->start + vpmem->size-1;
> > 
> > nit: " - 1;"
> > 
> >> +	vpmem->nd_desc.provider_name = "virtio-pmem";
> >> +	vpmem->nd_desc.module = THIS_MODULE;
> >> +
> >> +	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> >> +						&vpmem->nd_desc);
> >> +	if (!vpmem->nvdimm_bus) {
> >> +		dev_err(&vdev->dev, "failed to register device with nvdimm_bus\n");
> >> +		err = -ENXIO;
> >> +		goto out_vq;
> >> +	}
> >> +
> >> +	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
> >> +
> >> +	ndr_desc.res = &res;
> >> +	ndr_desc.numa_node = nid;
> >> +	ndr_desc.flush = async_pmem_flush;
> >> +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> >> +	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> >> +	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> >> +	if (!nd_region) {
> >> +		dev_err(&vdev->dev, "failed to create nvdimm region\n");
> >> +		err = -ENXIO;
> >> +		goto out_nd;
> >> +	}
> >> +	nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
> >> +	return 0;
> >> +out_nd:
> >> +	nvdimm_bus_unregister(vpmem->nvdimm_bus);
> >> +out_vq:
> >> +	vdev->config->del_vqs(vdev);
> >> +out_err:
> >> +	return err;
> >> +}
> >> +
> >> +static void virtio_pmem_remove(struct virtio_device *vdev)
> >> +{
> >> +	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> >> +
> >> +	nvdimm_bus_unregister(nvdimm_bus);
> >> +	vdev->config->del_vqs(vdev);
> >> +	vdev->config->reset(vdev);
> >> +}
> >> +
> >> +static struct virtio_driver virtio_pmem_driver = {
> >> +	.driver.name		= KBUILD_MODNAME,
> >> +	.driver.owner		= THIS_MODULE,
> >> +	.id_table		= id_table,
> >> +	.probe			= virtio_pmem_probe,
> >> +	.remove			= virtio_pmem_remove,
> >> +};
> >> +
> >> +module_virtio_driver(virtio_pmem_driver);
> >> +MODULE_DEVICE_TABLE(virtio, id_table);
> >> +MODULE_DESCRIPTION("Virtio pmem driver");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
> >> new file mode 100644
> >> index 000000000000..ab1da877575d
> >> --- /dev/null
> >> +++ b/drivers/nvdimm/virtio_pmem.h
> >> @@ -0,0 +1,60 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * virtio_pmem.h: virtio pmem Driver
> >> + *
> >> + * Discovers persistent memory range information
> >> + * from host and provides a virtio based flushing
> >> + * interface.
> >> + **/
> >> +
> >> +#ifndef _LINUX_VIRTIO_PMEM_H
> >> +#define _LINUX_VIRTIO_PMEM_H
> >> +
> >> +#include <linux/virtio_ids.h>
> >> +#include <linux/module.h>
> >> +#include <linux/virtio_config.h>
> >> +#include <uapi/linux/virtio_pmem.h>
> >> +#include <linux/libnvdimm.h>
> >> +#include <linux/spinlock.h>
> >> +
> >> +struct virtio_pmem_request {
> >> +	/* Host return status corresponding to flush request */
> >> +	int ret;
> >> +
> >> +	/* command name*/
> >> +	char name[16];
> > 
> > So ... why are we sending string commands and expect native-endianess
> > integers and don't define a proper request/response structure + request
> > types in include/uapi/linux/virtio_pmem.h like
> > 
> > struct virtio_pmem_resp {
> > 	__virtio32 ret;
> > }
> 
> FWIW, I wonder if we should even properly translate return values and
> define types like
> 
> VIRTIO_PMEM_RESP_TYPE_OK	0
> VIRTIO_PMEM_RESP_TYPE_EIO	1

Don't think these are required as only failure and success
return types easy to understand.

Thanks,
Pankaj
> 
> ..
> 
> > 
> > #define VIRTIO_PMEM_REQ_TYPE_FLUSH	1
> > struct virtio_pmem_req {
> > 	__virtio16 type;
> > }
> > 
> > ... and this way we also define a proper endianess format for exchange
> > and keep it extensible
> > 
> > @MST, what's your take on this?
> > 
> > 
> 
> 
> --
> 
> Thanks,
> 
> David / dhildenb
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-15 20:46   ` David Hildenbrand
  2019-05-15 20:52     ` David Hildenbrand
  2019-05-16  7:49     ` Pankaj Gupta
@ 2019-05-16 13:59     ` Michael S. Tsirkin
  2019-05-17  5:31       ` [Qemu-devel] " Pankaj Gupta
  2 siblings, 1 reply; 25+ messages in thread
From: Michael S. Tsirkin @ 2019-05-16 13:59 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Pankaj Gupta, linux-nvdimm, linux-kernel, virtualization, kvm,
	linux-fsdevel, linux-acpi, qemu-devel, linux-ext4, linux-xfs,
	dan.j.williams, zwisler, vishal.l.verma, dave.jiang, jasowang,
	willy, rjw, hch, lenb, jack, tytso, adilger.kernel, darrick.wong,
	lcapitulino, kwolf, imammedo, jmoyer, nilal, riel, stefanha,
	aarcange, david, cohuck, xiaoguangrong.eric, pbonzini, kilobyte,
	yuval.shaia, jstaron

On Wed, May 15, 2019 at 10:46:00PM +0200, David Hildenbrand wrote:
> > +	vpmem->vdev = vdev;
> > +	vdev->priv = vpmem;
> > +	err = init_vq(vpmem);
> > +	if (err) {
> > +		dev_err(&vdev->dev, "failed to initialize virtio pmem vq's\n");
> > +		goto out_err;
> > +	}
> > +
> > +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > +			start, &vpmem->start);
> > +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > +			size, &vpmem->size);
> > +
> > +	res.start = vpmem->start;
> > +	res.end   = vpmem->start + vpmem->size-1;
> 
> nit: " - 1;"
> 
> > +	vpmem->nd_desc.provider_name = "virtio-pmem";
> > +	vpmem->nd_desc.module = THIS_MODULE;
> > +
> > +	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > +						&vpmem->nd_desc);
> > +	if (!vpmem->nvdimm_bus) {
> > +		dev_err(&vdev->dev, "failed to register device with nvdimm_bus\n");
> > +		err = -ENXIO;
> > +		goto out_vq;
> > +	}
> > +
> > +	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
> > +
> > +	ndr_desc.res = &res;
> > +	ndr_desc.numa_node = nid;
> > +	ndr_desc.flush = async_pmem_flush;
> > +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > +	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > +	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> > +	if (!nd_region) {
> > +		dev_err(&vdev->dev, "failed to create nvdimm region\n");
> > +		err = -ENXIO;
> > +		goto out_nd;
> > +	}
> > +	nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
> > +	return 0;
> > +out_nd:
> > +	nvdimm_bus_unregister(vpmem->nvdimm_bus);
> > +out_vq:
> > +	vdev->config->del_vqs(vdev);
> > +out_err:
> > +	return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > +	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > +	nvdimm_bus_unregister(nvdimm_bus);
> > +	vdev->config->del_vqs(vdev);
> > +	vdev->config->reset(vdev);
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > +	.driver.name		= KBUILD_MODNAME,
> > +	.driver.owner		= THIS_MODULE,
> > +	.id_table		= id_table,
> > +	.probe			= virtio_pmem_probe,
> > +	.remove			= virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
> > new file mode 100644
> > index 000000000000..ab1da877575d
> > --- /dev/null
> > +++ b/drivers/nvdimm/virtio_pmem.h
> > @@ -0,0 +1,60 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * virtio_pmem.h: virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and provides a virtio based flushing
> > + * interface.
> > + **/
> > +
> > +#ifndef _LINUX_VIRTIO_PMEM_H
> > +#define _LINUX_VIRTIO_PMEM_H
> > +
> > +#include <linux/virtio_ids.h>
> > +#include <linux/module.h>
> > +#include <linux/virtio_config.h>
> > +#include <uapi/linux/virtio_pmem.h>
> > +#include <linux/libnvdimm.h>
> > +#include <linux/spinlock.h>
> > +
> > +struct virtio_pmem_request {
> > +	/* Host return status corresponding to flush request */
> > +	int ret;
> > +
> > +	/* command name*/
> > +	char name[16];
> 
> So ... why are we sending string commands and expect native-endianess
> integers and don't define a proper request/response structure + request
> types in include/uapi/linux/virtio_pmem.h like

passing names could be ok.
I missed the fact we return a native endian int.
Pls fix that.


> 
> struct virtio_pmem_resp {
> 	__virtio32 ret;
> }
> 
> #define VIRTIO_PMEM_REQ_TYPE_FLUSH	1
> struct virtio_pmem_req {
> 	__virtio16 type;
> }
> 
> ... and this way we also define a proper endianess format for exchange
> and keep it extensible
> 
> @MST, what's your take on this?

Extensions can always use feature bits so I don't think
it's a problem.

> 
> -- 
> 
> Thanks,
> 
> David / dhildenb

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-14 14:54 ` [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver Pankaj Gupta
  2019-05-14 15:09   ` Randy Dunlap
  2019-05-15 20:46   ` David Hildenbrand
@ 2019-05-17  0:12   ` Jakub Staroń
  2019-05-17  5:35     ` [Qemu-devel] " Pankaj Gupta
  2 siblings, 1 reply; 25+ messages in thread
From: Jakub Staroń @ 2019-05-17  0:12 UTC (permalink / raw)
  To: Pankaj Gupta, linux-nvdimm, linux-kernel, virtualization, kvm,
	linux-fsdevel, linux-acpi, qemu-devel, linux-ext4, linux-xfs
  Cc: dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
	jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
	darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
	stefanha, aarcange, david, david, cohuck, xiaoguangrong.eric,
	pbonzini, kilobyte, yuval.shaia, smbarber

On 5/14/19 7:54 AM, Pankaj Gupta wrote:
> +		if (!list_empty(&vpmem->req_list)) {
> +			req_buf = list_first_entry(&vpmem->req_list,
> +					struct virtio_pmem_request, list);
> +			req_buf->wq_buf_avail = true;
> +			wake_up(&req_buf->wq_buf);
> +			list_del(&req_buf->list);
Yes, this change is the right one, thank you!

> +	 /*
> +	  * If virtqueue_add_sgs returns -ENOSPC then req_vq virtual
> +	  * queue does not have free descriptor. We add the request
> +	  * to req_list and wait for host_ack to wake us up when free
> +	  * slots are available.
> +	  */
> +	while ((err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req,
> +					GFP_ATOMIC)) == -ENOSPC) {
> +
> +		dev_err(&vdev->dev, "failed to send command to virtio pmem" \
> +			"device, no free slots in the virtqueue\n");
> +		req->wq_buf_avail = false;
> +		list_add_tail(&req->list, &vpmem->req_list);
> +		spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +
> +		/* A host response results in "host_ack" getting called */
> +		wait_event(req->wq_buf, req->wq_buf_avail);
> +		spin_lock_irqsave(&vpmem->pmem_lock, flags);
> +	}
> +	err1 = virtqueue_kick(vpmem->req_vq);
> +	spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +
> +	/*
> +	 * virtqueue_add_sgs failed with error different than -ENOSPC, we can't
> +	 * do anything about that.
> +	 */
> +	if (err || !err1) {
> +		dev_info(&vdev->dev, "failed to send command to virtio pmem device\n");
> +		err = -EIO;
> +	} else {
> +		/* A host repsonse results in "host_ack" getting called */
> +		wait_event(req->host_acked, req->done);
> +		err = req->ret;
> +I confirm that the failures I was facing with the `-ENOSPC` error path are not present in v9.

Best,
Jakub Staron

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-16 13:59     ` Michael S. Tsirkin
@ 2019-05-17  5:31       ` " Pankaj Gupta
  0 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-17  5:31 UTC (permalink / raw)
  To: Michael S. Tsirkin, David Hildenbrand
  Cc: cohuck, jack, kvm, jasowang, david, qemu-devel, virtualization,
	adilger kernel, zwisler, aarcange, dave jiang, jstaron,
	linux-nvdimm, vishal l verma, willy, hch, linux-acpi, jmoyer,
	linux-ext4, lenb, kilobyte, riel, yuval shaia, stefanha,
	pbonzini, dan j williams, lcapitulino, kwolf, nilal, tytso,
	xiaoguangrong eric, darrick wong, rjw, linux-kernel, linux-xfs,
	linux-fsdevel, imammedo


> 
> On Wed, May 15, 2019 at 10:46:00PM +0200, David Hildenbrand wrote:
> > > +	vpmem->vdev = vdev;
> > > +	vdev->priv = vpmem;
> > > +	err = init_vq(vpmem);
> > > +	if (err) {
> > > +		dev_err(&vdev->dev, "failed to initialize virtio pmem vq's\n");
> > > +		goto out_err;
> > > +	}
> > > +
> > > +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > > +			start, &vpmem->start);
> > > +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > > +			size, &vpmem->size);
> > > +
> > > +	res.start = vpmem->start;
> > > +	res.end   = vpmem->start + vpmem->size-1;
> > 
> > nit: " - 1;"
> > 
> > > +	vpmem->nd_desc.provider_name = "virtio-pmem";
> > > +	vpmem->nd_desc.module = THIS_MODULE;
> > > +
> > > +	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > > +						&vpmem->nd_desc);
> > > +	if (!vpmem->nvdimm_bus) {
> > > +		dev_err(&vdev->dev, "failed to register device with nvdimm_bus\n");
> > > +		err = -ENXIO;
> > > +		goto out_vq;
> > > +	}
> > > +
> > > +	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
> > > +
> > > +	ndr_desc.res = &res;
> > > +	ndr_desc.numa_node = nid;
> > > +	ndr_desc.flush = async_pmem_flush;
> > > +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > > +	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > > +	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> > > +	if (!nd_region) {
> > > +		dev_err(&vdev->dev, "failed to create nvdimm region\n");
> > > +		err = -ENXIO;
> > > +		goto out_nd;
> > > +	}
> > > +	nd_region->provider_data =
> > > dev_to_virtio(nd_region->dev.parent->parent);
> > > +	return 0;
> > > +out_nd:
> > > +	nvdimm_bus_unregister(vpmem->nvdimm_bus);
> > > +out_vq:
> > > +	vdev->config->del_vqs(vdev);
> > > +out_err:
> > > +	return err;
> > > +}
> > > +
> > > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > > +{
> > > +	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > > +
> > > +	nvdimm_bus_unregister(nvdimm_bus);
> > > +	vdev->config->del_vqs(vdev);
> > > +	vdev->config->reset(vdev);
> > > +}
> > > +
> > > +static struct virtio_driver virtio_pmem_driver = {
> > > +	.driver.name		= KBUILD_MODNAME,
> > > +	.driver.owner		= THIS_MODULE,
> > > +	.id_table		= id_table,
> > > +	.probe			= virtio_pmem_probe,
> > > +	.remove			= virtio_pmem_remove,
> > > +};
> > > +
> > > +module_virtio_driver(virtio_pmem_driver);
> > > +MODULE_DEVICE_TABLE(virtio, id_table);
> > > +MODULE_DESCRIPTION("Virtio pmem driver");
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/drivers/nvdimm/virtio_pmem.h b/drivers/nvdimm/virtio_pmem.h
> > > new file mode 100644
> > > index 000000000000..ab1da877575d
> > > --- /dev/null
> > > +++ b/drivers/nvdimm/virtio_pmem.h
> > > @@ -0,0 +1,60 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * virtio_pmem.h: virtio pmem Driver
> > > + *
> > > + * Discovers persistent memory range information
> > > + * from host and provides a virtio based flushing
> > > + * interface.
> > > + **/
> > > +
> > > +#ifndef _LINUX_VIRTIO_PMEM_H
> > > +#define _LINUX_VIRTIO_PMEM_H
> > > +
> > > +#include <linux/virtio_ids.h>
> > > +#include <linux/module.h>
> > > +#include <linux/virtio_config.h>
> > > +#include <uapi/linux/virtio_pmem.h>
> > > +#include <linux/libnvdimm.h>
> > > +#include <linux/spinlock.h>
> > > +
> > > +struct virtio_pmem_request {
> > > +	/* Host return status corresponding to flush request */
> > > +	int ret;
> > > +
> > > +	/* command name*/
> > > +	char name[16];
> > 
> > So ... why are we sending string commands and expect native-endianess
> > integers and don't define a proper request/response structure + request
> > types in include/uapi/linux/virtio_pmem.h like
> 
> passing names could be ok.
> I missed the fact we return a native endian int.
> Pls fix that.

Sure. will fix this.

> 
> 
> > 
> > struct virtio_pmem_resp {
> > 	__virtio32 ret;
> > }
> > 
> > #define VIRTIO_PMEM_REQ_TYPE_FLUSH	1
> > struct virtio_pmem_req {
> > 	__virtio16 type;
> > }
> > 
> > ... and this way we also define a proper endianess format for exchange
> > and keep it extensible
> > 
> > @MST, what's your take on this?
> 
> Extensions can always use feature bits so I don't think
> it's a problem.

That was exactly my thought when I implemented this. Though I am
fine with separate structures for request/response and I made the
change. 

Thank you for all the comments.

Best regards,
Pankaj 
> > 
> > --
> > 
> > Thanks,
> > 
> > David / dhildenb
> 
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-17  0:12   ` Jakub Staroń
@ 2019-05-17  5:35     ` " Pankaj Gupta
  2019-05-18  1:10       ` Jakub Staroń
  0 siblings, 1 reply; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-17  5:35 UTC (permalink / raw)
  To: Jakub Staroń
  Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs, jack, mst,
	jasowang, david, lcapitulino, adilger kernel, smbarber, zwisler,
	aarcange, dave jiang, darrick wong, vishal l verma, david, willy,
	hch, jmoyer, nilal, lenb, kilobyte, riel, yuval shaia, stefanha,
	pbonzini, dan j williams, kwolf, tytso, xiaoguangrong eric,
	cohuck, rjw, imammedo



Hi Jakub,

> 
> On 5/14/19 7:54 AM, Pankaj Gupta wrote:
> > +		if (!list_empty(&vpmem->req_list)) {
> > +			req_buf = list_first_entry(&vpmem->req_list,
> > +					struct virtio_pmem_request, list);
> > +			req_buf->wq_buf_avail = true;
> > +			wake_up(&req_buf->wq_buf);
> > +			list_del(&req_buf->list);
> Yes, this change is the right one, thank you!

Thank you for the confirmation.

> 
> > +	 /*
> > +	  * If virtqueue_add_sgs returns -ENOSPC then req_vq virtual
> > +	  * queue does not have free descriptor. We add the request
> > +	  * to req_list and wait for host_ack to wake us up when free
> > +	  * slots are available.
> > +	  */
> > +	while ((err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req,
> > +					GFP_ATOMIC)) == -ENOSPC) {
> > +
> > +		dev_err(&vdev->dev, "failed to send command to virtio pmem" \
> > +			"device, no free slots in the virtqueue\n");
> > +		req->wq_buf_avail = false;
> > +		list_add_tail(&req->list, &vpmem->req_list);
> > +		spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +
> > +		/* A host response results in "host_ack" getting called */
> > +		wait_event(req->wq_buf, req->wq_buf_avail);
> > +		spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > +	}
> > +	err1 = virtqueue_kick(vpmem->req_vq);
> > +	spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +
> > +	/*
> > +	 * virtqueue_add_sgs failed with error different than -ENOSPC, we can't
> > +	 * do anything about that.
> > +	 */
> > +	if (err || !err1) {
> > +		dev_info(&vdev->dev, "failed to send command to virtio pmem device\n");
> > +		err = -EIO;
> > +	} else {
> > +		/* A host repsonse results in "host_ack" getting called */
> > +		wait_event(req->host_acked, req->done);
> > +		err = req->ret;
> > +I confirm that the failures I was facing with the `-ENOSPC` error path are
> > not present in v9.

Can I take it your reviewed/acked-by? or tested-by tag? for the virtio patch :)

Thank you,
Pankaj

> 
> Best,
> Jakub Staron
> 
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-17  5:35     ` [Qemu-devel] " Pankaj Gupta
@ 2019-05-18  1:10       ` Jakub Staroń
  2019-05-20  3:46         ` Pankaj Gupta
  0 siblings, 1 reply; 25+ messages in thread
From: Jakub Staroń @ 2019-05-18  1:10 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
	linux-acpi, qemu-devel, linux-ext4, linux-xfs, jack, mst,
	jasowang, david, lcapitulino, adilger kernel, smbarber, zwisler,
	aarcange, dave jiang, darrick wong, vishal l verma, david, willy,
	hch, jmoyer, nilal, lenb, kilobyte, riel, yuval shaia, stefanha,
	pbonzini, dan j williams, kwolf, tytso, xiaoguangrong eric,
	cohuck, rjw, imammedo

On 5/16/19 10:35 PM, Pankaj Gupta wrote:
> Can I take it your reviewed/acked-by? or tested-by tag? for the virtio patch :)I don't feel that I have enough expertise to give the reviewed-by tag, but you can
take my acked-by + tested-by.

Acked-by: Jakub Staron <jstaron@google.com>
Tested-by: Jakub Staron <jstaron@google.com>

No kernel panics/stalls encountered during testing this patches (v9) with QEMU + xfstests.
Some CPU stalls encountered while testing with crosvm instead of QEMU with xfstests
(test generic/464) but no repro for QEMU, so the fault may be on the side of crosvm.


The dump for the crosvm/xfstests stall:
[ 2504.175276] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
[ 2504.176681] rcu:     0-...!: (1 GPs behind) idle=9b2/1/0x4000000000000000 softirq=1089198/1089202 fqs=0 
[ 2504.178270] rcu:     2-...!: (1 ticks this GP) idle=cfe/1/0x4000000000000002 softirq=1055108/1055110 fqs=0 
[ 2504.179802] rcu:     3-...!: (1 GPs behind) idle=1d6/1/0x4000000000000002 softirq=1046798/1046802 fqs=0 
[ 2504.181215] rcu:     4-...!: (2 ticks this GP) idle=522/1/0x4000000000000002 softirq=1249063/1249064 fqs=0 
[ 2504.182625] rcu:     5-...!: (1 GPs behind) idle=6da/1/0x4000000000000000 softirq=1131036/1131047 fqs=0 
[ 2504.183955]  (detected by 3, t=0 jiffies, g=1232529, q=1370)
[ 2504.184762] Sending NMI from CPU 3 to CPUs 0:
[ 2504.186400] NMI backtrace for cpu 0
[ 2504.186401] CPU: 0 PID: 6670 Comm: 464 Not tainted 5.1.0+ #1
[ 2504.186401] Hardware name: ChromiumOS crosvm, BIOS 0 
[ 2504.186402] RIP: 0010:queued_spin_lock_slowpath+0x1c/0x1e0
[ 2504.186402] Code: e7 89 c8 f0 44 0f b1 07 39 c1 75 dc f3 c3 0f 1f 44 00 00 ba 01 00 00 00 8b 07 85 c0 75 0a f0 0f b1 17 85 c0 75 f2 f3 c3 f3 90 <eb> ec 81 fe 00 01 00 00 0f 84 ab 00 00 00 81 e6 00 ff ff ff 75 44
[ 2504.186403] RSP: 0018:ffffc90000003ee8 EFLAGS: 00000002
[ 2504.186404] RAX: 0000000000000001 RBX: 0000000000000246 RCX: 0000000000404044
[ 2504.186404] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ffffffff8244a280
[ 2504.186405] RBP: ffffffff8244a280 R08: 00000000000f4200 R09: 0000024709ed6c32
[ 2504.186405] R10: 0000000000000000 R11: 0000000000000001 R12: ffffffff8244a280
[ 2504.186405] R13: 0000000000000009 R14: 0000000000000009 R15: 0000000000000000
[ 2504.186406] FS:  0000000000000000(0000) GS:ffff8880cc600000(0000) knlGS:0000000000000000
[ 2504.186406] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2504.186406] CR2: 00007efd6b0f15d8 CR3: 000000000260a006 CR4: 0000000000360ef0
[ 2504.186407] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 2504.186407] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 2504.186407] Call Trace:
[ 2504.186408]  <IRQ>
[ 2504.186408]  _raw_spin_lock_irqsave+0x1d/0x30
[ 2504.186408]  rcu_core+0x3b6/0x740
[ 2504.186408]  ? __hrtimer_run_queues+0x133/0x280
[ 2504.186409]  ? recalibrate_cpu_khz+0x10/0x10
[ 2504.186409]  __do_softirq+0xd8/0x2e4
[ 2504.186409]  irq_exit+0xa3/0xb0
[ 2504.186410]  smp_apic_timer_interrupt+0x67/0x120
[ 2504.186410]  apic_timer_interrupt+0xf/0x20
[ 2504.186410]  </IRQ>
[ 2504.186410] RIP: 0010:unmap_page_range+0x47a/0x9b0
[ 2504.186411] Code: 0f 46 46 10 49 39 6e 18 49 89 46 10 48 89 e8 49 0f 43 46 18 41 80 4e 20 08 4d 85 c9 49 89 46 18 0f 84 68 ff ff ff 49 8b 51 08 <48> 8d 42 ff 83 e2 01 49 0f 44 c1 f6 40 18 01 75 38 48 ba ff 0f 00
[ 2504.186411] RSP: 0018:ffffc900036cbcc8 EFLAGS: 00000282 ORIG_RAX: ffffffffffffff13
[ 2504.186412] RAX: ffffffffffffffff RBX: 800000003751d045 RCX: 0000000000000001
[ 2504.186413] RDX: ffffea0002e09288 RSI: 000000000269b000 RDI: ffff8880b6525e40
[ 2504.186413] RBP: 000000000269c000 R08: 0000000000000000 R09: ffffea0000dd4740
[ 2504.186413] R10: ffffea0001755700 R11: ffff8880cc62d120 R12: 0000000002794000
[ 2504.186414] R13: 000000000269b000 R14: ffffc900036cbdf0 R15: ffff8880572434d8
[ 2504.186414]  ? unmap_page_range+0x420/0x9b0
[ 2504.186414]  ? release_pages+0x175/0x390
[ 2504.186414]  unmap_vmas+0x7c/0xe0
[ 2504.186415]  exit_mmap+0xa4/0x190
[ 2504.186415]  mmput+0x3b/0x100
[ 2504.186415]  do_exit+0x276/0xc10
[ 2504.186415]  do_group_exit+0x35/0xa0
[ 2504.186415]  __x64_sys_exit_group+0xf/0x10
[ 2504.186416]  do_syscall_64+0x43/0x120
[ 2504.186416]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 2504.186416] RIP: 0033:0x7efd6ae10618
[ 2504.186416] Code: Bad RIP value.
[ 2504.186417] RSP: 002b:00007ffcac9bde38 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
[ 2504.186417] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007efd6ae10618
[ 2504.186418] RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
[ 2504.186418] RBP: 00007efd6b0ed8e0 R08: 00000000000000e7 R09: ffffffffffffff98
[ 2504.186418] R10: 00007ffcac9bddb8 R11: 0000000000000246 R12: 00007efd6b0ed8e0
[ 2504.186419] R13: 00007efd6b0f2c20 R14: 0000000000000060 R15: 000000000070e705
[ 2504.186421] NMI backtrace for cpu 3
[ 2504.226980] CPU: 3 PID: 6596 Comm: xfs_io Not tainted 5.1.0+ #1
[ 2504.227661] Hardware name: ChromiumOS crosvm, BIOS 0 
[ 2504.228261] Call Trace:
[ 2504.228552]  <IRQ>
[ 2504.228795]  dump_stack+0x46/0x5b
[ 2504.229180]  nmi_cpu_backtrace+0x89/0x90
[ 2504.229649]  ? lapic_can_unplug_cpu+0x90/0x90
[ 2504.230157]  nmi_trigger_cpumask_backtrace+0x82/0xc0
[ 2504.230751]  rcu_dump_cpu_stacks+0x8b/0xb7
[ 2504.231222]  rcu_sched_clock_irq+0x6f6/0x720
[ 2504.231726]  ? tick_sched_do_timer+0x50/0x50
[ 2504.232214]  update_process_times+0x23/0x50
[ 2504.232693]  tick_sched_handle+0x2f/0x40
[ 2504.233144]  tick_sched_timer+0x32/0x70
[ 2504.233594]  __hrtimer_run_queues+0x103/0x280
[ 2504.234092]  hrtimer_interrupt+0xe0/0x240
[ 2504.234580]  smp_apic_timer_interrupt+0x5d/0x120
[ 2504.235152]  apic_timer_interrupt+0xf/0x20
[ 2504.235627]  </IRQ>
[ 2504.235879] RIP: 0010:__memcpy_flushcache+0x4b/0x180
[ 2504.236452] Code: 8d 5d e0 4c 8d 62 20 48 89 f7 48 29 d7 48 89 d9 48 83 e1 e0 4c 01 e1 48 8d 04 17 4c 8b 02 4c 8b 4a 08 4c 8b 52 10 4c 8b 5a 18 <4c> 0f c3 00 4c 0f c3 48 08 4c 0f c3 50 10 4c 0f c3 58 18 48 83 c2
[ 2504.238592] RSP: 0018:ffffc90003ae38e8 EFLAGS: 00010286 ORIG_RAX: ffffffffffffff13
[ 2504.239467] RAX: ffff888341800000 RBX: 0000000000000fe0 RCX: ffff88801bd22000
[ 2504.240277] RDX: ffff88801bd21000 RSI: ffff888341800000 RDI: 0000000325adf000
[ 2504.241092] RBP: 0000000000001000 R08: cdcdcdcdcdcdcdcd R09: cdcdcdcdcdcdcdcd
[ 2504.241908] R10: cdcdcdcdcdcdcdcd R11: cdcdcdcdcdcdcdcd R12: ffff88801bd21020
[ 2504.242751] R13: ffff8880b916b600 R14: ffff888341800000 R15: ffffea00006f4840
[ 2504.243602]  write_pmem+0x61/0x90
[ 2504.244002]  pmem_do_bvec+0x178/0x2c0
[ 2504.244469]  ? chksum_update+0xe/0x20
[ 2504.244908]  pmem_make_request+0xf7/0x270
[ 2504.245509]  generic_make_request+0x199/0x3f0
[ 2504.246179]  ? submit_bio+0x67/0x130
[ 2504.246710]  submit_bio+0x67/0x130
[ 2504.247117]  ext4_io_submit+0x44/0x50
[ 2504.247556]  ext4_writepages+0x621/0xe80
[ 2504.248028]  ? 0xffffffff81000000
[ 2504.248418]  ? do_writepages+0x46/0xd0
[ 2504.248880]  ? ext4_mark_inode_dirty+0x1d0/0x1d0
[ 2504.249417]  do_writepages+0x46/0xd0
[ 2504.249833]  ? release_pages+0x175/0x390
[ 2504.250290]  ? __filemap_fdatawrite_range+0x7c/0xb0
[ 2504.250879]  __filemap_fdatawrite_range+0x7c/0xb0
[ 2504.251427]  ext4_release_file+0x67/0xa0
[ 2504.251897]  __fput+0xb1/0x220
[ 2504.252260]  task_work_run+0x79/0xa0
[ 2504.252676]  do_exit+0x2ca/0xc10
[ 2504.253063]  ? __switch_to_asm+0x40/0x70
[ 2504.253530]  ? __switch_to_asm+0x34/0x70
[ 2504.253995]  ? __switch_to_asm+0x40/0x70
[ 2504.254446]  do_group_exit+0x35/0xa0
[ 2504.254865]  get_signal+0x14e/0x7a0
[ 2504.255281]  ? __switch_to_asm+0x34/0x70
[ 2504.255749]  ? __switch_to_asm+0x40/0x70
[ 2504.256224]  do_signal+0x2b/0x5e0
[ 2504.256619]  ? __switch_to_asm+0x40/0x70
[ 2504.257086]  ? __switch_to_asm+0x34/0x70
[ 2504.257552]  ? __switch_to_asm+0x40/0x70
[ 2504.258022]  ? __switch_to_asm+0x34/0x70
[ 2504.258488]  ? __schedule+0x253/0x530
[ 2504.258943]  ? __switch_to_asm+0x34/0x70
[ 2504.259398]  exit_to_usermode_loop+0x87/0xa0
[ 2504.259900]  do_syscall_64+0xf7/0x120
[ 2504.260326]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 2504.260923] RIP: 0033:0x7faf347e28bd
[ 2504.261348] Code: Bad RIP value.
[ 2504.261727] RSP: 002b:00007faf33fc5f40 EFLAGS: 00000293 ORIG_RAX: 0000000000000022
[ 2504.262594] RAX: fffffffffffffdfe RBX: 0000000000000000 RCX: 00007faf347e28bd
[ 2504.263416] RDX: 8b9da1f4246cdb38 RSI: 0000000000000000 RDI: 0000000000000000
[ 2504.264215] RBP: 0000000000000000 R08: 00007faf33fc6700 R09: 00007faf33fc6700
[ 2504.265061] R10: 000000000000012d R11: 0000000000000293 R12: 00007ffdf142327e
[ 2504.266082] R13: 00007ffdf142327f R14: 00007faf337c6000 R15: 0000000000000003

Arch: x86_64
Kernel: stable top with virtio-pmem v9 patches applied
Distro: Debian Stretch

But as I said, it may be just a problem with crosvm.


Thank you,
Jakub Staron

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-18  1:10       ` Jakub Staroń
@ 2019-05-20  3:46         ` Pankaj Gupta
  2019-05-20  3:47           ` Pankaj Gupta
  0 siblings, 1 reply; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-20  3:46 UTC (permalink / raw)
  To: Jakub Staroń
  Cc: cohuck, jack, kvm, mst, jasowang, david, qemu-devel,
	virtualization, adilger kernel, smbarber, zwisler, aarcange,
	dave jiang, linux-nvdimm, vishal l verma, david, willy, hch,
	linux-acpi, jmoyer, linux-ext4, lenb, kilobyte, riel,
	yuval shaia, stefanha, imammedo, dan j williams, lcapitulino,
	kwolf, nilal, tytso, xiaoguangrong eric, darrick wong, rjw,
	linux-kernel, linux-xfs, linux-fsdevel, pbonzini


> On 5/16/19 10:35 PM, Pankaj Gupta wrote:
> > Can I take it your reviewed/acked-by? or tested-by tag? for the virtio
> > patch :)I don't feel that I have enough expertise to give the reviewed-by
> > tag, but you can
> take my acked-by + tested-by.
> 
> Acked-by: Jakub Staron <jstaron@google.com>
> Tested-by: Jakub Staron <jstaron@google.com>
> 
> No kernel panics/stalls encountered during testing this patches (v9) with
> QEMU + xfstests.

Thank you for testing and confirming the results. I will add your tested &
acked-by in v10.

> Some CPU stalls encountered while testing with crosvm instead of QEMU with
> xfstests
> (test generic/464) but no repro for QEMU, so the fault may be on the side of
> crosvm.

yes, looks like crosvm related as we did not see any of this in my and your
testing with Qemu. 

> 
> 
> The dump for the crosvm/xfstests stall:
> [ 2504.175276] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
> [ 2504.176681] rcu:     0-...!: (1 GPs behind) idle=9b2/1/0x4000000000000000
> softirq=1089198/1089202 fqs=0
> [ 2504.178270] rcu:     2-...!: (1 ticks this GP)
> idle=cfe/1/0x4000000000000002 softirq=1055108/1055110 fqs=0
> [ 2504.179802] rcu:     3-...!: (1 GPs behind) idle=1d6/1/0x4000000000000002
> softirq=1046798/1046802 fqs=0
> [ 2504.181215] rcu:     4-...!: (2 ticks this GP)
> idle=522/1/0x4000000000000002 softirq=1249063/1249064 fqs=0
> [ 2504.182625] rcu:     5-...!: (1 GPs behind) idle=6da/1/0x4000000000000000
> softirq=1131036/1131047 fqs=0
> [ 2504.183955]  (detected by 3, t=0 jiffies, g=1232529, q=1370)
> [ 2504.184762] Sending NMI from CPU 3 to CPUs 0:
> [ 2504.186400] NMI backtrace for cpu 0
> [ 2504.186401] CPU: 0 PID: 6670 Comm: 464 Not tainted 5.1.0+ #1
> [ 2504.186401] Hardware name: ChromiumOS crosvm, BIOS 0
> [ 2504.186402] RIP: 0010:queued_spin_lock_slowpath+0x1c/0x1e0
> [ 2504.186402] Code: e7 89 c8 f0 44 0f b1 07 39 c1 75 dc f3 c3 0f 1f 44 00 00
> ba 01 00 00 00 8b 07 85 c0 75 0a f0 0f b1 17 85 c0 75 f2 f3 c3 f3 90 <eb> ec
> 81 fe 00 01 00 00 0f 84 ab 00 00 00 81 e6 00 ff ff ff 75 44
> [ 2504.186403] RSP: 0018:ffffc90000003ee8 EFLAGS: 00000002
> [ 2504.186404] RAX: 0000000000000001 RBX: 0000000000000246 RCX:
> 0000000000404044
> [ 2504.186404] RDX: 0000000000000001 RSI: 0000000000000001 RDI:
> ffffffff8244a280
> [ 2504.186405] RBP: ffffffff8244a280 R08: 00000000000f4200 R09:
> 0000024709ed6c32
> [ 2504.186405] R10: 0000000000000000 R11: 0000000000000001 R12:
> ffffffff8244a280
> [ 2504.186405] R13: 0000000000000009 R14: 0000000000000009 R15:
> 0000000000000000
> [ 2504.186406] FS:  0000000000000000(0000) GS:ffff8880cc600000(0000)
> knlGS:0000000000000000
> [ 2504.186406] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 2504.186406] CR2: 00007efd6b0f15d8 CR3: 000000000260a006 CR4:
> 0000000000360ef0
> [ 2504.186407] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [ 2504.186407] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [ 2504.186407] Call Trace:
> [ 2504.186408]  <IRQ>
> [ 2504.186408]  _raw_spin_lock_irqsave+0x1d/0x30
> [ 2504.186408]  rcu_core+0x3b6/0x740
> [ 2504.186408]  ? __hrtimer_run_queues+0x133/0x280
> [ 2504.186409]  ? recalibrate_cpu_khz+0x10/0x10
> [ 2504.186409]  __do_softirq+0xd8/0x2e4
> [ 2504.186409]  irq_exit+0xa3/0xb0
> [ 2504.186410]  smp_apic_timer_interrupt+0x67/0x120
> [ 2504.186410]  apic_timer_interrupt+0xf/0x20
> [ 2504.186410]  </IRQ>
> [ 2504.186410] RIP: 0010:unmap_page_range+0x47a/0x9b0
> [ 2504.186411] Code: 0f 46 46 10 49 39 6e 18 49 89 46 10 48 89 e8 49 0f 43 46
> 18 41 80 4e 20 08 4d 85 c9 49 89 46 18 0f 84 68 ff ff ff 49 8b 51 08 <48> 8d
> 42 ff 83 e2 01 49 0f 44 c1 f6 40 18 01 75 38 48 ba ff 0f 00
> [ 2504.186411] RSP: 0018:ffffc900036cbcc8 EFLAGS: 00000282 ORIG_RAX:
> ffffffffffffff13
> [ 2504.186412] RAX: ffffffffffffffff RBX: 800000003751d045 RCX:
> 0000000000000001
> [ 2504.186413] RDX: ffffea0002e09288 RSI: 000000000269b000 RDI:
> ffff8880b6525e40
> [ 2504.186413] RBP: 000000000269c000 R08: 0000000000000000 R09:
> ffffea0000dd4740
> [ 2504.186413] R10: ffffea0001755700 R11: ffff8880cc62d120 R12:
> 0000000002794000
> [ 2504.186414] R13: 000000000269b000 R14: ffffc900036cbdf0 R15:
> ffff8880572434d8
> [ 2504.186414]  ? unmap_page_range+0x420/0x9b0
> [ 2504.186414]  ? release_pages+0x175/0x390
> [ 2504.186414]  unmap_vmas+0x7c/0xe0
> [ 2504.186415]  exit_mmap+0xa4/0x190
> [ 2504.186415]  mmput+0x3b/0x100
> [ 2504.186415]  do_exit+0x276/0xc10
> [ 2504.186415]  do_group_exit+0x35/0xa0
> [ 2504.186415]  __x64_sys_exit_group+0xf/0x10
> [ 2504.186416]  do_syscall_64+0x43/0x120
> [ 2504.186416]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 2504.186416] RIP: 0033:0x7efd6ae10618
> [ 2504.186416] Code: Bad RIP value.
> [ 2504.186417] RSP: 002b:00007ffcac9bde38 EFLAGS: 00000246 ORIG_RAX:
> 00000000000000e7
> [ 2504.186417] RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
> 00007efd6ae10618
> [ 2504.186418] RDX: 0000000000000000 RSI: 000000000000003c RDI:
> 0000000000000000
> [ 2504.186418] RBP: 00007efd6b0ed8e0 R08: 00000000000000e7 R09:
> ffffffffffffff98
> [ 2504.186418] R10: 00007ffcac9bddb8 R11: 0000000000000246 R12:
> 00007efd6b0ed8e0
> [ 2504.186419] R13: 00007efd6b0f2c20 R14: 0000000000000060 R15:
> 000000000070e705
> [ 2504.186421] NMI backtrace for cpu 3
> [ 2504.226980] CPU: 3 PID: 6596 Comm: xfs_io Not tainted 5.1.0+ #1
> [ 2504.227661] Hardware name: ChromiumOS crosvm, BIOS 0
> [ 2504.228261] Call Trace:
> [ 2504.228552]  <IRQ>
> [ 2504.228795]  dump_stack+0x46/0x5b
> [ 2504.229180]  nmi_cpu_backtrace+0x89/0x90
> [ 2504.229649]  ? lapic_can_unplug_cpu+0x90/0x90
> [ 2504.230157]  nmi_trigger_cpumask_backtrace+0x82/0xc0
> [ 2504.230751]  rcu_dump_cpu_stacks+0x8b/0xb7
> [ 2504.231222]  rcu_sched_clock_irq+0x6f6/0x720
> [ 2504.231726]  ? tick_sched_do_timer+0x50/0x50
> [ 2504.232214]  update_process_times+0x23/0x50
> [ 2504.232693]  tick_sched_handle+0x2f/0x40
> [ 2504.233144]  tick_sched_timer+0x32/0x70
> [ 2504.233594]  __hrtimer_run_queues+0x103/0x280
> [ 2504.234092]  hrtimer_interrupt+0xe0/0x240
> [ 2504.234580]  smp_apic_timer_interrupt+0x5d/0x120
> [ 2504.235152]  apic_timer_interrupt+0xf/0x20
> [ 2504.235627]  </IRQ>
> [ 2504.235879] RIP: 0010:__memcpy_flushcache+0x4b/0x180
> [ 2504.236452] Code: 8d 5d e0 4c 8d 62 20 48 89 f7 48 29 d7 48 89 d9 48 83 e1
> e0 4c 01 e1 48 8d 04 17 4c 8b 02 4c 8b 4a 08 4c 8b 52 10 4c 8b 5a 18 <4c> 0f
> c3 00 4c 0f c3 48 08 4c 0f c3 50 10 4c 0f c3 58 18 48 83 c2
> [ 2504.238592] RSP: 0018:ffffc90003ae38e8 EFLAGS: 00010286 ORIG_RAX:
> ffffffffffffff13
> [ 2504.239467] RAX: ffff888341800000 RBX: 0000000000000fe0 RCX:
> ffff88801bd22000
> [ 2504.240277] RDX: ffff88801bd21000 RSI: ffff888341800000 RDI:
> 0000000325adf000
> [ 2504.241092] RBP: 0000000000001000 R08: cdcdcdcdcdcdcdcd R09:
> cdcdcdcdcdcdcdcd
> [ 2504.241908] R10: cdcdcdcdcdcdcdcd R11: cdcdcdcdcdcdcdcd R12:
> ffff88801bd21020
> [ 2504.242751] R13: ffff8880b916b600 R14: ffff888341800000 R15:
> ffffea00006f4840
> [ 2504.243602]  write_pmem+0x61/0x90
> [ 2504.244002]  pmem_do_bvec+0x178/0x2c0
> [ 2504.244469]  ? chksum_update+0xe/0x20
> [ 2504.244908]  pmem_make_request+0xf7/0x270
> [ 2504.245509]  generic_make_request+0x199/0x3f0
> [ 2504.246179]  ? submit_bio+0x67/0x130
> [ 2504.246710]  submit_bio+0x67/0x130
> [ 2504.247117]  ext4_io_submit+0x44/0x50
> [ 2504.247556]  ext4_writepages+0x621/0xe80
> [ 2504.248028]  ? 0xffffffff81000000
> [ 2504.248418]  ? do_writepages+0x46/0xd0
> [ 2504.248880]  ? ext4_mark_inode_dirty+0x1d0/0x1d0
> [ 2504.249417]  do_writepages+0x46/0xd0
> [ 2504.249833]  ? release_pages+0x175/0x390
> [ 2504.250290]  ? __filemap_fdatawrite_range+0x7c/0xb0
> [ 2504.250879]  __filemap_fdatawrite_range+0x7c/0xb0
> [ 2504.251427]  ext4_release_file+0x67/0xa0
> [ 2504.251897]  __fput+0xb1/0x220
> [ 2504.252260]  task_work_run+0x79/0xa0
> [ 2504.252676]  do_exit+0x2ca/0xc10
> [ 2504.253063]  ? __switch_to_asm+0x40/0x70
> [ 2504.253530]  ? __switch_to_asm+0x34/0x70
> [ 2504.253995]  ? __switch_to_asm+0x40/0x70
> [ 2504.254446]  do_group_exit+0x35/0xa0
> [ 2504.254865]  get_signal+0x14e/0x7a0
> [ 2504.255281]  ? __switch_to_asm+0x34/0x70
> [ 2504.255749]  ? __switch_to_asm+0x40/0x70
> [ 2504.256224]  do_signal+0x2b/0x5e0
> [ 2504.256619]  ? __switch_to_asm+0x40/0x70
> [ 2504.257086]  ? __switch_to_asm+0x34/0x70
> [ 2504.257552]  ? __switch_to_asm+0x40/0x70
> [ 2504.258022]  ? __switch_to_asm+0x34/0x70
> [ 2504.258488]  ? __schedule+0x253/0x530
> [ 2504.258943]  ? __switch_to_asm+0x34/0x70
> [ 2504.259398]  exit_to_usermode_loop+0x87/0xa0
> [ 2504.259900]  do_syscall_64+0xf7/0x120
> [ 2504.260326]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 2504.260923] RIP: 0033:0x7faf347e28bd
> [ 2504.261348] Code: Bad RIP value.
> [ 2504.261727] RSP: 002b:00007faf33fc5f40 EFLAGS: 00000293 ORIG_RAX:
> 0000000000000022
> [ 2504.262594] RAX: fffffffffffffdfe RBX: 0000000000000000 RCX:
> 00007faf347e28bd
> [ 2504.263416] RDX: 8b9da1f4246cdb38 RSI: 0000000000000000 RDI:
> 0000000000000000
> [ 2504.264215] RBP: 0000000000000000 R08: 00007faf33fc6700 R09:
> 00007faf33fc6700
> [ 2504.265061] R10: 000000000000012d R11: 0000000000000293 R12:
> 00007ffdf142327e
> [ 2504.266082] R13: 00007ffdf142327f R14: 00007faf337c6000 R15:
> 0000000000000003
> 
> Arch: x86_64
> Kernel: stable top with virtio-pmem v9 patches applied
> Distro: Debian Stretch
> 
> But as I said, it may be just a problem with crosvm.
Right.

Thanks,
Pankaj
> 
> 
> Thank you,
> Jakub Staron
> 
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver
  2019-05-20  3:46         ` Pankaj Gupta
@ 2019-05-20  3:47           ` Pankaj Gupta
  0 siblings, 0 replies; 25+ messages in thread
From: Pankaj Gupta @ 2019-05-20  3:47 UTC (permalink / raw)
  To: Jakub Staroń
  Cc: cohuck, jack, kvm, mst, jasowang, david, qemu-devel,
	virtualization, adilger kernel, smbarber, zwisler, aarcange,
	dave jiang, linux-nvdimm, vishal l verma, david, willy, hch,
	linux-acpi, jmoyer, linux-ext4, lenb, kilobyte, riel,
	yuval shaia, stefanha, imammedo, dan j williams, lcapitulino,
	kwolf, nilal, tytso, xiaoguangrong eric, darrick wong, rjw,
	linux-kernel, linux-xfs, linux-fsdevel, pbonzini


> 
> 
> > On 5/16/19 10:35 PM, Pankaj Gupta wrote:
> > > Can I take it your reviewed/acked-by? or tested-by tag? for the virtio
> > > patch :)I don't feel that I have enough expertise to give the reviewed-by
> > > tag, but you can
> > take my acked-by + tested-by.
> > 
> > Acked-by: Jakub Staron <jstaron@google.com>
> > Tested-by: Jakub Staron <jstaron@google.com>
> > 
> > No kernel panics/stalls encountered during testing this patches (v9) with
> > QEMU + xfstests.
> 
> Thank you for testing and confirming the results. I will add your tested &
> acked-by in v10.
> 
> > Some CPU stalls encountered while testing with crosvm instead of QEMU with
> > xfstests
> > (test generic/464) but no repro for QEMU, so the fault may be on the side
> > of
> > crosvm.
> 
> yes, looks like crosvm related as we did not see any of this in my and your
> testing with Qemu.

Also, they don't seem to be related with virtio-pmem.

Thanks,
Pankaj

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, back to index

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14 14:54 [PATCH v9 0/7] virtio pmem driver Pankaj Gupta
2019-05-14 14:54 ` [PATCH v9 1/7] libnvdimm: nd_region flush callback support Pankaj Gupta
2019-05-15 21:07   ` Dan Williams
2019-05-16  6:28     ` Pankaj Gupta
2019-05-14 14:54 ` [PATCH v9 2/7] virtio-pmem: Add virtio pmem driver Pankaj Gupta
2019-05-14 15:09   ` Randy Dunlap
2019-05-14 15:25     ` [Qemu-devel] " Pankaj Gupta
2019-05-15 20:46       ` Dan Williams
2019-05-15 20:46   ` David Hildenbrand
2019-05-15 20:52     ` David Hildenbrand
2019-05-16  7:54       ` Pankaj Gupta
2019-05-16  7:49     ` Pankaj Gupta
2019-05-16 13:59     ` Michael S. Tsirkin
2019-05-17  5:31       ` [Qemu-devel] " Pankaj Gupta
2019-05-17  0:12   ` Jakub Staroń
2019-05-17  5:35     ` [Qemu-devel] " Pankaj Gupta
2019-05-18  1:10       ` Jakub Staroń
2019-05-20  3:46         ` Pankaj Gupta
2019-05-20  3:47           ` Pankaj Gupta
2019-05-14 14:54 ` [PATCH v9 3/7] libnvdimm: add dax_dev sync flag Pankaj Gupta
2019-05-14 14:54 ` [PATCH v9 4/7] dm: enable synchronous dax Pankaj Gupta
2019-05-15 20:48   ` Dan Williams
2019-05-14 14:54 ` [PATCH v9 5/7] dax: check synchronous mapping is supported Pankaj Gupta
2019-05-14 14:54 ` [PATCH v9 6/7] ext4: disable map_sync for async flush Pankaj Gupta
2019-05-14 14:54 ` [PATCH v9 7/7] xfs: " Pankaj Gupta

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox