[nvmewin] Review comments for Namespace Management patch

Foster, Carolyn D carolyn.d.foster at intel.com
Fri Jan 29 08:05:34 PST 2016


Hi Suman, Thank you for the feedback.  I will review your comments and make the appropriate changes.  I will try to send the updates out for review in the next two weeks.
Carolyn
From: SUMAN PRAKASH B [mailto:suman.p at samsung.com]
Sent: Wednesday, January 27, 2016 9:58 AM
To: nvmewin-request at lists.openfabrics.org; nvmewin at lists.openfabrics.org; Foster, Carolyn D <carolyn.d.foster at intel.com>; Robles, Raymond C <raymond.c.robles at intel.com>
Cc: judy.brock at ssi.samsung.com; tru.nguyen at ssi.samsung.com; MANOJ THAPLIYAL <m.thapliyal at samsung.com>; anshul at samsung.com
Subject: Review comments for Namespace Management patch


Hi Carolyn,



Please find below our Review comments and Queries.



Review Comments:

1.  Function:  NVMeIoctlNamespaceMgmt()   Line no:  4005   File: nvmeStd.c
In the NVMeIoctlNamespaceMgmt() function, the namesapce status verification can be done only incase of NAMESPACE_MANAGEMENT_DELETE as for NAMESPACE_MANAGEMENT_CREATE, the NSID value is reserved.
NVMeGetNamespaceStatusAndSlot(pDevExt, currentNSID, &lunId); This function can be moved inside the below condition.
   if (pNsMgmtDW10->SEL == NAMESPACE_MANAGEMENT_DELETE) { }



2. Function:  NVMeIoctlNamespaceMgmt()   Line no:  4017   File: nvmeStd.c
The DataBuffer(pNvmePtIoctl->DataBuffer) may not be allocated any memory as no data structure required for NAMESPACE_MANAGEMENT_DELETE operation.
pNsCtrlsList = (PNVMe_CONTROLLER_LIST)&pNvmePtIoctl->DataBuffer;

 It is good to allocate memory for pNsCtrlsList and free the memory after command completion.



3. Function:  NVMeCompletionNsAttachment() Line No: 4138  File: nvmeStd.c
It is safe to set SEL field  to NAMESPACE_MANAGEMENT_DELETE  in the pNsMgmtCmd structure.



4. Function: NVMeIoctlNamespaceAttachment() Line no: 3931  File: nvmeStd.c

The StorPortNotification(BusChangeDetected, pDevExt); has been called twice while performing the NAMESPACE DELETE.

This is for the below sequence of operations.

    a. Create Namespace

    b. Attach Namespace

    c. Delete Namespace

The overall execution flow looks as below.

1) Namespace Delete command issued.
2) Set:   pLunExt->slotStatus = OFFLINE;
       pLunExt->offlineReason = DELETE_IN_PROGRESS;

3) StorPortNotification(BusChangeDetected, pDevExt);

4) Namespace Detach Command Issued
5) Detach Command Success

6) Set: pLunExt->nsStatus = INACTIVE;
        pLunExt->slotStatus = FREE;
      pLunExt->ReadOnly = TRUE;

7)  pDevExt->visibleLuns--;
8) StorPortNotification(BusChangeDetected, pDevExt);

9) Delete Command Issued
10)Delete Command Success
11) Set Lun Extn Table to zero



The same behavior observed while executing NAMESPACE DETACH command.

This is for the below sequence of operations.

    a. Create Namespace

    b. Attach Namespace

    c. Detach Namespace

The overall execution flow looks  as below.

1) Namespace Detach command issued.
2) Set:   pLunExt->slotStatus = OFFLINE;
       pLunExt->offlineReason = DETACH_IN_PROGRESS;

3) StorPortNotification(BusChangeDetected, pDevExt);

4) Detach Command Success

5) Set: pLunExt->nsStatus = INACTIVE;
        pLunExt->slotStatus = FREE;
       pLunExt->ReadOnly = TRUE;

       pLunExt->OfflineReason = NOT_OFFLINE;

6)  pDevExt->visibleLuns--;
7) StorPortNotification(BusChangeDetected, pDevExt);

This StorPortNotification() call of multiple times can be avoided.



Queries:

1) Looks like The Identify namespace, set feature/get feature commands during the init state machine have been sent to  inactive namespaces also.

For example one of the below check condition, leads to the above issue. This has to be taken care in all the scenario.

Function: NVMeSetFeaturesCompletion()   Line no:  1631  File: nvmeInit.c
  if (pAE->DriverState.TtlLbaRangeExamined ==
    pAE->DriverState.NumKnownNamespaces) {
                /* We have called identify namespace as well as get/set
                 * features for each of the NN namespaces that exist.
                 * Move on to the next state in the state machine.
                 */
                pAE->visibleLuns = pAE->DriverState.VisibleNamespacesExamined;
                pAE->DriverState.NextDriverState = NVMeWaitOnSetupQueues;
            }

Could you please confirm is it required to issue commands to the inactive namespaces also? If not, we can keep track of active namespaces and issue commands to only those namespaces.



2) Function: NVMeCompletionNsAttachment() Line no: 4099  File: nvmeStd.c

Could you please confirm if its a Identify Controller or Identify Namespace commands?

If this is Identify Controller command, the NSID need not be set and CNS should be mentioned.

If this is Identify Namespace command, then size of PRP preparation should not be sizeof(ADMIN_IDENTIFY_CONTROLLER) at line no: 4102





Thanks,

Suman



----------------------------------------------------------------------

Message: 1
Date: Fri, 15 Jan 2016 23:57:26 +0000
From: "Foster, Carolyn D" <carolyn.d.foster at intel.com<mailto:carolyn.d.foster at intel.com>>
To: "nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>" <nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>>
Subject: [nvmewin] Patch with changes for Namespace Management
Message-ID:
 <B3A485AFDDB1DD4598621E85E8EB67A83C26A801 at FMSMSX105.amr.corp.intel.com<mailto:B3A485AFDDB1DD4598621E85E8EB67A83C26A801 at FMSMSX105.amr.corp.intel.com>>

Content-Type: text/plain; charset="us-ascii"

Hi all,
This patch includes changes to support Namespace Management updates from the NVMe specification 1.2.  This patch implements some fixes for handling non-continuous namespaces, adds handling for attached and detached namespaces, and implements new IOCTLs to create, delete, attach and detach namespaces.

I have made a detailed overview of the changes in the text file in the attached zip file, the contents are also copied here below.

Password is intelnvme


Please let me know if you have any questions.

Carolyn Foster



This patch includes changes to support Namespace management, including create, delete,
attach and detach namespace operations.  The new functionality in this patch was tested
using proprietary tools.  We tested on Server 2008 R2, Server 2012 R2 and Windows 8.1


******************
Design Assumptions
******************

1. The numbering of namespaces need not be consecutive.
2. The namespace ID can be any number between 1 and 2^32.
3. A namespace is considered "active" only when it is created and attached to this controller.
4. A detached namespace, or one that is just created but not yet attached is considered "inactive".
5. A non-existent, or deleted namespace is considered "invalid".
6. An active namespace will result in one (and only one) "Online" LUN.
7. Assuming single-host, and single-controller NVMe system.

*********************
Architecture Overview
*********************

There are four new IOCTLs for namespace management, Create, Delete, Attach and Detach.  An attached
namespace will result in a visible LUN to the Windows OS.  The LUN extension table has been updated
to have a Namespace status:

typedef enum _NS_STATUS
{
    INVALID = 0,    //Namespace ID does not exist (not known to controller).
    INACTIVE,       //Namespace is created, but not attached to controller.
    ATTACHED        //Namespace is created and attached to controller.
} NS_STATUS;

In order to properly build the LUN extension table during initialization, we made changes to identify
all namespaces, and to determine each namespace's status.  These changes include new states in the
Init State Machine NVMeRnningWaitOnListAttachedNs and NVMeRunningWaitOnListExistingNs.  The updated
state machine steps are as follows:

1. Send an Identify Namespace command with CNS set to 02h. This should return a list of all active (created and attached) namespaces.
2. Go through the list and update LUN extension entries accordingly, one entry for each namespace. Set all LUN status to online.
3. Send an Identify Namespace command with CNS set to 10h. This should return a list of all existing namespaces in the system, active and inactive.
4. Go through the list.
5. If a corresponding LUN entry exists, skip this step, as this must have been an active namespace that was covered in previous steps.
LUN extension entries are populated as follows:

When a namespace is created:
- namespaceId is set.
- nsStatus is set to "INACTIVE"
- slotStatus is set to "FREE"
- identifyData is partially set

When a namespace is attached:
- drive is pulled for namespace identify
- identifyData is set accordingly
- nsStatus is set to "ATTACHED"
- slotStatus is set to "ONLINE"
- ReadOnly is set to FALSE

When a namespace is detached:
- nsStatus is set to "INACTIVE"
- slotStatus is set to "FREE"
- ReadOnly is set to TRUE

When a namespace is deleted:
- The entire LUN extension entry is set to 0.

There are also new reasons for the LUN to be offline:

typedef enum _LUN_OFFLINE_REASON
{
    NOT_OFFLINE,
    FORMAT_IN_PROGRESS,
    DETACH_IN_PROGRESS,
    DELETE_IN_PROGRESS
    // Add more as needed
} LUN_OFFLINE_REASON;

When delete or detach requests are made, the driver will call StorportDeviceBusy to pause incoming requests,
and the LUN is marked as offline with the appropriate reason.  When a user tries to delete an attached namespace,
the driver will first send a detach command, and then the delete command.

*****************
Known Limitations
*****************

1. If no namepsaces are present, the driver will fail to load.
2. If an error happens on any one namespace during initialization the driver will fail to load.

The handling for these two scenarios could be strengthened and improved, which we did not get to in this patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20160115/6e282642/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IntelNamespaceManagementV1.zip
Type: application/x-zip-compressed
Size: 222253 bytes
Desc: IntelNamespaceManagementV1.zip
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20160115/6e282642/attachment.bin>

------------------------------

_______________________________________________
nvmewin mailing list
nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
http://lists.openfabrics.org/mailman/listinfo/nvmewin


End of nvmewin Digest, Vol 49, Issue 3
**************************************

<p> </p><p> </p>
[http://ext.samsung.net/mailcheck/SeenTimeChecker?do=28bccb0e7a1e0d06003ec0f993c867c045ed77d46f93657db8a3fd6b4a260c0ed33a9d35f6e1735f20a30c65ae77ad69c7b41e955949e5c8a728c55b39cc59eacf878f9a26ce15a0]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20160129/8192383f/attachment.html>


More information about the nvmewin mailing list