| Message ID | 20190410040826.24371-2-pagupta@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio pmem driver | expand |
On Tue, Apr 9, 2019 at 9:09 PM 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. > > This also handles asynchronous flush requests from the block layer > by creating a child bio and chaining it with parent bio. > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > --- [..] > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > index b4ef7d9ff22e..fb1041ab32a6 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, false); > + 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 = generic_nvdimm_flush; > + > nd_device_register(dev); > > return nd_region; > @@ -1125,11 +1132,36 @@ 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, bool async) > +{ I don't quite see the point of the 'async' argument. All the usages of this routine are either nvdimm_flush(nd_region, bio, true) ...or: nvdimm_flush(nd_region, NULL, false) ...so why not gate async behavior on the presence of the 'bio' argument? > + int rc = 0; > + > + /* Create child bio for asynchronous flush and chain with > + * parent bio. Otherwise directly call nd_region flush. > + */ > + if (async && 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); I understand how this works, but it's a bit too "magical" for my taste. I would prefer that all flush implementations take an optional 'bio' argument rather than rely on the make_request implementation to stash the bio away on a driver specific list. > + } else { > + if (nd_region->flush(nd_region)) > + rc = -EIO; Given the common case wants to be fast and synchronous I think we should try to avoid retpoline overhead by default. So something like this: if (nd_region->flush == generic_nvdimm_flush) rc = generic_nvdimm_flush(...);
> > > > 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. > > > > This also handles asynchronous flush requests from the block layer > > by creating a child bio and chaining it with parent bio. > > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > > ---bio_chain Dan williams > [..] > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > > index b4ef7d9ff22e..fb1041ab32a6 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, false); > > + 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 = generic_nvdimm_flush; > > + > > nd_device_register(dev); > > > > return nd_region; > > @@ -1125,11 +1132,36 @@ 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, bool async) > > +{ > > I don't quite see the point of the 'async' argument. All the usages of > this routine are either > > nvdimm_flush(nd_region, bio, true) > ...or: > nvdimm_flush(nd_region, NULL, false) Agree. > > ...so why not gate async behavior on the presence of the 'bio' argument? Sure. > > > > + int rc = 0; > > + > > + /* Create child bio for asynchronous flush and chain with > > + * parent bio. Otherwise directly call nd_region flush. > > + */ > > + if (async && 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); > > I understand how this works, but it's a bit too "magical" for my > taste. I would prefer that all flush implementations take an optional > 'bio' argument rather than rely on the make_request implementation to > stash the bio away on a driver specific list. I did this to make use of "bio_chain" for chaining child bio for async flush suggested [1]. Are you saying to remove this and just call "flush" based on bio argument? Or I implemented the 'bio_chain' request entirely wrong? [1] https://lkml.org/lkml/2018/9/27/1028 > > > + } else { > > + if (nd_region->flush(nd_region)) > > + rc = -EIO; > > Given the common case wants to be fast and synchronous I think we > should try to avoid retpoline overhead by default. So something like > this: > > if (nd_region->flush == generic_nvdimm_flush) > rc = generic_nvdimm_flush(...); Sure. Thanks, Pankaj > >
On Thu, Apr 11, 2019 at 9:02 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. > > > > > > This also handles asynchronous flush requests from the block layer > > > by creating a child bio and chaining it with parent bio. > > > > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > > > ---bio_chain Dan williams > > [..] > > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > > > index b4ef7d9ff22e..fb1041ab32a6 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, false); > > > + 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 = generic_nvdimm_flush; > > > + > > > nd_device_register(dev); > > > > > > return nd_region; > > > @@ -1125,11 +1132,36 @@ 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, bool async) > > > +{ > > > > I don't quite see the point of the 'async' argument. All the usages of > > this routine are either > > > > nvdimm_flush(nd_region, bio, true) > > ...or: > > nvdimm_flush(nd_region, NULL, false) > > Agree. > > > > > ...so why not gate async behavior on the presence of the 'bio' argument? > > Sure. > > > > > > > > + int rc = 0; > > > + > > > + /* Create child bio for asynchronous flush and chain with > > > + * parent bio. Otherwise directly call nd_region flush. > > > + */ > > > + if (async && 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); > > > > I understand how this works, but it's a bit too "magical" for my > > taste. I would prefer that all flush implementations take an optional > > 'bio' argument rather than rely on the make_request implementation to > > stash the bio away on a driver specific list. > > I did this to make use of "bio_chain" for chaining child bio for async flush > suggested [1]. Are you saying to remove this and just call "flush" based on > bio argument? Or I implemented the 'bio_chain' request entirely wrong? No, I think you implemented it correctly. I'm just asking for the chaining to be performed internal to the ->flush() callback rather than in the common nvdimm_flush() front-end.
> > > > 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. > > > > > > > > This also handles asynchronous flush requests from the block layer > > > > by creating a child bio and chaining it with parent bio. > > > > > > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > > > > ---bio_chain Dan williams > > > [..] > > > > diff --git a/drivers/nvdimm/region_devs.c > > > > b/drivers/nvdimm/region_devs.c > > > > index b4ef7d9ff22e..fb1041ab32a6 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, false); > > > > + 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 = generic_nvdimm_flush; > > > > + > > > > nd_device_register(dev); > > > > > > > > return nd_region; > > > > @@ -1125,11 +1132,36 @@ 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, bool > > > > async) > > > > +{ > > > > > > I don't quite see the point of the 'async' argument. All the usages of > > > this routine are either > > > > > > nvdimm_flush(nd_region, bio, true) > > > ...or: > > > nvdimm_flush(nd_region, NULL, false) > > > > Agree. > > > > > > > > ...so why not gate async behavior on the presence of the 'bio' argument? > > > > Sure. > > > > > > > > > > > > + int rc = 0; > > > > + > > > > + /* Create child bio for asynchronous flush and chain with > > > > + * parent bio. Otherwise directly call nd_region flush. > > > > + */ > > > > + if (async && 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); > > > > > > I understand how this works, but it's a bit too "magical" for my > > > taste. I would prefer that all flush implementations take an optional > > > 'bio' argument rather than rely on the make_request implementation to > > > stash the bio away on a driver specific list. > > > > I did this to make use of "bio_chain" for chaining child bio for async > > flush > > suggested [1]. Are you saying to remove this and just call "flush" based on > > bio argument? Or I implemented the 'bio_chain' request entirely wrong? > > No, I think you implemented it correctly. I'm just asking for the > chaining to be performed internal to the ->flush() callback rather > than in the common nvdimm_flush() front-end. Sure. Perfect! Thank you very much for all the suggestions. Best regards, Pankaj >
On Thu 11-04-19 07:51:48, Dan Williams wrote: > On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote: > > + } else { > > + if (nd_region->flush(nd_region)) > > + rc = -EIO; > > Given the common case wants to be fast and synchronous I think we > should try to avoid retpoline overhead by default. So something like > this: > > if (nd_region->flush == generic_nvdimm_flush) > rc = generic_nvdimm_flush(...); I'd either add a comment about avoiding retpoline overhead here or just make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't get confused by the code. Honza
Jan Kara <jack@suse.cz> writes: > On Thu 11-04-19 07:51:48, Dan Williams wrote: >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote: >> > + } else { >> > + if (nd_region->flush(nd_region)) >> > + rc = -EIO; >> >> Given the common case wants to be fast and synchronous I think we >> should try to avoid retpoline overhead by default. So something like >> this: >> >> if (nd_region->flush == generic_nvdimm_flush) >> rc = generic_nvdimm_flush(...); > > I'd either add a comment about avoiding retpoline overhead here or just > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't > get confused by the code. Isn't this premature optimization? I really don't like adding things like this without some numbers to show it's worth it. -Jeff
Hello, Thank you for the suggestions on this. > > > On Thu 11-04-19 07:51:48, Dan Williams wrote: > >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote: > >> > + } else { > >> > + if (nd_region->flush(nd_region)) > >> > + rc = -EIO; > >> > >> Given the common case wants to be fast and synchronous I think we > >> should try to avoid retpoline overhead by default. So something like > >> this: > >> > >> if (nd_region->flush == generic_nvdimm_flush) > >> rc = generic_nvdimm_flush(...); > > > > I'd either add a comment about avoiding retpoline overhead here or just > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't > > get confused by the code. > > Isn't this premature optimization? I really don't like adding things > like this without some numbers to show it's worth it. Can we add the optimization after this version is merged. Best regards, Pankaj > > -Jeff > >
On Fri, Apr 12, 2019 at 6:12 AM Jeff Moyer <jmoyer@redhat.com> wrote: > > Jan Kara <jack@suse.cz> writes: > > > On Thu 11-04-19 07:51:48, Dan Williams wrote: > >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote: > >> > + } else { > >> > + if (nd_region->flush(nd_region)) > >> > + rc = -EIO; > >> > >> Given the common case wants to be fast and synchronous I think we > >> should try to avoid retpoline overhead by default. So something like > >> this: > >> > >> if (nd_region->flush == generic_nvdimm_flush) > >> rc = generic_nvdimm_flush(...); > > > > I'd either add a comment about avoiding retpoline overhead here or just > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't > > get confused by the code. > > Isn't this premature optimization? I really don't like adding things > like this without some numbers to show it's worth it. I don't think it's premature given this optimization technique is already being deployed elsewhere, see: https://lwn.net/Articles/774347/
Dan Williams <dan.j.williams@intel.com> writes: > On Fri, Apr 12, 2019 at 6:12 AM Jeff Moyer <jmoyer@redhat.com> wrote: >> >> Jan Kara <jack@suse.cz> writes: >> >> > On Thu 11-04-19 07:51:48, Dan Williams wrote: >> >> On Tue, Apr 9, 2019 at 9:09 PM Pankaj Gupta <pagupta@redhat.com> wrote: >> >> > + } else { >> >> > + if (nd_region->flush(nd_region)) >> >> > + rc = -EIO; >> >> >> >> Given the common case wants to be fast and synchronous I think we >> >> should try to avoid retpoline overhead by default. So something like >> >> this: >> >> >> >> if (nd_region->flush == generic_nvdimm_flush) >> >> rc = generic_nvdimm_flush(...); >> > >> > I'd either add a comment about avoiding retpoline overhead here or just >> > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't >> > get confused by the code. >> >> Isn't this premature optimization? I really don't like adding things >> like this without some numbers to show it's worth it. > > I don't think it's premature given this optimization technique is > already being deployed elsewhere, see: > > https://lwn.net/Articles/774347/ The technique is fine, but that doesn't mean it should be applied everywhere. Is *this* code path really going to benefit from the optimization? -Jeff
On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: > > > I'd either add a comment about avoiding retpoline overhead here or just > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't > > > get confused by the code. > > > > Isn't this premature optimization? I really don't like adding things > > like this without some numbers to show it's worth it. > > I don't think it's premature given this optimization technique is > already being deployed elsewhere, see: > > https://lwn.net/Articles/774347/ For one this one was backed by numbers, and second after feedback from Linux we switched to the NULL pointer check instead.
On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: > > > > I'd either add a comment about avoiding retpoline overhead here or just > > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't > > > > get confused by the code. > > > > > > Isn't this premature optimization? I really don't like adding things > > > like this without some numbers to show it's worth it. > > > > I don't think it's premature given this optimization technique is > > already being deployed elsewhere, see: > > > > https://lwn.net/Articles/774347/ > > For one this one was backed by numbers, and second after feedback > from Linux we switched to the NULL pointer check instead. Ok I should have noticed the switch to NULL pointer check. However, the question still stands do we want everyone to run numbers to justify this optimization, or make it a new common kernel coding practice to do: if (!object->op) generic_op(object); else object->op(object); ...in hot paths? I agree with not doing premature optimization in principle, but this hack is minimally intrusive from a readability perspective similar to likely()/unlikely() usage which also don't come with numbers on a per patch basis.
Dan Williams <dan.j.williams@intel.com> writes: > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote: >> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: >> > > > I'd either add a comment about avoiding retpoline overhead here or just >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't >> > > > get confused by the code. >> > > >> > > Isn't this premature optimization? I really don't like adding things >> > > like this without some numbers to show it's worth it. >> > >> > I don't think it's premature given this optimization technique is >> > already being deployed elsewhere, see: >> > >> > https://lwn.net/Articles/774347/ >> >> For one this one was backed by numbers, and second after feedback >> from Linux we switched to the NULL pointer check instead. > > Ok I should have noticed the switch to NULL pointer check. However, > the question still stands do we want everyone to run numbers to > justify this optimization, or make it a new common kernel coding > practice to do: > > if (!object->op) > generic_op(object); > else > object->op(object); > > ...in hot paths? I don't think nvdimm_flush is a hot path. Numbers of some representative workload would prove one of us right. -Jeff
On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer <jmoyer@redhat.com> wrote: > > Dan Williams <dan.j.williams@intel.com> writes: > > > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote: > >> > >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: > >> > > > I'd either add a comment about avoiding retpoline overhead here or just > >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't > >> > > > get confused by the code. > >> > > > >> > > Isn't this premature optimization? I really don't like adding things > >> > > like this without some numbers to show it's worth it. > >> > > >> > I don't think it's premature given this optimization technique is > >> > already being deployed elsewhere, see: > >> > > >> > https://lwn.net/Articles/774347/ > >> > >> For one this one was backed by numbers, and second after feedback > >> from Linux we switched to the NULL pointer check instead. > > > > Ok I should have noticed the switch to NULL pointer check. However, > > the question still stands do we want everyone to run numbers to > > justify this optimization, or make it a new common kernel coding > > practice to do: > > > > if (!object->op) > > generic_op(object); > > else > > object->op(object); > > > > ...in hot paths? > > I don't think nvdimm_flush is a hot path. Numbers of some > representative workload would prove one of us right. I'd rather say that the if "if (!op) do_generic()" pattern is more readable in the general case, saves grepping for who set the op in the common case. The fact that it has the potential to be faster is gravy at that point.
Dan Williams <dan.j.williams@intel.com> writes: > On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer <jmoyer@redhat.com> wrote: >> >> Dan Williams <dan.j.williams@intel.com> writes: >> >> > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> wrote: >> >> >> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: >> >> > > > I'd either add a comment about avoiding retpoline overhead here or just >> >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't >> >> > > > get confused by the code. >> >> > > >> >> > > Isn't this premature optimization? I really don't like adding things >> >> > > like this without some numbers to show it's worth it. >> >> > >> >> > I don't think it's premature given this optimization technique is >> >> > already being deployed elsewhere, see: >> >> > >> >> > https://lwn.net/Articles/774347/ >> >> >> >> For one this one was backed by numbers, and second after feedback >> >> from Linux we switched to the NULL pointer check instead. >> > >> > Ok I should have noticed the switch to NULL pointer check. However, >> > the question still stands do we want everyone to run numbers to >> > justify this optimization, or make it a new common kernel coding >> > practice to do: >> > >> > if (!object->op) >> > generic_op(object); >> > else >> > object->op(object); >> > >> > ...in hot paths? >> >> I don't think nvdimm_flush is a hot path. Numbers of some >> representative workload would prove one of us right. > > I'd rather say that the if "if (!op) do_generic()" pattern is more > readable in the general case, saves grepping for who set the op in the > common case. The fact that it has the potential to be faster is gravy > at that point. If the primary motivation is performance, then I'd expect performance numbers to back it up. If that isn't the primary motivation, then choose whichever way you feel is appropriate. Cheers, Jeff
> > Dan Williams <dan.j.williams@intel.com> writes: > > > On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer <jmoyer@redhat.com> wrote: > >> > >> Dan Williams <dan.j.williams@intel.com> writes: > >> > >> > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig <hch@infradead.org> > >> > wrote: > >> >> > >> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: > >> >> > > > I'd either add a comment about avoiding retpoline overhead here > >> >> > > > or just > >> >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that > >> >> > > > people don't > >> >> > > > get confused by the code. > >> >> > > > >> >> > > Isn't this premature optimization? I really don't like adding > >> >> > > things > >> >> > > like this without some numbers to show it's worth it. > >> >> > > >> >> > I don't think it's premature given this optimization technique is > >> >> > already being deployed elsewhere, see: > >> >> > > >> >> > https://lwn.net/Articles/774347/ > >> >> > >> >> For one this one was backed by numbers, and second after feedback > >> >> from Linux we switched to the NULL pointer check instead. > >> > > >> > Ok I should have noticed the switch to NULL pointer check. However, > >> > the question still stands do we want everyone to run numbers to > >> > justify this optimization, or make it a new common kernel coding > >> > practice to do: > >> > > >> > if (!object->op) > >> > generic_op(object); > >> > else > >> > object->op(object); > >> > > >> > ...in hot paths? > >> > >> I don't think nvdimm_flush is a hot path. Numbers of some > >> representative workload would prove one of us right. > > > > I'd rather say that the if "if (!op) do_generic()" pattern is more > > readable in the general case, saves grepping for who set the op in the > > common case. The fact that it has the potential to be faster is gravy > > at that point. > > If the primary motivation is performance, then I'd expect performance > numbers to back it up. If that isn't the primary motivation, then > choose whichever way you feel is appropriate. Agree. This change enhances the code readability. Will add this change in v6 with other changes. Thank you! Pankaj > > Cheers, > Jeff >
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 5a389a4f4f65..567017a2190e 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, false); 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, false); 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..a1dfa066786b 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, false); + if (ret) + rc = ret; return rc; } diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h index a5ac3b240293..916cd6c5451a 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 nd_mapping mapping[0]; }; diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index bc2f700feef8..5a5b3ea4d073 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, true); 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, true); + + if (ret) + bio->bi_status = errno_to_blk_status(ret); bio_endio(bio); return BLK_QC_T_NONE; @@ -463,13 +467,13 @@ static int pmem_attach_disk(struct device *dev, disk->bb = &pmem->bb; dax_dev = alloc_dax(pmem, disk->disk_name, &pmem_dax_ops); + if (!dax_dev) { put_disk(disk); return -ENOMEM; } 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 +531,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, false); 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, false); } 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..fb1041ab32a6 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, false); + 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 = generic_nvdimm_flush; + nd_device_register(dev); return nd_region; @@ -1125,11 +1132,36 @@ 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, bool async) +{ + int rc = 0; + + /* Create child bio for asynchronous flush and chain with + * parent bio. Otherwise directly call nd_region flush. + */ + if (async && 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); + } else { + if (nd_region->flush(nd_region)) + 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 +1185,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..d9d2ab8a6e64 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 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, bool async); +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);
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. This also handles asynchronous flush requests from the block layer by creating a child bio and chaining it with parent bio. 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 | 14 ++++++++----- drivers/nvdimm/region_devs.c | 38 ++++++++++++++++++++++++++++++++++-- include/linux/libnvdimm.h | 8 +++++++- 6 files changed, 59 insertions(+), 12 deletions(-)