WRITE SAME (16) differences between SBC-3 vs SBC-4

[jpocas]

In SBC-4, a new bit is added to WRITE SAME (16) called NDOB. It's used to indicate whether the data in the data-out-buffer should be used when the UNMAP bit is set. It's only valid when UNMAP==1 and is a check condition if NDOB and not UNMAP. If NDOB==1, this means that there doesn't need to be any data supplied with the WRITE SAME (16) command and that the target will proceed as if the data-out-buffer had a block of all zeroes.

The test_writesame16_unmap tests appear to be using the SBC-3 behavior where there is no knowledge of the NDOB flag, and UNMAP==1 implies that there doesn't have to be any data in the data-out-buffer. It's expecting that and the target will write it's own buffer of zeroes.

This is not necessarily a bug, but will cause test failures when talking to an SBC-4 compliant target since it will see that there is insufficient data in the data-out-buffer in these WRITE SAME (16) w/ UNMAP tests. I'm not sure what the correct solution is.

[sahlberg]

Interesting.

I need to take a look at this new draft. Is there any vpd that desctibes if this NDOB flag is set or not? We definitely want to add tests for this. (and I should add support for this to TGTD so I have something to test against)

On Mon, May 5, 2014 at 12:11 PM, jpocas [email protected] wrote:

In SBC-4, a new bit is added to WRITE SAME (16) called NDOB. It's used to indicate whether the data in the data-out-buffer should be used when the UNMAP bit is set. It's only valid when UNMAP==1 and is a check condition if NDOB and not UNMAP. If NDOB==1, this means that there doesn't need to be any data supplied with the WRITE SAME (16) command and that the target will proceed as if the data-out-buffer had a block of all zeroes.

The test_writesame16_unmap tests appear to be using the SBC-3 behavior where there is no knowledge of the NDOB flag, and UNMAP==1 implies that there doesn't have to be any data in the data-out-buffer. It's expecting that and the target will write it's own buffer of zeroes.

This is not necessarily a bug, but will cause test failures when talking to an SBC-4 compliant target since it will see that there is insufficient data in the data-out-buffer in these WRITE SAME (16) w/ UNMAP tests. I'm not sure what the correct solution is.

— Reply to this email directly or view it on GitHubhttps://github.com/sahlberg/libiscsi/issues/110 .

[sahlberg]

After initial glance,

Some tests we can add: NDOB==1 and UNMAP==0 should always result in a check condition in both SBC3 and SBC4.

IF the target claims SBC4 support in the version descriptors for INQUIRY we can verify that NDBO==1 and UNMAP==1 will be successful.

IF the target claims explicit SBC3 support then we can assume it does not support SBC4 so we can verify that NDOB==1 and UNMAP==1 will fail.

If SBC version is unknown we can probe by sending a valid cdb with NDOB==1 and UNMAP==1. IF we get a check condition it means the target does not support NDOB and we can skip the rest of the tests. IF the command does not fail with a check condition we can proceed to do additional tests that the target implements NDOB correctly.

On Tue, May 6, 2014 at 9:17 AM, ronnie sahlberg [email protected]:

Interesting.

I need to take a look at this new draft. Is there any vpd that desctibes if this NDOB flag is set or not? We definitely want to add tests for this. (and I should add support for this to TGTD so I have something to test against)

On Mon, May 5, 2014 at 12:11 PM, jpocas [email protected] wrote:

In SBC-4, a new bit is added to WRITE SAME (16) called NDOB. It's used to indicate whether the data in the data-out-buffer should be used when the UNMAP bit is set. It's only valid when UNMAP==1 and is a check condition if NDOB and not UNMAP. If NDOB==1, this means that there doesn't need to be any data supplied with the WRITE SAME (16) command and that the target will proceed as if the data-out-buffer had a block of all zeroes.

The test_writesame16_unmap tests appear to be using the SBC-3 behavior where there is no knowledge of the NDOB flag, and UNMAP==1 implies that there doesn't have to be any data in the data-out-buffer. It's expecting that and the target will write it's own buffer of zeroes.

This is not necessarily a bug, but will cause test failures when talking to an SBC-4 compliant target since it will see that there is insufficient data in the data-out-buffer in these WRITE SAME (16) w/ UNMAP tests. I'm not sure what the correct solution is.

— Reply to this email directly or view it on GitHubhttps://github.com/sahlberg/libiscsi/issues/110 .

[jpocas]

I don't mind taking a crack at your suggestion since I brought up the issue, but I'd like to wait until your original repo gets synced up with my recent pull requests WRT writesame16. This issue is not urgent though, because SBC-4 is still in development and I was using a very recent draft.

[sahlberg]

Sorry. I completely forgot since I have been so busy. You should have reminded me earlier.

I have merged your patches. Thanks!

There were two small issues, With git, the convention is to format the commit messages so that the first line is <80 characters and contains a brief description of the patch. Then you have a blank line followed by, optionally, a longer description of the patch. I dont enforce this in libiscsi but other projects often are quite strict with this, and there are patching tools that expect a certain format of the commit message.

I also changed the tests to use alloca instead of malloc. It makes error paths simpler by not having to worry about when to free the block.

Thanks!

regards ronnie sahlberg

On Mon, May 19, 2014 at 8:28 AM, jpocas [email protected] wrote:

I don't mind taking a crack at your suggestion since I brought up the issue, but I'd like to wait until your original repo gets synced up with my recent pull requests WRT writesame16. This issue is not urgent though, because SBC-4 is still in development and I was using a very recent draft.

— Reply to this email directly or view it on GitHubhttps://github.com/sahlberg/libiscsi/issues/110#issuecomment-43518646 .

[jpocas]

Thanks for the tips Ronnie,

I am happy to comply with the standard checkin message conventions. In fact on another note I noticed that you are using hard TAB indentations and my editor (emacs) was setup to use 4 spaces instead and never automatically emit hard TABs. I know indentation spacing and style is a common sticking point (of quasi-religious proportions) between developers. I was careful and tried not to disrespect the original indentation and I hope my emacs config and auto-indentation didn't betray me. I will have to come up with an alternate config when I am editing your project so that I don't inadvertently introduce inconsistent indentation styles.

About the alloca(), I noticed that used in other places. It does make cleanup easier. On my linux dev environment (ubuntu 14.04), the stack size is a whopping 8MiB which is fine for just about everything I've ever used it for, but when using alloca() in place of malloc() it is easy to go over this. So caveat emptor to anyone reading this who is writing code with large buffers or recursive calls and using alloca().

I will take a stab at this issue when I get some time and send a pull request once I have it working.

Thanks. -Jamie

[sahlberg]

A friendly ping. Any updates Or do you want me to add such tests?

[bonzini]

an old issue, but FWIW the NDOB bit can be set even if UNMAP=0.