NVMe target log page offset not supported

NVMe target log page offset not supported

[Verkamp, Daniel]

Hello,

It looks like the Linux NVMe over Fabrics target Get Log Page implementation is missing support for the Log Page Offset field, which is part of the optional "extended data for Get Log Page" feature.

Of the two Get Log Page implementations, in drivers/nvme/target/{admin-cmd.c,discovery.c}, both advertise support for extended data for Get Log Page in the Identify Controller LPA field (bit 2), but neither looks at the offset field (CDW12 and CDW13, named LPOL and LPOU in the spec).

In theory, it is tempting to just stop advertising the extended log page feature, but reading between the lines in the NVMe-oF 1.0 spec, it seems like this might be intended to be a required feature for at least discovery controllers (e.g. the Discovery Log Page section says "The Log Page Offset is used (if needed) to retrieve specific records").  Maybe the feature should be temporarily disabled anyway until the offset is correctly implemented?

It also looks like the discovery.c Get Log Page implementation has some funny ideas about the meaning of the log page length (Number of Dwords field).  A request with a length that doesn't fully cover a discovery log entry will not return the partial data as requested but will instead stop at the last full entry and skip the entire final partial entry, leaving it as zeroes.  To be fair, as is usual with NVMe, the spec doesn't actually say what to do in this situation (it only defines expected behavior when the user requests more than the total length of the log page), but it seems like the right thing to do would be to return exactly the [offset, offset + length) bytes of the log page, even if that range overlaps the beginning/end of a discovery entry (or the header, which can happen once offset is implemented).

Unfortunately, I don't have time right now to write and thoroughly test a patch for this, so I thought I'd at least make it known.  It's on my todo list...

Thanks,
-- Daniel

NVMe target log page offset not supported

[Christoph Helwig]

On Wed, Oct 18, 2017@11:52:04PM +0000, Verkamp, Daniel wrote:
> Unfortunately, I don't have time right now to write and thoroughly test a patch for this, so I thought I'd at least make it known.  It's on my todo list...

Yes, implementing it should be fairly easy, but I'd like to start
wanting test coverage for things like this.

Chaitanya has a really nice test framework, I think it might be a good
fit for that.

NVMe target log page offset not supported

[Johannes Thumshirn]

Christoph Helwig <hch at lst.de> writes:
> Chaitanya has a really nice test framework, I think it might be a good

Do you (or Chaitanya) have a link to this test framework? You
promised it to me quite some time ago ;-)

Thanks,
        Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

NVMe target log page offset not supported

[Chaitanya Kulkarni]

Hi Johannes,

I was able to incorporate all the comments received on the first version and added some new testcases.

Please have a look at the following link:-

https://github.com/ChaitanayaKulkarni/nvmftests.

Following is the list of the changes since V1 :-

    1. Addition of new testcases to improve the coverage.
    2. Improved directory structure for code clarity.
    3. Libification of the common code.
    4. Support to use different block devices on the target side.
    5. Centralize test configuration management.
    6. Support to execute File System related operations on the host side.
    7. Adjust the code to incorporate the comments received for the
       first version.
    8. Support for setting different log levels for each component.
    9. Support for quick performance measurements.
    10. Class hierarchy documentation and sequence diagram.
    11. Improved workqueue management for host namesapce to
        run parallel commands.

It will be great if I can get some feedback, I'll be happy to incorporate
the comments and make it more usable.

After completing the comments for this version may be we can tag a release.

Regards,
-Chaitanya







From: Johannes Thumshirn <jthumshirn@suse.de>
Sent: Thursday, October 19, 2017 2:04 AM
To: Christoph Helwig
Cc: Verkamp, Daniel; Sagi Grimberg; linux-nvme at lists.infradead.org; Chaitanya Kulkarni; Hannes Reinecke
Subject: Re: NVMe target log page offset not supported
?    
Christoph Helwig <hch at lst.de> writes:
> Chaitanya has a really nice test framework, I think it might be a good

Do you (or Chaitanya) have a link to this test framework? You
promised it to me quite some time ago ;-)

Thanks,
??????? Johannes
-- 
Johannes Thumshirn????????????????????????????????????????? Storage
jthumshirn at suse.de??????????????????????????????? +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
    

NVMe target log page offset not supported

[Sagi Grimberg]

> Hi Johannes,
> 
> I was able to incorporate all the comments received on the first version and added some new testcases.
> 
> Please have a look at the following link:-
> 
> https://github.com/ChaitanayaKulkarni/nvmftests.
> 
> Following is the list of the changes since V1 :-
> 
>      1. Addition of new testcases to improve the coverage.
>      2. Improved directory structure for code clarity.
>      3. Libification of the common code.
>      4. Support to use different block devices on the target side.
>      5. Centralize test configuration management.
>      6. Support to execute File System related operations on the host side.
>      7. Adjust the code to incorporate the comments received for the
>         first version.
>      8. Support for setting different log levels for each component.
>      9. Support for quick performance measurements.
>      10. Class hierarchy documentation and sequence diagram.
>      11. Improved workqueue management for host namesapce to
>          run parallel commands.

Hi Chaitanya,

Would it be possible to post patches so we can effectively provide
feedback?

One thing by a 5 seconds browse, its a shame that you work directly
on top of configfs and not import nvme plugin from nvmetcli repo...

NVMe target log page offset not supported

[Johannes Thumshirn]

Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com> writes:

> Hi Johannes,
>
> I was able to incorporate all the comments received on the first version and added some new testcases.
>
> Please have a look at the following link:-
>
> https://github.com/ChaitanayaKulkarni/nvmftests.

I'll have a look at this version.

Thanks a lot,
       Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

NVMe target log page offset not supported

[Chaitanya Kulkarni]

Sure, I'll post a patch series for the latest version.

With the new structure of the code, it should be easy to add nvme plug-in from the nvmetcli.

Prior to that what I'm looking for in the comments:-

1. What are new file system types we should add in the utils? 
2. Is threading model on the host side needs to be replicated on the target side?
3. Right now we have a single thread for each namespace and associated work queue to execute the command on the namespace for the host, similar to namespace do we really need a separate thread for each controller also on the host side? What about target side controller.
4. Any specific fio workloads need to be added for basic testing?

Any other comments.

Regards,
-Chaitanya
  
From: Sagi Grimberg <sagi@grimberg.me>
Sent: Thursday, October 19, 2017 3:01:24 AM
To: Chaitanya Kulkarni; Johannes Thumshirn
Cc: Verkamp, Daniel; linux-nvme at lists.infradead.org; Hannes Reinecke; Christoph Hellwig
Subject: Re: NVMe target log page offset not supported
?   
> Hi Johannes,
> 
> I was able to incorporate all the comments received on the first version and added some new testcases.
> 
> Please have a look at the following link:-
> 
> https://github.com/ChaitanayaKulkarni/nvmftests.

 https://avatars3.githubusercontent.com/u/24584550?s=400&v=4 

ChaitanayaKulkarni/nvmftests
github.com
Contribute to nvmftests development by creating an account on GitHub.

> 
> Following is the list of the changes since V1 :-
> 
>????? 1. Addition of new testcases to improve the coverage.
>????? 2. Improved directory structure for code clarity.
>????? 3. Libification of the common code.
>????? 4. Support to use different block devices on the target side.
>????? 5. Centralize test configuration management.
>????? 6. Support to execute File System related operations on the host side.
>????? 7. Adjust the code to incorporate the comments received for the
>???????? first version.
>????? 8. Support for setting different log levels for each component.
>????? 9. Support for quick performance measurements.
>????? 10. Class hierarchy documentation and sequence diagram.
>????? 11. Improved workqueue management for host namesapce to
>????????? run parallel commands.

Hi Chaitanya,

Would it be possible to post patches so we can effectively provide
feedback?

One thing by a 5 seconds browse, its a shame that you work directly
on top of configfs and not import nvme plugin from nvmetcli repo...