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: > > On Thu, Aug 10, 2023 at 01:10:30PM +0000, Niklas Cassel wrote: > > > On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote: > > (snip) > > > 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. > > > > > 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? > > > > > > 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
