Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp43259imm; Fri, 21 Sep 2018 17:47:05 -0700 (PDT) X-Google-Smtp-Source: ACcGV63tprqk067IqZuWG9qsSXrWXBknAQehIdiLBRxnMDRMORDTud1Js60C6sfmCUChtpCYA1qR X-Received: by 2002:a17:902:4a0c:: with SMTP id w12-v6mr147966pld.289.1537577225177; Fri, 21 Sep 2018 17:47:05 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1537577225; cv=none; d=google.com; s=arc-20160816; b=zKav7obECQ7XeER23s6fkJxFDamMjK27ApObVu4/s0NAq5VLgWF8Dyz48GoPSppaBM sIJ94Ni7oMfeUDFATuOF7ErcMly5ORWSSi/UBLLPdbHbTRHku3xZU3nJSkXsXxVAnIpL w3g3gHw+4B27motgXej8CUIcL0m0Z+oblYREU6ElaZbWHzYgvgDGgh6cPL4lT0Cc5rc/ i+pEBEbbQ1KRyvc1KVrHZsvzBJubj0PA3M7WjUTp/mOnF9tb63paDtsJ0TLaoljP3l4/ tHfHCIhNGyLidf8hD07I0vqt6S2ztE1hjt2zu3Nu2exXIX2I+9MxONX0cJ6nnYRx6xdQ h8ZQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=TyrevxK5pV+sYSGt19Tnl/B2FPBdTF8yPD+StB8mmtw=; b=SEbcqASNCWOqZMa+oW2Ifc8A+lpIpFkcGHitCaeVKGdEmA3GuszPRrmFkvpmIOpEJB v7ldzUx+tKRtJIu5lHlr/46nrgJjgWG0r+9TSiEy8TBB+NJPw9Kuw2Tceev9havvxCOk alIZpT/vAip+Rxhm+cseKn3cf6ARdD/1J+X47fouwjK0ILHrJL1+GN0r4DQYWWVcpxIf UZR2Q98M3HnM+UKSyOivAfWe+idgjUqfYu4KnD9rTTxCotH5eLQ65POODcCm1JJy+W21 p8x7qGHdjrCXDrLHBrG/1Lv59vpVSKTq+MmW3UulEY/B96AsTmKL/6NENYs03LkOjmX2 5Mng== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel-com.20150623.gappssmtp.com header.s=20150623 header.b=S1o6XQLP; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 8-v6si27408876pgu.519.2018.09.21.17.46.49; Fri, 21 Sep 2018 17:47:05 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@intel-com.20150623.gappssmtp.com header.s=20150623 header.b=S1o6XQLP; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2391743AbeIVGfd (ORCPT + 99 others); Sat, 22 Sep 2018 02:35:33 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:40709 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2391614AbeIVGfd (ORCPT ); Sat, 22 Sep 2018 02:35:33 -0400 Received: by mail-oi0-f67.google.com with SMTP id l202-v6so12865260oig.7 for ; Fri, 21 Sep 2018 17:44:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TyrevxK5pV+sYSGt19Tnl/B2FPBdTF8yPD+StB8mmtw=; b=S1o6XQLPB4L9XwGu3G0CWfqUbK8TpF7ozUahTqh5urUeFNc2H3ZRLge3k5tw/OaHKE dond3/KMSjWQSYF1AYHW6DyMJPGYW/E9S+LQ4nhnKRC5j0STYJo0DWOyrFeRX3XjDYxG 79ULny5l33BAJQASv5m5eIebspU9eTWqeSGzdXgGXdIkhFyzpXSBhmWx9HuUbUtZwW6o Hr703XxIjWAVOLTnUMYTuncRhdXEHCbfeqOAfRGBWQSqzWiQppuT1VXmTgTfKanp/89a d2Wg+GBNWgbGjI/r2Qmngmh0qr/Zu7SdjLeITDfCIAtxfTXMmPoCJksuAG2BTEBU2e6R PTlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TyrevxK5pV+sYSGt19Tnl/B2FPBdTF8yPD+StB8mmtw=; b=KyB27/8+tIpHJzXNOYk/XUPiHjBVXSEPA3wLvaYL3rkWMNxnLHiVitOV6vVHRSZyuU SVhlgF2zE10TBKk4lySND/PzbLlei7SscOaKhRYis48sx6Q8dtwa7g84IOiCh6PJhuo1 F8mR+gZ1IdPKzk1tMaFaPRMlNv6xDjLvf1SOBmC1+YWCER9pcXHdw38P2b6tDg0xYmp1 Ql9dU6UoejNshgTWZ/kgyheFdcfzPlDZ7SZ7PHsIiEDn4iEznuHlwnAn9BS3zlvactdU CndPRg9KQnjjYzHw4Gu9BbEOj4sN2cGBrp2a2cdKA3ZfbDvRXsKW/S+AoOgvaTXcSIAG hDxQ== X-Gm-Message-State: APzg51DElmgcrQzSjcfksVmMslllitd7cmfaxo+ccqdsQaNdrWXgF3N4 ZortFckOp399TtfkABFIJZQvUoFCsQZiptmwD74QbQ== X-Received: by 2002:aca:60c1:: with SMTP id u184-v6mr31937oib.99.1537577050674; Fri, 21 Sep 2018 17:44:10 -0700 (PDT) MIME-Version: 1.0 References: <20180831133019.27579-1-pagupta@redhat.com> <20180831133019.27579-3-pagupta@redhat.com> In-Reply-To: <20180831133019.27579-3-pagupta@redhat.com> From: Dan Williams Date: Fri, 21 Sep 2018 17:43:58 -0700 Message-ID: Subject: Re: [PATCH 2/3] libnvdimm: nd_region flush callback support To: Pankaj Gupta Cc: Linux Kernel Mailing List , KVM list , Qemu Developers , linux-nvdimm , Jan Kara , Stefan Hajnoczi , Rik van Riel , Nitesh Narayan Lal , Kevin Wolf , Paolo Bonzini , "Zwisler, Ross" , David Hildenbrand , Xiao Guangrong , Christoph Hellwig , "Michael S. Tsirkin" , niteshnarayanlal@hotmail.com, lcapitulino@redhat.com, Igor Mammedov , Eric Blake Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Aug 31, 2018 at 6:32 AM Pankaj Gupta 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 This looks ok to me, just some nits below. > --- > drivers/acpi/nfit/core.c | 7 +++++-- > drivers/nvdimm/claim.c | 3 ++- > drivers/nvdimm/pmem.c | 12 ++++++++---- > drivers/nvdimm/region_devs.c | 12 ++++++++++-- > include/linux/libnvdimm.h | 4 +++- > 5 files changed, 28 insertions(+), 10 deletions(-) > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index b072cfc..cd63b69 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -2216,6 +2216,7 @@ static void write_blk_ctl(struct nfit_blk *nfit_blk, unsigned int bw, > { > u64 cmd, offset; > struct nfit_blk_mmio *mmio = &nfit_blk->mmio[DCR]; > + struct nd_region *nd_region = nfit_blk->nd_region; > > enum { > BCW_OFFSET_MASK = (1ULL << 48)-1, > @@ -2234,7 +2235,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); > + nd_region->flush(nd_region); I would keep the indirect function call override inside of nvdimm_flush. Then this hunk can go away... > > if (nfit_blk->dimm_flags & NFIT_BLK_DCR_LATCH) > readq(mmio->addr.base + offset); > @@ -2245,6 +2246,7 @@ static int acpi_nfit_blk_single_io(struct nfit_blk *nfit_blk, > unsigned int lane) > { > struct nfit_blk_mmio *mmio = &nfit_blk->mmio[BDW]; > + struct nd_region *nd_region = nfit_blk->nd_region; > unsigned int copied = 0; > u64 base_offset; > int rc; > @@ -2283,7 +2285,8 @@ static int acpi_nfit_blk_single_io(struct nfit_blk *nfit_blk, > } > > if (rw) > - nvdimm_flush(nfit_blk->nd_region); > + nd_region->flush(nd_region); > + > ...ditto, no need to touch this code. > rc = read_blk_stat(nfit_blk, lane) ? -EIO : 0; > return rc; > diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c > index fb667bf..49dce9c 100644 > --- a/drivers/nvdimm/claim.c > +++ b/drivers/nvdimm/claim.c > @@ -262,6 +262,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); > + struct nd_region *nd_region = to_nd_region(ndns->dev.parent); > sector_t sector = offset >> 9; > int rc = 0; > > @@ -301,7 +302,7 @@ 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)); > + nd_region->flush(nd_region); For this you would need to teach nsio_rw_bytes() that the flush can fail. > > return rc; > } > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > index 6071e29..ba57cfa 100644 > --- a/drivers/nvdimm/pmem.c > +++ b/drivers/nvdimm/pmem.c > @@ -201,7 +201,8 @@ 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); > + bio->bi_status = nd_region->flush(nd_region); > + Let's have nvdimm_flush() return 0 or -EIO if it fails since thats what nsio_rw_bytes() expects, and you'll need to translate that to: BLK_STS_IOERR > > do_acct = nd_iostat_start(bio, &start); > bio_for_each_segment(bvec, bio, iter) { > @@ -216,7 +217,7 @@ 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); > + bio->bi_status = nd_region->flush(nd_region); Same comment. > > bio_endio(bio); > return BLK_QC_T_NONE; > @@ -517,6 +518,7 @@ static int nd_pmem_probe(struct device *dev) > static int nd_pmem_remove(struct device *dev) > { > struct pmem_device *pmem = dev_get_drvdata(dev); > + struct nd_region *nd_region = to_region(pmem); > > if (is_nd_btt(dev)) > nvdimm_namespace_detach_btt(to_nd_btt(dev)); > @@ -528,14 +530,16 @@ static int nd_pmem_remove(struct device *dev) > sysfs_put(pmem->bb_state); > pmem->bb_state = NULL; > } > - nvdimm_flush(to_nd_region(dev->parent)); > + nd_region->flush(nd_region); Not needed if the indirect function call moves inside nvdimm_flush(). > > return 0; > } > > static void nd_pmem_shutdown(struct device *dev) > { > - nvdimm_flush(to_nd_region(dev->parent)); > + struct nd_region *nd_region = to_nd_region(dev->parent); > + > + nd_region->flush(nd_region); > } > > 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 fa37afc..a170a6b 100644 > --- a/drivers/nvdimm/region_devs.c > +++ b/drivers/nvdimm/region_devs.c > @@ -290,7 +290,7 @@ static ssize_t deep_flush_store(struct device *dev, struct device_attribute *att > return rc; > if (!flush) > return -EINVAL; > - nvdimm_flush(nd_region); > + nd_region->flush(nd_region); Let's pass the error code through if the flush fails. > > return len; > } > @@ -1065,6 +1065,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 = nvdimm_flush; > + We'll need to rename the existing nvdimm_flush() to generic_nvdimm_flush(). > nd_device_register(dev); > > return nd_region; > @@ -1109,7 +1114,7 @@ EXPORT_SYMBOL_GPL(nvdimm_volatile_region_create); > * 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 nvdimm_flush(struct nd_region *nd_region) > { > struct nd_region_data *ndrd = dev_get_drvdata(&nd_region->dev); > int i, idx; > @@ -1133,7 +1138,10 @@ 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; > } > + Needless newline. > EXPORT_SYMBOL_GPL(nvdimm_flush); > > /** > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h > index 097072c..3af7177 100644 > --- a/include/linux/libnvdimm.h > +++ b/include/linux/libnvdimm.h > @@ -115,6 +115,7 @@ struct nd_mapping_desc { > int position; > }; > > +struct nd_region; > struct nd_region_desc { > struct resource *res; > struct nd_mapping_desc *mapping; > @@ -126,6 +127,7 @@ struct nd_region_desc { > int numa_node; > unsigned long flags; > struct device_node *of_node; > + int (*flush)(struct nd_region *nd_region); > }; > > struct device; > @@ -201,7 +203,7 @@ 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); > int nvdimm_has_flush(struct nd_region *nd_region); > int nvdimm_has_cache(struct nd_region *nd_region); > > -- > 2.9.3 >