[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[Hannes Reinecke]

Bit 6 in the ANACAP field is used to indicate that the ANA group ID
doesn't change while the namespace is attached to the controller.
There is an optimisation in the code to only allocate space
for the ANA group header, as the namespace list won't change and
hence would not need to be refreshed.
However, this optimisation was never carried over to the actual
workflow, which always assumes that the buffer is large enough
to hold the ANA header _and_ the namespace list.
So drop this optimisation and always allocate enough space.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index df4b3a6db51b..b9fff3b8ed1b 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -545,8 +545,7 @@ int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	timer_setup(&ctrl->anatt_timer, nvme_anatt_timeout, 0);
 	ctrl->ana_log_size = sizeof(struct nvme_ana_rsp_hdr) +
 		ctrl->nanagrpid * sizeof(struct nvme_ana_group_desc);
-	if (!(ctrl->anacap & (1 << 6)))
-		ctrl->ana_log_size += ctrl->max_namespaces * sizeof(__le32);
+	ctrl->ana_log_size += ctrl->max_namespaces * sizeof(__le32);
 
 	if (ctrl->ana_log_size > ctrl->max_hw_sectors << SECTOR_SHIFT) {
 		dev_err(ctrl->device,
-- 
2.16.4

[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[Christoph Hellwig]

On Wed, Jan 09, 2019@09:45:15AM +0100, Hannes Reinecke wrote:
> Bit 6 in the ANACAP field is used to indicate that the ANA group ID
> doesn't change while the namespace is attached to the controller.
> There is an optimisation in the code to only allocate space
> for the ANA group header, as the namespace list won't change and
> hence would not need to be refreshed.
> However, this optimisation was never carried over to the actual
> workflow, which always assumes that the buffer is large enough
> to hold the ANA header _and_ the namespace list.
> So drop this optimisation and always allocate enough space.

Isn't the real bug that we always clear groups_only in nvme_ana_work,
and we could simply make the conditional instead?

[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[Christoph Hellwig]

On Wed, Jan 09, 2019@07:43:00PM +0100, Christoph Hellwig wrote:
> Isn't the real bug that we always clear groups_only in nvme_ana_work,
> and we could simply make the conditional instead?

Hannes, can you look into this?

[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[Christoph Hellwig]

Ok, based on the version that actually keeps the optimization I
have to say I actually prefer this variant, given that it is a lot
less code and special cases.  Sorry for sending you off the wrong track.

Reviewed-by: Christoph Hellwig <hch at lst.de>

Note that we could also remove the cached anacap value now.

[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[Sagi Grimberg]

> Ok, based on the version that actually keeps the optimization I
> have to say I actually prefer this variant, given that it is a lot
> less code and special cases.  Sorry for sending you off the wrong track.
> 
> Reviewed-by: Christoph Hellwig <hch at lst.de>

Oh, so this ends up preferred over v2... I don't have any problem
with allocating for the worse case here...

Doesn't look like this is urgent for 5.0 rc, shall we queue it
for 5.1?

[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[Christoph Hellwig]

On Mon, Jan 21, 2019@01:23:39AM -0800, Sagi Grimberg wrote:
>
>> Ok, based on the version that actually keeps the optimization I
>> have to say I actually prefer this variant, given that it is a lot
>> less code and special cases.  Sorry for sending you off the wrong track.
>>
>> Reviewed-by: Christoph Hellwig <hch at lst.de>
>
> Oh, so this ends up preferred over v2... I don't have any problem
> with allocating for the worse case here...
>
> Doesn't look like this is urgent for 5.0 rc, shall we queue it
> for 5.1?

Without it we use unallocate memory if the controller sets bit 6
in anacap.  So yes, I think it should go into 5.0 and -stable.

[PATCH] nvme-multipath: drop optimization for static ANA group IDs

[Sagi Grimberg]

>>> Ok, based on the version that actually keeps the optimization I
>>> have to say I actually prefer this variant, given that it is a lot
>>> less code and special cases.  Sorry for sending you off the wrong track.
>>>
>>> Reviewed-by: Christoph Hellwig <hch at lst.de>
>>
>> Oh, so this ends up preferred over v2... I don't have any problem
>> with allocating for the worse case here...
>>
>> Doesn't look like this is urgent for 5.0 rc, shall we queue it
>> for 5.1?
> 
> Without it we use unallocate memory if the controller sets bit 6
> in anacap.  So yes, I think it should go into 5.0 and -stable.

OK, applied to nvme-5.0