[v3,2/3] fs: Convert kiocb rw_hint from enum to u16

Commit Message

Adam Manzanares May 8, 2018, 5:42 p.m. UTC

From: Adam Manzanares <[e-mail]>

In order to avoid kiocb bloat for per command iopriority support, rw_hint
is converted from enum to a u16. Added a guard around ki_hint assigment.

Signed-off-by: Adam Manzanares <[e-mail]>
---
 include/linux/fs.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Theodore Ts'o May 9, 2018, 1:34 p.m. UTC | #1

On Tue, May 08, 2018 at 10:42:01AM -0700, [e-mail] wrote:
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..7a90ce387e00 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -284,6 +284,8 @@ enum rw_hint {
>  	WRITE_LIFE_EXTREME	= RWH_WRITE_LIFE_EXTREME,
>  };
>  
> +#define MAX_KI_HINT		((1 << 16) - 1) /* ki_hint type is u16 */
> +

Do we really think there will be *ever* be a need for more than 16 I/O
priority levels?  I would much rather use the low four bits of KI_HINT
for the priority level, and reserve the rest of the 16 bits in KI_HINT
for some future use.  (For example, we might want to use some number
of bits for a stream ID.)

					- Ted

Jens Axboe May 9, 2018, 2:23 p.m. UTC | #2

On 5/9/18 7:34 AM, Theodore Y. Ts'o wrote:
> On Tue, May 08, 2018 at 10:42:01AM -0700, [e-mail] wrote:
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 760d8da1b6c7..7a90ce387e00 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -284,6 +284,8 @@ enum rw_hint {
>>  	WRITE_LIFE_EXTREME	= RWH_WRITE_LIFE_EXTREME,
>>  };
>>  
>> +#define MAX_KI_HINT		((1 << 16) - 1) /* ki_hint type is u16 */
>> +
> 
> Do we really think there will be *ever* be a need for more than 16 I/O
> priority levels?  I would much rather use the low four bits of KI_HINT
> for the priority level, and reserve the rest of the 16 bits in KI_HINT
> for some future use.  (For example, we might want to use some number
> of bits for a stream ID.)

Streams is essentially the only thing ki_hint is currently used for,
with the write life time hints mapping to a stream. The idea for the
user side API was to have other things than just write life time hints.

Since Adam wants to do priorities, he'd either need to pack into the
existing ki_hint, or do this patch does, which is make it smaller and
add a new member. I think the latter is cleaner.

Theodore Ts'o May 9, 2018, 3:21 p.m. UTC | #3

On Wed, May 09, 2018 at 08:23:00AM -0600, Jens Axboe wrote:
> Streams is essentially the only thing ki_hint is currently used for,
> with the write life time hints mapping to a stream. The idea for the
> user side API was to have other things than just write life time hints.
> 
> Since Adam wants to do priorities, he'd either need to pack into the
> existing ki_hint, or do this patch does, which is make it smaller and
> add a new member. I think the latter is cleaner.

Fair enough; but maybe we can use a u8 instead of a u16?  65,535
priorities still seem like way more than would ever make sense.  I
think 256 priorities is still way to many, but it's simpler while
still reserving number of bits for future se.

            		       	       	   - Ted

Adam Manzanares May 9, 2018, 3:29 p.m. UTC | #4

On 5/9/18 11:21 AM, Theodore Y. Ts'o wrote:
> On Wed, May 09, 2018 at 08:23:00AM -0600, Jens Axboe wrote:

>> Streams is essentially the only thing ki_hint is currently used for,

>> with the write life time hints mapping to a stream. The idea for the

>> user side API was to have other things than just write life time hints.

>>

>> Since Adam wants to do priorities, he'd either need to pack into the

>> existing ki_hint, or do this patch does, which is make it smaller and

>> add a new member. I think the latter is cleaner.

> 

> Fair enough; but maybe we can use a u8 instead of a u16?  65,535

> priorities still seem like way more than would ever make sense.  I

> think 256 priorities is still way to many, but it's simpler while

> still reserving number of bits for future se.


The intention was to mimic the ioprio_set system call, which uses 3 bits 
for a prio class and 13 bits for a prio_value.

IDK what is the right amount of bits to use, but the existing use of 
bits seemed flexible enough to support many types of applications and 
devices.
> 

>              		       	       	   - Ted

>

Patch

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..7a90ce387e00 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -284,6 +284,8 @@  enum rw_hint {
 	WRITE_LIFE_EXTREME	= RWH_WRITE_LIFE_EXTREME,
 };
 
+#define MAX_KI_HINT		((1 << 16) - 1) /* ki_hint type is u16 */
+
 #define IOCB_EVENTFD		(1 << 0)
 #define IOCB_APPEND		(1 << 1)
 #define IOCB_DIRECT		(1 << 2)
@@ -299,7 +301,7 @@  struct kiocb {
 	void (*ki_complete)(struct kiocb *iocb, long ret, long ret2);
 	void			*private;
 	int			ki_flags;
-	enum rw_hint		ki_hint;
+	u16			ki_hint;
 } __randomize_layout;
 
 static inline bool is_sync_kiocb(struct kiocb *kiocb)
@@ -1927,12 +1929,21 @@  static inline enum rw_hint file_write_hint(struct file *file)
 
 static inline int iocb_flags(struct file *file);
 
+/* ki_hint changed from enum to u16, make sure rw_hint fits into u16 */
+static inline u16 ki_hint_valid(enum rw_hint hint)
+{
+	if (hint > MAX_KI_HINT)
+		return 0;
+
+	return hint;
+}
+
 static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
 {
 	*kiocb = (struct kiocb) {
 		.ki_filp = filp,
 		.ki_flags = iocb_flags(filp),
-		.ki_hint = file_write_hint(filp),
+		.ki_hint = ki_hint_valid(file_write_hint(filp)),
 	};
 }