[RFC] nvme: Fabrics should use the fctype field for data direction

[RFC] nvme: Fabrics should use the fctype field for data direction

[Jon Derrick]

Fabrics commands with opcode 0x7F use the fctype field to indicate data
direction.

Fixes: eb793e2c ("nvme.h: add NVMe over Fabrics definitions")

Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>
---
I don't have a setup available to test this, but according to the fabrics spec,
we should be using fctype. Any write commands would appear to work, including
Connect, but Property Get and other read commands with opcode 0x7F looks like
they should fail.

 include/linux/nvme.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 983975b..7246059 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1017,7 +1017,7 @@ static inline bool nvme_is_write(struct nvme_command *cmd)
 	 * Why can't we simply have a Fabrics In and Fabrics out command?
 	 */
 	if (unlikely(cmd->common.opcode == nvme_fabrics_command))
-		return cmd->fabrics.opcode & 1;
+		return cmd->fabrics.fctype & 1;
 	return cmd->common.opcode & 1;
 }
 
-- 
2.9.3

[RFC] nvme: Fabrics should use the fctype field for data direction

[Sagi Grimberg]

On 12/07/17 19:58, Jon Derrick wrote:
> Fabrics commands with opcode 0x7F use the fctype field to indicate data
> direction.
> 
> Fixes: eb793e2c ("nvme.h: add NVMe over Fabrics definitions")
> 
> Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>
> ---
> I don't have a setup available to test this,

nvme-loop (with null_blk/loopback/ramdisk) runs everywhere....

  but according to the fabrics spec,
> we should be using fctype. Any write commands would appear to work, including
> Connect, but Property Get and other read commands with opcode 0x7F looks like
> they should fail.
> 
>   include/linux/nvme.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 983975b..7246059 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1017,7 +1017,7 @@ static inline bool nvme_is_write(struct nvme_command *cmd)
>   	 * Why can't we simply have a Fabrics In and Fabrics out command?
>   	 */
>   	if (unlikely(cmd->common.opcode == nvme_fabrics_command))
> -		return cmd->fabrics.opcode & 1;
> +		return cmd->fabrics.fctype & 1;
>   	return cmd->common.opcode & 1;
>   }

Makes sense,

Reviewed-by: Sagi Grimberg <sai at grmberg.me>

Please test with nvme-loop though..

[RFC] nvme: Fabrics should use the fctype field for data direction

[Christoph Hellwig]

Interesting.  I wonder how things ever worked without this..

But the Fabrics multiplexer is a bidi command, so nvme_is_write
will always return true currently.

On the host side it only matters for the passthrough ioctls,
which no one uses for the fabrics commands.

On the target side this means we always try to copy data in and
never out, strange.

So this looks good to me, but it'll need a little test run.  As Sagi
said the loop driver is trivial to test, but one of us should also
run it though rdma testing.

[RFC] nvme: Fabrics should use the fctype field for data direction

[Sagi Grimberg]

> Interesting.  I wonder how things ever worked without this..
> 
> But the Fabrics multiplexer is a bidi command, so nvme_is_write
> will always return true currently.
> 
> On the host side it only matters for the passthrough ioctls,
> which no one uses for the fabrics commands.
> 
> On the target side this means we always try to copy data in and
> never out, strange.

There is no data-out in any of the fabrics commands.

[RFC] nvme: Fabrics should use the fctype field for data direction

[Christoph Hellwig]

Thanks, applied to nvme-4.13.