[08/14] nvmet: implement the changed namespaces log

Commit Message

Christoph Hellwig May 29, 2018, 5:24 p.m. UTC

On Tue, May 29, 2018 at 04:59:05PM +0000, Verkamp, Daniel wrote:
> > +	} else if (ctrl->nr_changed_ns == NVME_MAX_CHANGED_NAMESPACES) {
> > +		ctrl->changed_ns_list[0] = cpu_to_le32(0xffffffff);
> > +	}
> 
> Unless I'm missing it happening somewhere else, the list-full case that sets element 0 to 0xffffffff should also explicitly zero out the rest of the list to satisfy the "remainder of the list shall be zero-filled" wording in the spec, since the other changed_ns_list entries will be filled with non-zero NSIDs when we get here.

True.  We actually zero out unused elements already, but that doesn't
catch the ctrl->nr_changed_ns == NVME_MAX_CHANGED_NAMESPACES special
case.  This relative patch should fix it:

Comments

Daniel Verkamp May 29, 2018, 8:44 p.m. UTC | #1

On 05/29/2018 10:24 AM, Christoph Hellwig wrote:
> On Tue, May 29, 2018 at 04:59:05PM +0000, Verkamp, Daniel wrote:
>>> +	} else if (ctrl->nr_changed_ns == NVME_MAX_CHANGED_NAMESPACES) {
>>> +		ctrl->changed_ns_list[0] = cpu_to_le32(0xffffffff);
>>> +	}
>>
>> Unless I'm missing it happening somewhere else, the list-full case that sets element 0 to 0xffffffff should also explicitly zero out the rest of the list to satisfy the "remainder of the list shall be zero-filled" wording in the spec, since the other changed_ns_list entries will be filled with non-zero NSIDs when we get here.
> 
> True.  We actually zero out unused elements already, but that doesn't
> catch the ctrl->nr_changed_ns == NVME_MAX_CHANGED_NAMESPACES special
> case.  This relative patch should fix it:
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index d7b6293e830b..7b69c348d608 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -136,7 +136,10 @@ static void nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
>  		goto out;
>  
>  	mutex_lock(&ctrl->lock);
> -	len = ctrl->nr_changed_ns * sizeof(__le32);
> +	if (ctrl->nr_changed_ns == NVME_MAX_CHANGED_NAMESPACES)
> +		len = sizeof(__le32);
> +	else
> +		len = ctrl->nr_changed_ns * sizeof(__le32);
>  	status = nvmet_copy_to_sgl(req, 0, ctrl->changed_ns_list, len);
>  	if (!status)
>  		status = nvmet_zero_sgl(req, len, req->data_len - len);
> 

I'm not sure that's quite right; if nr_changed_ns == NVME_MAX_CHANGED_NAMESPACES but we haven't overflowed (in other words, the list has exactly NVME_MAX_CHANGED_NAMESPACES NSIDs), this will still only copy over the first entry.

I think it needs a:
	&& ctrlr->changed_ns_list[0] == cpu_to_le32(0xffffffff)
in the NVME_MAX_CHANGED_NAMESPACES condition to make it fully correct.

Thanks,
-- Daniel

Patch

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index d7b6293e830b..7b69c348d608 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -136,7 +136,10 @@  static void nvmet_execute_get_log_changed_ns(struct nvmet_req *req)
 		goto out;
 
 	mutex_lock(&ctrl->lock);
-	len = ctrl->nr_changed_ns * sizeof(__le32);
+	if (ctrl->nr_changed_ns == NVME_MAX_CHANGED_NAMESPACES)
+		len = sizeof(__le32);
+	else
+		len = ctrl->nr_changed_ns * sizeof(__le32);
 	status = nvmet_copy_to_sgl(req, 0, ctrl->changed_ns_list, len);
 	if (!status)
 		status = nvmet_zero_sgl(req, len, req->data_len - len);