[v6,1/3] ublk: add opcode offsets for DRV_IN/DRV_OUT

  From: Andreas Hindborg <a.hindborg@samsung.com>

Ublk zoned storage support relies on DRV_IN handling for zone report.
Prepare for this change by adding offsets for the DRV_IN/DRV_OUT commands.

Also add parenthesis to existing opcodes for better macro hygiene.

Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
---
 include/uapi/linux/ublk_cmd.h | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
  

On 7/6/23 22:09, Andreas Hindborg wrote:
> From: Andreas Hindborg <a.hindborg@samsung.com>
> 
> Ublk zoned storage support relies on DRV_IN handling for zone report.
> Prepare for this change by adding offsets for the DRV_IN/DRV_OUT commands.
> 
> Also add parenthesis to existing opcodes for better macro hygiene.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> ---
>  include/uapi/linux/ublk_cmd.h | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index 4b8558db90e1..2ebb8d5d827a 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -229,12 +229,22 @@ struct ublksrv_ctrl_dev_info {
>  	__u64   reserved2;
>  };
>  
> -#define		UBLK_IO_OP_READ		0
> +#define		UBLK_IO_OP_READ			0
>  #define		UBLK_IO_OP_WRITE		1
>  #define		UBLK_IO_OP_FLUSH		2
> -#define		UBLK_IO_OP_DISCARD	3
> -#define		UBLK_IO_OP_WRITE_SAME	4
> -#define		UBLK_IO_OP_WRITE_ZEROES	5
> +#define		UBLK_IO_OP_DISCARD		3
> +#define		UBLK_IO_OP_WRITE_SAME		4
> +#define		UBLK_IO_OP_WRITE_ZEROES		5
> +/*
> + * Passthrough (driver private) operation codes range between

This is unclear... Here, what does "driver" refer to ? If it is the ublk
kernel driver, than these commands should not be defined in the uapi
header file, they should be defined in drivers/block/ublk.h. However, if
these are for the user space driver, like all the other operations, then
let's clearly state so. But then, I still not understand why these need
a different naming pattern using the "__UBLK" prefix...

> + * __UBLK_IO_OP_DRV_IN_START and __UBLK_IO_OP_DRV_IN_END for REQ_OP_DRV_IN type
> + * operations and between __UBLK_IO_OP_DRV_OUT_START and
> + * __UBLK_IO_OP_DRV_OUT_END for REQ_OP_DRV_OUT type operations.
> + */
> +#define		__UBLK_IO_OP_DRV_IN_START	32
> +#define		__UBLK_IO_OP_DRV_IN_END		96
> +#define		__UBLK_IO_OP_DRV_OUT_START	__UBLK_IO_OP_DRV_IN_END
> +#define		__UBLK_IO_OP_DRV_OUT_END	160
>  
>  #define		UBLK_IO_F_FAILFAST_DEV		(1U << 8)
>  #define		UBLK_IO_F_FAILFAST_TRANSPORT	(1U << 9)
  

On Fri, Jul 07, 2023 at 08:50:01AM +0900, Damien Le Moal wrote:
> On 7/6/23 22:09, Andreas Hindborg wrote:
> > From: Andreas Hindborg <a.hindborg@samsung.com>
> > 
> > Ublk zoned storage support relies on DRV_IN handling for zone report.
> > Prepare for this change by adding offsets for the DRV_IN/DRV_OUT commands.
> > 
> > Also add parenthesis to existing opcodes for better macro hygiene.
> > 
> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> > ---
> >  include/uapi/linux/ublk_cmd.h | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > index 4b8558db90e1..2ebb8d5d827a 100644
> > --- a/include/uapi/linux/ublk_cmd.h
> > +++ b/include/uapi/linux/ublk_cmd.h
> > @@ -229,12 +229,22 @@ struct ublksrv_ctrl_dev_info {
> >  	__u64   reserved2;
> >  };
> >  
> > -#define		UBLK_IO_OP_READ		0
> > +#define		UBLK_IO_OP_READ			0
> >  #define		UBLK_IO_OP_WRITE		1
> >  #define		UBLK_IO_OP_FLUSH		2
> > -#define		UBLK_IO_OP_DISCARD	3
> > -#define		UBLK_IO_OP_WRITE_SAME	4
> > -#define		UBLK_IO_OP_WRITE_ZEROES	5
> > +#define		UBLK_IO_OP_DISCARD		3
> > +#define		UBLK_IO_OP_WRITE_SAME		4
> > +#define		UBLK_IO_OP_WRITE_ZEROES		5
> > +/*
> > + * Passthrough (driver private) operation codes range between
> 
> This is unclear... Here, what does "driver" refer to ? If it is the ublk
> kernel driver, than these commands should not be defined in the uapi
> header file, they should be defined in drivers/block/ublk.h. However, if
> these are for the user space driver, like all the other operations, then

Like normal IO, these passthrough requests needs userspace to handle too,
usually they just belong to specific ublk target, such as report zones.,
so here it is part of UAPI.

But yes, we should document it clearly, maybe something below?

	Ublk passthrough operation code ranges, and each passthrough
	operation provides generic interface between ublk kernel driver
	and ublk userspace, and this interface is usually used for handling
	generic block layer request, such as command of zoned report zones.
	Passthrough operation is only needed iff ublk kernel driver has to
	be involved for handling this operation.

> let's clearly state so. But then, I still not understand why these need
> a different naming pattern using the "__UBLK" prefix...

I think __UBLK just meant we don't suggest userspace to use it directly,
since the added macros are just for making ranges for DRV_IN and DRV_OUT,
so we can check command direction easily be using this start/end info in
both sides.


Thanks, 
Ming
  

On 7/7/23 09:59, Ming Lei wrote:
> On Fri, Jul 07, 2023 at 08:50:01AM +0900, Damien Le Moal wrote:
>> On 7/6/23 22:09, Andreas Hindborg wrote:
>>> From: Andreas Hindborg <a.hindborg@samsung.com>
>>>
>>> Ublk zoned storage support relies on DRV_IN handling for zone report.
>>> Prepare for this change by adding offsets for the DRV_IN/DRV_OUT commands.
>>>
>>> Also add parenthesis to existing opcodes for better macro hygiene.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>>> ---
>>>  include/uapi/linux/ublk_cmd.h | 18 ++++++++++++++----
>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>>> index 4b8558db90e1..2ebb8d5d827a 100644
>>> --- a/include/uapi/linux/ublk_cmd.h
>>> +++ b/include/uapi/linux/ublk_cmd.h
>>> @@ -229,12 +229,22 @@ struct ublksrv_ctrl_dev_info {
>>>  	__u64   reserved2;
>>>  };
>>>  
>>> -#define		UBLK_IO_OP_READ		0
>>> +#define		UBLK_IO_OP_READ			0
>>>  #define		UBLK_IO_OP_WRITE		1
>>>  #define		UBLK_IO_OP_FLUSH		2
>>> -#define		UBLK_IO_OP_DISCARD	3
>>> -#define		UBLK_IO_OP_WRITE_SAME	4
>>> -#define		UBLK_IO_OP_WRITE_ZEROES	5
>>> +#define		UBLK_IO_OP_DISCARD		3
>>> +#define		UBLK_IO_OP_WRITE_SAME		4
>>> +#define		UBLK_IO_OP_WRITE_ZEROES		5
>>> +/*
>>> + * Passthrough (driver private) operation codes range between
>>
>> This is unclear... Here, what does "driver" refer to ? If it is the ublk
>> kernel driver, than these commands should not be defined in the uapi
>> header file, they should be defined in drivers/block/ublk.h. However, if
>> these are for the user space driver, like all the other operations, then
> 
> Like normal IO, these passthrough requests needs userspace to handle too,
> usually they just belong to specific ublk target, such as report zones.,
> so here it is part of UAPI.
> 
> But yes, we should document it clearly, maybe something below?
> 
> 	Ublk passthrough operation code ranges, and each passthrough
> 	operation provides generic interface between ublk kernel driver
> 	and ublk userspace, and this interface is usually used for handling
> 	generic block layer request, such as command of zoned report zones.
> 	Passthrough operation is only needed iff ublk kernel driver has to
> 	be involved for handling this operation.

Yes, that is better.

> 
>> let's clearly state so. But then, I still not understand why these need
>> a different naming pattern using the "__UBLK" prefix...
> 
> I think __UBLK just meant we don't suggest userspace to use it directly,
> since the added macros are just for making ranges for DRV_IN and DRV_OUT,
> so we can check command direction easily be using this start/end info in
> both sides.

Personally, I would still prefer to not add this "__" prefix as these
are operations that the ublk user driver will have to deal with, like
the other ones. So I do not see the point of that prefix. But no strong
feeling about that :)

> 
> 
> Thanks, 
> Ming
>
  

On Fri, Jul 07, 2023 at 08:59:03AM +0800, Ming Lei wrote:
> > let's clearly state so. But then, I still not understand why these need
> > a different naming pattern using the "__UBLK" prefix...
> 
> I think __UBLK just meant we don't suggest userspace to use it directly,
> since the added macros are just for making ranges for DRV_IN and DRV_OUT,
> so we can check command direction easily be using this start/end info in
> both sides.

Folks, please stop coupling a uapi (or on-disk protocol) too tightly
to Linux internals.  Think of what makes sense as a communication
protocol, not what is an internal kernel interface.

REPORT_ZONES is a sensible command, and supported in ATA/SCSI/NVMe in
one way or another.  In Linux it is a synchronous method call right now
for one reason or another, and most implementation map it to a
passthrough command - be that the actual protocol command or something
internal for virtio.

So for ublk this is just another command like any other, that needs to
be defined and documented.  Nothing internal or driver specific.
  

On Sun, Jul 09, 2023 at 11:52:39PM -0700, Christoph Hellwig wrote:
> On Fri, Jul 07, 2023 at 08:59:03AM +0800, Ming Lei wrote:
> > > let's clearly state so. But then, I still not understand why these need
> > > a different naming pattern using the "__UBLK" prefix...
> > 
> > I think __UBLK just meant we don't suggest userspace to use it directly,
> > since the added macros are just for making ranges for DRV_IN and DRV_OUT,
> > so we can check command direction easily be using this start/end info in
> > both sides.
> 
> Folks, please stop coupling a uapi (or on-disk protocol) too tightly
> to Linux internals.  Think of what makes sense as a communication
> protocol, not what is an internal kernel interface.
> 
> REPORT_ZONES is a sensible command, and supported in ATA/SCSI/NVMe in
> one way or another.  In Linux it is a synchronous method call right now
> for one reason or another, and most implementation map it to a
> passthrough command - be that the actual protocol command or something
> internal for virtio.
> 
> So for ublk this is just another command like any other, that needs to
> be defined and documented.  Nothing internal or driver specific.
 
Yes, that is exactly what we are doing.

The added macros of UBLK_IO_OP_DRV_IN_START[END] are just for supporting
more ublk passthrough commands, and the motivation is for running
check(such as buffer direction) in two sides easily.

However, I think it is just fine to delay to add it until introducing
the 2nd ublk pt command.

Thanks, 
Ming
  

On Mon, Jul 10, 2023 at 05:27:23PM +0800, Ming Lei wrote:
> Yes, that is exactly what we are doing.
> 
> The added macros of UBLK_IO_OP_DRV_IN_START[END] are just for supporting
> more ublk passthrough commands, and the motivation is for running
> check(such as buffer direction) in two sides easily.
> 
> However, I think it is just fine to delay to add it until introducing
> the 2nd ublk pt command.

The concept of a passthrough command just doesn't make sense for an
on the wire protocol.  It is a linux concept that distinguished between
the Linux synthetic command like REQ_OP_READ/WRITE/DISCARD etc that are
well defined and can be used by file systems and other consumers, and
ways to pass through arbitrary blobs only known by the driver.

Anything in a wire protocol needs to be very well defined in that
protocol completely indpendent of what Linux concept it maps to.
Especially as the Linux concepts can change, and fairly frequently do.
  

On Mon, Jul 10, 2023 at 02:32:44AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 10, 2023 at 05:27:23PM +0800, Ming Lei wrote:
> > Yes, that is exactly what we are doing.
> > 
> > The added macros of UBLK_IO_OP_DRV_IN_START[END] are just for supporting
> > more ublk passthrough commands, and the motivation is for running
> > check(such as buffer direction) in two sides easily.
> > 
> > However, I think it is just fine to delay to add it until introducing
> > the 2nd ublk pt command.
> 
> The concept of a passthrough command just doesn't make sense for an
> on the wire protocol.  It is a linux concept that distinguished between
> the Linux synthetic command like REQ_OP_READ/WRITE/DISCARD etc that are
> well defined and can be used by file systems and other consumers, and
> ways to pass through arbitrary blobs only known by the driver.
> 
> Anything in a wire protocol needs to be very well defined in that
> protocol completely indpendent of what Linux concept it maps to.
> Especially as the Linux concepts can change, and fairly frequently do.

Yeah, you are right wrt. linux pt command, and here we shouldn't use
the term of passthrough.

Actually the UAPI is trying to define interface between driver and
userspace, which is just like interface between driver and hardware, such
as, how nvme/sd_zbc is dealing with actual hardware wrt. reporting
zones.

Thanks,
Ming
  

Christoph Hellwig <hch@infradead.org> writes:

> On Mon, Jul 10, 2023 at 05:27:23PM +0800, Ming Lei wrote:
>> Yes, that is exactly what we are doing.
>> 
>> The added macros of UBLK_IO_OP_DRV_IN_START[END] are just for supporting
>> more ublk passthrough commands, and the motivation is for running
>> check(such as buffer direction) in two sides easily.
>> 
>> However, I think it is just fine to delay to add it until introducing
>> the 2nd ublk pt command.
>
> The concept of a passthrough command just doesn't make sense for an
> on the wire protocol.  It is a linux concept that distinguished between
> the Linux synthetic command like REQ_OP_READ/WRITE/DISCARD etc that are
> well defined and can be used by file systems and other consumers, and
> ways to pass through arbitrary blobs only known by the driver.

Yet most on-the-wire protocols for actual hardware does support this
some way or another. But I agree that for ublk it is probably not
needed. It would probably be easier to talk to the ublk daemon through
other means than passthrough in the block layer.

>
> Anything in a wire protocol needs to be very well defined in that
> protocol completely indpendent of what Linux concept it maps to.
> Especially as the Linux concepts can change, and fairly frequently do.

I somewhat agree in the sense that for consistency, we should either
move zone management commands to the DRV_OUT range OR move report_zones
out of this special range and just next to the zone management
operations. I like the latter option better, and I would love to see the
block layer do the same at some point. It feels backwards that
report_zones get special treatment all over the place.

Best regards,
Andreas
  

On Tue, Jul 11, 2023 at 08:23:40AM +0200, Andreas Hindborg (Samsung) wrote:
> Yet most on-the-wire protocols for actual hardware does support this
> some way or another.

Supports what?  Passthrough?  No.

> I somewhat agree in the sense that for consistency, we should either
> move zone management commands to the DRV_OUT range OR move report_zones
> out of this special range and just next to the zone management
> operations. I like the latter option better, and I would love to see the
> block layer do the same at some point. It feels backwards that
> report_zones get special treatment all over the place.

DRV_IN/OUT is purely a Linux concept.  It doesn't make any sense for
a wire protocol.
  

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Jul 11, 2023 at 08:23:40AM +0200, Andreas Hindborg (Samsung) wrote:
>> Yet most on-the-wire protocols for actual hardware does support this
>> some way or another.
>
> Supports what?  Passthrough?  No.

Both SCSI and NVMe has command identifier ranges reserved for vendor
specific commands. I would assume that one use of these is to implement
passthrough channels to a device for testing out new interfaces. Just
guessing though.

>
>> I somewhat agree in the sense that for consistency, we should either
>> move zone management commands to the DRV_OUT range OR move report_zones
>> out of this special range and just next to the zone management
>> operations. I like the latter option better, and I would love to see the
>> block layer do the same at some point. It feels backwards that
>> report_zones get special treatment all over the place.
>
> DRV_IN/OUT is purely a Linux concept.  It doesn't make any sense for
> a wire protocol.

Ok 👍

BR Andreas
  

On Tue, Jul 11, 2023 at 11:02:15AM +0200, Andreas Hindborg (Samsung) wrote:
> 
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Tue, Jul 11, 2023 at 08:23:40AM +0200, Andreas Hindborg (Samsung) wrote:
> >> Yet most on-the-wire protocols for actual hardware does support this
> >> some way or another.
> >
> > Supports what?  Passthrough?  No.
> 
> Both SCSI and NVMe has command identifier ranges reserved for vendor
> specific commands. I would assume that one use of these is to implement
> passthrough channels to a device for testing out new interfaces. Just
> guessing though.

Vendor specific commands is an entirely different concept from Linux
passthrough requests.
  

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Jul 11, 2023 at 11:02:15AM +0200, Andreas Hindborg (Samsung) wrote:
>> 
>> Christoph Hellwig <hch@infradead.org> writes:
>> 
>> > On Tue, Jul 11, 2023 at 08:23:40AM +0200, Andreas Hindborg (Samsung) wrote:
>> >> Yet most on-the-wire protocols for actual hardware does support this
>> >> some way or another.
>> >
>> > Supports what?  Passthrough?  No.
>> 
>> Both SCSI and NVMe has command identifier ranges reserved for vendor
>> specific commands. I would assume that one use of these is to implement
>> passthrough channels to a device for testing out new interfaces. Just
>> guessing though.
>
> Vendor specific commands is an entirely different concept from Linux
> passthrough requests.

And yet they are somewhat similar, in the sense that they allow the user
of a protocol to express semantics that is not captured in the
established protocol. Uring command passthrough -> request passthrough
-> vendor specific commands. They sort of map well in terms of what they
allow the user to achieve. Or did I misunderstand something completely?
  

On Tue, Jul 11, 2023 at 12:15:18PM +0200, Andreas Hindborg (Samsung) wrote:
> And yet they are somewhat similar, in the sense that they allow the user
> of a protocol to express semantics that is not captured in the
> established protocol. Uring command passthrough -> request passthrough
> -> vendor specific commands. They sort of map well in terms of what they
> allow the user to achieve. Or did I misunderstand something completely?

Well, there is a relationship, but it's one way.

Vendor specific command are basically always going to be used through
a passthrough interface, because they aren't standardized.

But most commands used through a passthrough interface are normal
standardized commands, just either used in a way not supported by
the normal Linux interface or just in creative ways.