* [PATCH] NVMe: Do not save result for range type feature
2013-04-03 12:38 ` Matthew Wilcox
@ 2013-03-23 4:16 ` Keith Busch
2013-04-03 21:45 ` Busch, Keith
0 siblings, 1 reply; 4+ messages in thread
From: Keith Busch @ 2013-03-23 4:16 UTC (permalink / raw)
On Wed, 3 Apr 2013, Matthew Wilcox wrote:
> On Tue, Apr 02, 2013@02:31:35PM -0600, Keith Busch wrote:
>> The LBA range type feature being optional, we do not want to save
>> the result or propagate it up the call stack when a device does not
>> support it.
>
> Looking at the whole function, the description of what it returns
> is rather odd. "If nvme_setup_io_queues fails, returns that error.
> If nvme_identify for the controller fails, returns -EIO. Otherwise,
> return an error if nvme_identify for the last namespace fails or if
> nvme_get_features for the last namespace fails."
>
> So we always discard errors for not-the-last namespace, but return an
> error if identify or get_features for the last namespace gets an error.
> Worse, on returning an error, the caller will free the queues and all the
> controller structures, even though the namespaces have been registered as
> devices! I really didn't think through the error path here, did I? :-)
>
> So, I would propose that we _always_ discard any errors we hit going through
> the namespace addition loop. As long as we've been able to set up queues
> and identify the controller, this function should return success.
>
> And I think *that* patch is as simple as adding:
>
> list_add_tail(&ns->list, &dev->namespaces);
> }
> + res = 0;
> list_for_each_entry(ns, &dev->namespaces, list)
> add_disk(ns->disk);
>
> What do you think?
Great idea! Better than waiting for each possible failure to occur and
have a bunch of patches to fix essentially the same problem. :)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] NVMe: Do not save result for range type feature
@ 2013-04-02 20:31 Keith Busch
2013-04-03 12:38 ` Matthew Wilcox
0 siblings, 1 reply; 4+ messages in thread
From: Keith Busch @ 2013-04-02 20:31 UTC (permalink / raw)
The LBA range type feature being optional, we do not want to save
the result or propagate it up the call stack when a device does not
support it.
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
drivers/block/nvme-core.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index a89f7db..00819d7 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1544,9 +1544,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
if (id_ns->ncap == 0)
continue;
- res = nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
- dma_addr + 4096, NULL);
- if (res)
+ if (nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
+ dma_addr + 4096, NULL))
memset(mem + 4096, 0, 4096);
ns = nvme_alloc_ns(dev, i, mem, mem + 4096);
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] NVMe: Do not save result for range type feature
2013-04-02 20:31 [PATCH] NVMe: Do not save result for range type feature Keith Busch
@ 2013-04-03 12:38 ` Matthew Wilcox
2013-03-23 4:16 ` Keith Busch
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2013-04-03 12:38 UTC (permalink / raw)
On Tue, Apr 02, 2013@02:31:35PM -0600, Keith Busch wrote:
> The LBA range type feature being optional, we do not want to save
> the result or propagate it up the call stack when a device does not
> support it.
Looking at the whole function, the description of what it returns
is rather odd. "If nvme_setup_io_queues fails, returns that error.
If nvme_identify for the controller fails, returns -EIO. Otherwise,
return an error if nvme_identify for the last namespace fails or if
nvme_get_features for the last namespace fails."
So we always discard errors for not-the-last namespace, but return an
error if identify or get_features for the last namespace gets an error.
Worse, on returning an error, the caller will free the queues and all the
controller structures, even though the namespaces have been registered as
devices! I really didn't think through the error path here, did I? :-)
So, I would propose that we _always_ discard any errors we hit going through
the namespace addition loop. As long as we've been able to set up queues
and identify the controller, this function should return success.
And I think *that* patch is as simple as adding:
list_add_tail(&ns->list, &dev->namespaces);
}
+ res = 0;
list_for_each_entry(ns, &dev->namespaces, list)
add_disk(ns->disk);
What do you think?
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
> drivers/block/nvme-core.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index a89f7db..00819d7 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -1544,9 +1544,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
> if (id_ns->ncap == 0)
> continue;
>
> - res = nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
> - dma_addr + 4096, NULL);
> - if (res)
> + if (nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
> + dma_addr + 4096, NULL))
> memset(mem + 4096, 0, 4096);
>
> ns = nvme_alloc_ns(dev, i, mem, mem + 4096);
> --
> 1.7.1
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://merlin.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] NVMe: Do not save result for range type feature
2013-03-23 4:16 ` Keith Busch
@ 2013-04-03 21:45 ` Busch, Keith
0 siblings, 0 replies; 4+ messages in thread
From: Busch, Keith @ 2013-04-03 21:45 UTC (permalink / raw)
On Wed, 3 Apr 2013, Matthew Wilcox wrote:
>
> > On Tue, Apr 02, 2013@02:31:35PM -0600, Keith Busch wrote:
> >> The LBA range type feature being optional, we do not want to save
> the
> >> result or propagate it up the call stack when a device does not
> >> support it.
> >
> > Looking at the whole function, the description of what it returns is
> > rather odd. "If nvme_setup_io_queues fails, returns that error.
> > If nvme_identify for the controller fails, returns -EIO. Otherwise,
> > return an error if nvme_identify for the last namespace fails or if
> > nvme_get_features for the last namespace fails."
> >
> > So we always discard errors for not-the-last namespace, but return an
> > error if identify or get_features for the last namespace gets an
> error.
> > Worse, on returning an error, the caller will free the queues and all
> > the controller structures, even though the namespaces have been
> > registered as devices! I really didn't think through the error path
> > here, did I? :-)
> >
> > So, I would propose that we _always_ discard any errors we hit going
> > through the namespace addition loop. As long as we've been able to
> > set up queues and identify the controller, this function should
> return success.
> >
> > And I think *that* patch is as simple as adding:
> >
> > list_add_tail(&ns->list, &dev->namespaces);
> > }
> > + res = 0;
> > list_for_each_entry(ns, &dev->namespaces, list)
> > add_disk(ns->disk);
> >
> > What do you think?
Great idea! Better than waiting for each possible failure to occur and
have a bunch of patches to fix essentially the same problem. :)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-04-03 21:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-02 20:31 [PATCH] NVMe: Do not save result for range type feature Keith Busch
2013-04-03 12:38 ` Matthew Wilcox
2013-03-23 4:16 ` Keith Busch
2013-04-03 21:45 ` Busch, Keith
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.