| Message ID | 20230628190649.11233-2-nmi@metaspace.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk: add zoned storage support | expand |
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)
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
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 >
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/
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
> From: Andreas Hindborg <a.hindborg@samsung.com > <mailto:a.hindborg@samsung.com>> > > > This change is in preparation for zoned storage support. > > > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com > <mailto: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, Curious to know if there is a reason to miss enum 14 here ? And if UBLK_IO_OP_ZONE_APPEND is defined along with other operations better to define that in patch 3 itself. > + 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) Regards, Aravind
aravind.ramesh@opensource.wdc.com writes: >> From: Andreas Hindborg <a.hindborg@samsung.com >> <mailto:a.hindborg@samsung.com>> >> This change is in preparation for zoned storage support. >> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com >> <mailto: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, > > Curious to know if there is a reason to miss enum 14 here ? > And if UBLK_IO_OP_ZONE_APPEND is defined along with other operations > better to define that in patch 3 itself. They are defined after REQ_OP_ZONE_*, and they have a hole at 14
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)