On Thu, Aug 10, 2023 at 02:22:50PM +0000, Niklas Cassel wrote:

On Thu, Aug 10, 2023 at 10:00:14PM +0800, Ming Lei wrote:

quoted

On Thu, Aug 10, 2023 at 01:10:30PM +0000, Niklas Cassel wrote:

quoted

On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:

(snip)
 

quoted

UBLK_IO_OP_ZONE_* is part of ublk UAPI, but REQ_OP_ZONE_* is just kernel
internal definition which may be changed time by time, so we can't use
REQ_OP_ZONE_* directly.

Here you can think of UBLK_IO_OP_ZONE_* as interface between driver and
hardware, so UBLK_IO_OP_ZONE_* has to be defined independently.

quoted

but if you want to keep this pattern, then perhaps you want
to define UBLK_IO_OP_ZONE_RESET_ALL to 17.

Why do you think that 17 is better than 14?

I never said that it was better :)
I even said: "I don't see any obvious advantage of keeping them the same" :)

OK, maybe I misunderstood your point.

Just that it would follow the existing pattern of keeping
UBLK_IO_OP_ZONE_* in sync with REQ_OP_ZONE_*.

No matter 14 or 17, it does sync with REQ_OP_ZONE_*, even though it
isn't necessary to do so.

Then your all comment on this patch should be addressed, right?

quoted

I'd rather use 14 to fill the hole, meantime the two ZONE_RESET OPs
can be kept together.

Ok, but then, considering that UBLK_IO_OP_ZONE_* is not part of any official
kernel release, and that the highest UBLK_IO_OP is currently defined as 5:

I don't think it is necessary, see below.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/ublk_cmd.h?h=v6.5-rc5#n237

why not define:
+#define		UBLK_IO_OP_ZONE_OPEN		6
+#define		UBLK_IO_OP_ZONE_CLOSE		7
+#define		UBLK_IO_OP_ZONE_FINISH		8
+#define		UBLK_IO_OP_ZONE_APPEND		9
+#define		UBLK_IO_OP_ZONE_RESET		10
+#define		UBLK_IO_OP_ZONE_RESET_ALL	11

instead of, like it currently is in linux-block/for-next (this patch included):

+#define		UBLK_IO_OP_ZONE_OPEN		10
+#define		UBLK_IO_OP_ZONE_CLOSE		11
+#define		UBLK_IO_OP_ZONE_FINISH		12
+#define		UBLK_IO_OP_ZONE_APPEND		13
+#define		UBLK_IO_OP_ZONE_RESET_ALL	14
+#define		UBLK_IO_OP_ZONE_RESET		15

Because, even after this patch, you would still have a hole between
UBLK_IO_OP_ value 5 and 10.

(5, 10) can be reserved for some more generic normal IOs, maybe new kind
of READ/WRITE, or whatever.

We have 256 OP available, which is big enough for future extend, and OP
code can be used from any offset, but still more readable to group them
according to their category. And it isn't big deal to use which number
for which command, what matters is that we can extend, and keep
compatible.


Thanks,
Ming