ublk: add zoned storage support

[PATCH v4 1/4] ublk: change ublk IO command defines to enum

Posted by Andreas Hindborg 2 years, 7 months ago

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

This change is in preparation for zoned storage support.

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

diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index 4b8558db90e1..471b3b983045 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -229,12 +229,23 @@ struct ublksrv_ctrl_dev_info {
 	__u64   reserved2;
 };
 
-#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
+enum ublk_op {
+	UBLK_IO_OP_READ = 0,
+	UBLK_IO_OP_WRITE = 1,
+	UBLK_IO_OP_FLUSH = 2,
+	UBLK_IO_OP_DISCARD = 3,
+	UBLK_IO_OP_WRITE_SAME = 4,
+	UBLK_IO_OP_WRITE_ZEROES = 5,
+	UBLK_IO_OP_ZONE_OPEN = 10,
+	UBLK_IO_OP_ZONE_CLOSE = 11,
+	UBLK_IO_OP_ZONE_FINISH = 12,
+	UBLK_IO_OP_ZONE_APPEND = 13,
+	UBLK_IO_OP_ZONE_RESET = 15,
+	__UBLK_IO_OP_DRV_IN_START = 32,
+	__UBLK_IO_OP_DRV_IN_END = 96,
+	__UBLK_IO_OP_DRV_OUT_START = __UBLK_IO_OP_DRV_IN_END,
+	__UBLK_IO_OP_DRV_OUT_END = 160,
+};
 
 #define		UBLK_IO_F_FAILFAST_DEV		(1U << 8)
 #define		UBLK_IO_F_FAILFAST_TRANSPORT	(1U << 9)
-- 
2.41.0

Re: [PATCH v4 1/4] ublk: change ublk IO command defines to enum

Posted by Damien Le Moal 2 years, 7 months ago

On 6/29/23 04:06, Andreas Hindborg wrote:
> From: Andreas Hindborg <a.hindborg@samsung.com>
> 
> This change is in preparation for zoned storage support.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> ---
>  include/uapi/linux/ublk_cmd.h | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index 4b8558db90e1..471b3b983045 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -229,12 +229,23 @@ struct ublksrv_ctrl_dev_info {
>  	__u64   reserved2;
>  };
>  
> -#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
> +enum ublk_op {
> +	UBLK_IO_OP_READ = 0,
> +	UBLK_IO_OP_WRITE = 1,
> +	UBLK_IO_OP_FLUSH = 2,
> +	UBLK_IO_OP_DISCARD = 3,
> +	UBLK_IO_OP_WRITE_SAME = 4,
> +	UBLK_IO_OP_WRITE_ZEROES = 5,
> +	UBLK_IO_OP_ZONE_OPEN = 10,
> +	UBLK_IO_OP_ZONE_CLOSE = 11,
> +	UBLK_IO_OP_ZONE_FINISH = 12,
> +	UBLK_IO_OP_ZONE_APPEND = 13,
> +	UBLK_IO_OP_ZONE_RESET = 15,
> +	__UBLK_IO_OP_DRV_IN_START = 32,
> +	__UBLK_IO_OP_DRV_IN_END = 96,
> +	__UBLK_IO_OP_DRV_OUT_START = __UBLK_IO_OP_DRV_IN_END,
> +	__UBLK_IO_OP_DRV_OUT_END = 160,
> +};

This patch does not do what the title says. You are also introducing the zone
operations, and the very obscure __UBLK_IO_OP_DRV_XXX operations without an
explanation. Also, why the "__" prefix for these ? I do not see the point...
Given that this is a uapi, a comment to explain the less obvious commands would
be nice.

So I think the change to an enum for the existing ops can be done either in
patch 2 or as a separate patch and the introduction of the zone operations done
in patch 3 or as a separate patch.

>  
>  #define		UBLK_IO_F_FAILFAST_DEV		(1U << 8)
>  #define		UBLK_IO_F_FAILFAST_TRANSPORT	(1U << 9)

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 1/4] ublk: change ublk IO command defines to enum

Posted by Ming Lei 2 years, 7 months ago

On Thu, Jun 29, 2023 at 07:47:47AM +0900, Damien Le Moal wrote:
> On 6/29/23 04:06, Andreas Hindborg wrote:
> > From: Andreas Hindborg <a.hindborg@samsung.com>
> > 
> > This change is in preparation for zoned storage support.
> > 
> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> > ---
> >  include/uapi/linux/ublk_cmd.h | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > index 4b8558db90e1..471b3b983045 100644
> > --- a/include/uapi/linux/ublk_cmd.h
> > +++ b/include/uapi/linux/ublk_cmd.h
> > @@ -229,12 +229,23 @@ struct ublksrv_ctrl_dev_info {
> >  	__u64   reserved2;
> >  };
> >  
> > -#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
> > +enum ublk_op {
> > +	UBLK_IO_OP_READ = 0,
> > +	UBLK_IO_OP_WRITE = 1,
> > +	UBLK_IO_OP_FLUSH = 2,
> > +	UBLK_IO_OP_DISCARD = 3,
> > +	UBLK_IO_OP_WRITE_SAME = 4,
> > +	UBLK_IO_OP_WRITE_ZEROES = 5,
> > +	UBLK_IO_OP_ZONE_OPEN = 10,
> > +	UBLK_IO_OP_ZONE_CLOSE = 11,
> > +	UBLK_IO_OP_ZONE_FINISH = 12,
> > +	UBLK_IO_OP_ZONE_APPEND = 13,
> > +	UBLK_IO_OP_ZONE_RESET = 15,
> > +	__UBLK_IO_OP_DRV_IN_START = 32,
> > +	__UBLK_IO_OP_DRV_IN_END = 96,
> > +	__UBLK_IO_OP_DRV_OUT_START = __UBLK_IO_OP_DRV_IN_END,
> > +	__UBLK_IO_OP_DRV_OUT_END = 160,
> > +};
> 
> This patch does not do what the title says. You are also introducing the zone
> operations, and the very obscure __UBLK_IO_OP_DRV_XXX operations without an
> explanation. Also, why the "__" prefix for these ? I do not see the point...

It should be to reserve space for ublk passthrough OP.

> Given that this is a uapi, a comment to explain the less obvious commands would
> be nice.
> 
> So I think the change to an enum for the existing ops can be done either in
> patch 2 or as a separate patch and the introduction of the zone operations done
> in patch 3 or as a separate patch.

Also it might break userspace by changing to enum from macro for existed
definition, cause userspace may check something by '#ifdef UBLK_IO_OP_*',
so probably it is better to keep these OPs as enum, or at least keep
existed definition as macro.

Thanks,
Ming

Re: [PATCH v4 1/4] ublk: change ublk IO command defines to enum

Posted by Damien Le Moal 2 years, 7 months ago

On 6/29/23 09:38, Ming Lei wrote:
> On Thu, Jun 29, 2023 at 07:47:47AM +0900, Damien Le Moal wrote:
>> On 6/29/23 04:06, Andreas Hindborg wrote:
>>> From: Andreas Hindborg <a.hindborg@samsung.com>
>>>
>>> This change is in preparation for zoned storage support.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>>> ---
>>>  include/uapi/linux/ublk_cmd.h | 23 +++++++++++++++++------
>>>  1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>>> index 4b8558db90e1..471b3b983045 100644
>>> --- a/include/uapi/linux/ublk_cmd.h
>>> +++ b/include/uapi/linux/ublk_cmd.h
>>> @@ -229,12 +229,23 @@ struct ublksrv_ctrl_dev_info {
>>>  	__u64   reserved2;
>>>  };
>>>  
>>> -#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
>>> +enum ublk_op {
>>> +	UBLK_IO_OP_READ = 0,
>>> +	UBLK_IO_OP_WRITE = 1,
>>> +	UBLK_IO_OP_FLUSH = 2,
>>> +	UBLK_IO_OP_DISCARD = 3,
>>> +	UBLK_IO_OP_WRITE_SAME = 4,
>>> +	UBLK_IO_OP_WRITE_ZEROES = 5,
>>> +	UBLK_IO_OP_ZONE_OPEN = 10,
>>> +	UBLK_IO_OP_ZONE_CLOSE = 11,
>>> +	UBLK_IO_OP_ZONE_FINISH = 12,
>>> +	UBLK_IO_OP_ZONE_APPEND = 13,
>>> +	UBLK_IO_OP_ZONE_RESET = 15,
>>> +	__UBLK_IO_OP_DRV_IN_START = 32,
>>> +	__UBLK_IO_OP_DRV_IN_END = 96,
>>> +	__UBLK_IO_OP_DRV_OUT_START = __UBLK_IO_OP_DRV_IN_END,
>>> +	__UBLK_IO_OP_DRV_OUT_END = 160,
>>> +};
>>
>> This patch does not do what the title says. You are also introducing the zone
>> operations, and the very obscure __UBLK_IO_OP_DRV_XXX operations without an
>> explanation. Also, why the "__" prefix for these ? I do not see the point...
> 
> It should be to reserve space for ublk passthrough OP.

A comment about that would be nice.

> 
>> Given that this is a uapi, a comment to explain the less obvious commands would
>> be nice.
>>
>> So I think the change to an enum for the existing ops can be done either in
>> patch 2 or as a separate patch and the introduction of the zone operations done
>> in patch 3 or as a separate patch.
> 
> Also it might break userspace by changing to enum from macro for existed
> definition, cause userspace may check something by '#ifdef UBLK_IO_OP_*',
> so probably it is better to keep these OPs as enum, or at least keep
> existed definition as macro.

Then let's keep defining things with #define instead of an enum.

> 
> Thanks,
> Ming
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 1/4] ublk: change ublk IO command defines to enum

Posted by Andreas Hindborg (Samsung) 2 years, 7 months ago

Ming Lei <ming.lei@redhat.com> writes:

> On Thu, Jun 29, 2023 at 07:47:47AM +0900, Damien Le Moal wrote:
>> On 6/29/23 04:06, Andreas Hindborg wrote:
>> > From: Andreas Hindborg <a.hindborg@samsung.com>
>> > 
>> > This change is in preparation for zoned storage support.
>> > 
>> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>> > ---
>> >  include/uapi/linux/ublk_cmd.h | 23 +++++++++++++++++------
>> >  1 file changed, 17 insertions(+), 6 deletions(-)
>> > 
>> > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>> > index 4b8558db90e1..471b3b983045 100644
>> > --- a/include/uapi/linux/ublk_cmd.h
>> > +++ b/include/uapi/linux/ublk_cmd.h
>> > @@ -229,12 +229,23 @@ struct ublksrv_ctrl_dev_info {
>> >  	__u64   reserved2;
>> >  };
>> >  
>> > -#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
>> > +enum ublk_op {
>> > +	UBLK_IO_OP_READ = 0,
>> > +	UBLK_IO_OP_WRITE = 1,
>> > +	UBLK_IO_OP_FLUSH = 2,
>> > +	UBLK_IO_OP_DISCARD = 3,
>> > +	UBLK_IO_OP_WRITE_SAME = 4,
>> > +	UBLK_IO_OP_WRITE_ZEROES = 5,
>> > +	UBLK_IO_OP_ZONE_OPEN = 10,
>> > +	UBLK_IO_OP_ZONE_CLOSE = 11,
>> > +	UBLK_IO_OP_ZONE_FINISH = 12,
>> > +	UBLK_IO_OP_ZONE_APPEND = 13,
>> > +	UBLK_IO_OP_ZONE_RESET = 15,
>> > +	__UBLK_IO_OP_DRV_IN_START = 32,
>> > +	__UBLK_IO_OP_DRV_IN_END = 96,
>> > +	__UBLK_IO_OP_DRV_OUT_START = __UBLK_IO_OP_DRV_IN_END,
>> > +	__UBLK_IO_OP_DRV_OUT_END = 160,
>> > +};
>> 
>> This patch does not do what the title says. You are also introducing the zone
>> operations, and the very obscure __UBLK_IO_OP_DRV_XXX operations without an
>> explanation. Also, why the "__" prefix for these ? I do not see the point...
>
> It should be to reserve space for ublk passthrough OP.
>
>> Given that this is a uapi, a comment to explain the less obvious commands would
>> be nice.
>> 
>> So I think the change to an enum for the existing ops can be done either in
>> patch 2 or as a separate patch and the introduction of the zone operations done
>> in patch 3 or as a separate patch.
>
> Also it might break userspace by changing to enum from macro for existed
> definition, cause userspace may check something by '#ifdef UBLK_IO_OP_*',
> so probably it is better to keep these OPs as enum, or at least keep
> existed definition as macro.

I can change it back to `#define` again, no problem. I only changed it
to `enum` on request from Ming [1]

Best regards,
Andreas

[1] https://lore.kernel.org/all/ZAHeWieKXtgYUbvz@ovpn-8-18.pek2.redhat.com/

Re: [PATCH v4 1/4] ublk: change ublk IO command defines to enum

Posted by Andreas Hindborg (Samsung) 2 years, 7 months ago

Damien Le Moal <dlemoal@kernel.org> writes:

> On 6/29/23 04:06, Andreas Hindborg wrote:
>> From: Andreas Hindborg <a.hindborg@samsung.com>
>> 
>> This change is in preparation for zoned storage support.
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>> ---
>>  include/uapi/linux/ublk_cmd.h | 23 +++++++++++++++++------
>>  1 file changed, 17 insertions(+), 6 deletions(-)
>> 
>> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>> index 4b8558db90e1..471b3b983045 100644
>> --- a/include/uapi/linux/ublk_cmd.h
>> +++ b/include/uapi/linux/ublk_cmd.h
>> @@ -229,12 +229,23 @@ struct ublksrv_ctrl_dev_info {
>>  	__u64   reserved2;
>>  };
>>  
>> -#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
>> +enum ublk_op {
>> +	UBLK_IO_OP_READ = 0,
>> +	UBLK_IO_OP_WRITE = 1,
>> +	UBLK_IO_OP_FLUSH = 2,
>> +	UBLK_IO_OP_DISCARD = 3,
>> +	UBLK_IO_OP_WRITE_SAME = 4,
>> +	UBLK_IO_OP_WRITE_ZEROES = 5,
>> +	UBLK_IO_OP_ZONE_OPEN = 10,
>> +	UBLK_IO_OP_ZONE_CLOSE = 11,
>> +	UBLK_IO_OP_ZONE_FINISH = 12,
>> +	UBLK_IO_OP_ZONE_APPEND = 13,
>> +	UBLK_IO_OP_ZONE_RESET = 15,
>> +	__UBLK_IO_OP_DRV_IN_START = 32,
>> +	__UBLK_IO_OP_DRV_IN_END = 96,
>> +	__UBLK_IO_OP_DRV_OUT_START = __UBLK_IO_OP_DRV_IN_END,
>> +	__UBLK_IO_OP_DRV_OUT_END = 160,
>> +};
>
> This patch does not do what the title says. You are also introducing the zone
> operations, and the very obscure __UBLK_IO_OP_DRV_XXX operations without an
> explanation. Also, why the "__" prefix for these ? I do not see the point...
> Given that this is a uapi, a comment to explain the less obvious commands would
> be nice.

It is a little vague, I'll make sure to include a better description 👍

>
> So I think the change to an enum for the existing ops can be done either in
> patch 2 or as a separate patch and the introduction of the zone operations done
> in patch 3 or as a separate patch.

I agree, the zone ops should not be introduced in this patch, I will
move them to patch 3. That is a mistake.

Best regards,
Andreas