On Mon, Oct 02, 2023 at 09:33:22AM -0700, Bart Van Assche wrote: > On 10/2/23 04:53, Niklas Cassel wrote: > > On Mon, Oct 02, 2023 at 01:37:59PM +0200, Niklas Cassel wrote: > > > I don't know which user facing API Martin's I/O hinting series is intending > > > to use. > > > > > > However, while discussing this series at ALPSS, we did ask ourselves why this > > > series is not reusing the already existing block layer API for providing I/O > > > hints: > > > https://github.com/torvalds/linux/blob/v6.6-rc4/include/uapi/linux/ioprio.h#L83-L103 > > > > > > We can have 1023 possible I/O hints, and so far we are only using 7, which > > > means that there are 1016 possible hints left. > > > This also enables you to define more than the 4 previous temperature hints > > > (extreme, long, medium, short), if so desired. > > > > > > There is also support in fio for these I/O hints: > > > https://github.com/axboe/fio/blob/master/HOWTO.rst?plain=1#L2294-L2302 > > > > > > When this new I/O hint API has added, there was no other I/O hint API > > > in the kernel (since the old fcntl() F_GET_FILE_RW_HINT / F_SET_FILE_RW_HINT > > > API had already been removed when this new API was added). > > > > > > So there should probably be a good argument why we would want to introduce > > > yet another API for providing I/O hints, instead of extending the I/O hint > > > API that we already have in the kernel right now. > > > (Especially since it seems fairly easy to modify your patches to reuse the > > > existing API.) > > > > One argument might be that the current I/O hints API does not allow hints to > > be stacked. So one would not e.g. be able to combine a command duration limit > > with a temperature hint... > > Hi Niklas, > > Is your feedback about the user space API only or also about the > mechanism that is used internally in the kernel? The concern is only related to the user space API. (However, if you do reuse the existing I/O prio hints, you will avoid adding a new struct member to a lot of structs.) > > Restoring the ability to pass data temperature information from a > filesystem to a block device is much more important to me than > restoring the ability to pass data temperature information from user > space to a filesystem. Would it be sufficient to address your concern > if patch 2/13 would be dropped from this series? Right now 0 means no I/O hint. Value 1-7 is used for CDL. This means that bits 0-2 are currently used by CDL. I guess we could define e.g. bits 3-5 to be used by temperature hints, i.e. temperature hints could have values 0-7, where 0 would be no temperature hint. (I guess we could still limit the temperature hints to 1-4 if we want to keep the previous extreme/long/medium/short constants.) This way, we can combine a CDL value with a temperature hint. I.e. if user space has set bits in both bits 0-2 and 3-5, then both CDL and temperature hints are used. (And we would still have 4 bits left in 10 bit long I/O hints field that can be used by some other I/O hint feature in the future.) We could theoretically do this without changing the existing I/O prio hints API, as all the existing hints (CDL descriptors 1-7) would keep their existing values. While I think this sounds quite nice, since it would avoid what your patches currently do: adding a new "write_hint" struct member to the following structs: struct kiocb, struct file, struct request, struct request, struct bio. Instead it would rely on the existing ioprio struct members in these structs. Additionally you would not need to add code that avoid merging of requests with different write hints, as the current code already avoids merging of requests with different ioprio (which thus extends to ioprio I/O hints). Anyway, even if I do think that modifying your patch series to use the I/O prio hints API would be a simpler and cleaner solution, including a smaller diffstat, I do not care too strongly about this, and will leave the pondering to the very wise maintainers. Kind regards, Niklas
