[PATCH v6 0/5] AIO add per-command iopriority

[PATCH v6 0/5] AIO add per-command iopriority

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

This is the per-I/O equivalent of the ioprio_set system call.
See the following link for performance implications on a SATA HDD:
https://lkml.org/lkml/2016/12/6/495

First patch factors ioprio_check_cap function out of ioprio_set system call to
also be used by the aio ioprio interface.

Second patch converts kiocb ki_hint field to a u16 to avoid kiocb bloat.

Third patch passes ioprio hint from aio iocb to kiocb and initializes kiocb
ioprio value appropriately when it is not explicitly set.

Fourth patch enables the feature for blkdev.

Fifth patch enables the feature for iomap direct IO

Note: this work is based on top of linux-vfs/for-next

v2: merge patches
    use IOCB_FLAG_IOPRIO
    validate intended use with IOCB_IOPRIO
    add linux-api and linux-block to cc

v3: add ioprio_check_cap function
    convert kiocb ki_hint to u16
    use ioprio_check_cap when adding ioprio to kiocb in aio.c

v4: handle IOCB_IOPRIO in aio_prep_rw
    note patch 3 depends on patch 1 in commit msg

v5: rename ki_hint_valid -> ki_hint_validate
    remove ki_hint_validate comment and whitespace
    remove IOCB_IOPRIO flag
    initialize kiocb to have no priority

v6: add __blkdev_direct_IO_simple ioprio support


Adam Manzanares (5):
  block: add ioprio_check_cap function
  fs: Convert kiocb rw_hint from enum to u16
  fs: Add aio iopriority support
  fs: blkdev set bio prio from kiocb prio
  fs: iomap dio set bio prio from kiocb prio

 block/ioprio.c               | 22 ++++++++++++++++------
 drivers/block/loop.c         |  3 +++
 fs/aio.c                     | 16 ++++++++++++++++
 fs/block_dev.c               |  2 ++
 fs/iomap.c                   |  1 +
 include/linux/fs.h           | 16 ++++++++++++++--
 include/linux/ioprio.h       |  2 ++
 include/uapi/linux/aio_abi.h |  1 +
 8 files changed, 55 insertions(+), 8 deletions(-)

-- 
2.15.1

[PATCH v6 0/5] AIO add per-command iopriority

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

This is the per-I/O equivalent of the ioprio_set system call.
See the following link for performance implications on a SATA HDD:
https://lkml.org/lkml/2016/12/6/495

First patch factors ioprio_check_cap function out of ioprio_set system call to
also be used by the aio ioprio interface.

Second patch converts kiocb ki_hint field to a u16 to avoid kiocb bloat.

Third patch passes ioprio hint from aio iocb to kiocb and initializes kiocb
ioprio value appropriately when it is not explicitly set.

Fourth patch enables the feature for blkdev.

Fifth patch enables the feature for iomap direct IO

Note: this work is based on top of linux-vfs/for-next

v2: merge patches
    use IOCB_FLAG_IOPRIO
    validate intended use with IOCB_IOPRIO
    add linux-api and linux-block to cc

v3: add ioprio_check_cap function
    convert kiocb ki_hint to u16
    use ioprio_check_cap when adding ioprio to kiocb in aio.c

v4: handle IOCB_IOPRIO in aio_prep_rw
    note patch 3 depends on patch 1 in commit msg

v5: rename ki_hint_valid -> ki_hint_validate
    remove ki_hint_validate comment and whitespace
    remove IOCB_IOPRIO flag
    initialize kiocb to have no priority

v6: add __blkdev_direct_IO_simple ioprio support


Adam Manzanares (5):
  block: add ioprio_check_cap function
  fs: Convert kiocb rw_hint from enum to u16
  fs: Add aio iopriority support
  fs: blkdev set bio prio from kiocb prio
  fs: iomap dio set bio prio from kiocb prio

 block/ioprio.c               | 22 ++++++++++++++++------
 drivers/block/loop.c         |  3 +++
 fs/aio.c                     | 16 ++++++++++++++++
 fs/block_dev.c               |  2 ++
 fs/iomap.c                   |  1 +
 include/linux/fs.h           | 16 ++++++++++++++--
 include/linux/ioprio.h       |  2 ++
 include/uapi/linux/aio_abi.h |  1 +
 8 files changed, 55 insertions(+), 8 deletions(-)

-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[PATCH v6 1/5] block: add ioprio_check_cap function

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

Aio per command iopriority support introduces a second interface between
userland and the kernel capable of passing iopriority. The aio interface also
needs the ability to verify that the submitting context has sufficient
privileges to submit IOPRIO_RT commands. This patch creates the
ioprio_check_cap function to be used by the ioprio_set system call and also by
the aio interface.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
---
 block/ioprio.c         | 22 ++++++++++++++++------
 include/linux/ioprio.h |  2 ++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/ioprio.c b/block/ioprio.c
index 6f5d0b6625e3..f9821080c92c 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -61,15 +61,10 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
 }
 EXPORT_SYMBOL_GPL(set_task_ioprio);
 
-SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+int ioprio_check_cap(int ioprio)
 {
 	int class = IOPRIO_PRIO_CLASS(ioprio);
 	int data = IOPRIO_PRIO_DATA(ioprio);
-	struct task_struct *p, *g;
-	struct user_struct *user;
-	struct pid *pgrp;
-	kuid_t uid;
-	int ret;
 
 	switch (class) {
 		case IOPRIO_CLASS_RT:
@@ -92,6 +87,21 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
 			return -EINVAL;
 	}
 
+	return 0;
+}
+
+SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+{
+	struct task_struct *p, *g;
+	struct user_struct *user;
+	struct pid *pgrp;
+	kuid_t uid;
+	int ret;
+
+	ret = ioprio_check_cap(ioprio);
+	if (ret)
+		return ret;
+
 	ret = -ESRCH;
 	rcu_read_lock();
 	switch (which) {
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 627efac73e6d..4a28cec49ec3 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -77,4 +77,6 @@ extern int ioprio_best(unsigned short aprio, unsigned short bprio);
 
 extern int set_task_ioprio(struct task_struct *task, int ioprio);
 
+extern int ioprio_check_cap(int ioprio);
+
 #endif
-- 
2.15.1

[PATCH v6 1/5] block: add ioprio_check_cap function

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

Aio per command iopriority support introduces a second interface between
userland and the kernel capable of passing iopriority. The aio interface also
needs the ability to verify that the submitting context has sufficient
privileges to submit IOPRIO_RT commands. This patch creates the
ioprio_check_cap function to be used by the ioprio_set system call and also by
the aio interface.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
---
 block/ioprio.c         | 22 ++++++++++++++++------
 include/linux/ioprio.h |  2 ++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/ioprio.c b/block/ioprio.c
index 6f5d0b6625e3..f9821080c92c 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -61,15 +61,10 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
 }
 EXPORT_SYMBOL_GPL(set_task_ioprio);
 
-SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+int ioprio_check_cap(int ioprio)
 {
 	int class = IOPRIO_PRIO_CLASS(ioprio);
 	int data = IOPRIO_PRIO_DATA(ioprio);
-	struct task_struct *p, *g;
-	struct user_struct *user;
-	struct pid *pgrp;
-	kuid_t uid;
-	int ret;
 
 	switch (class) {
 		case IOPRIO_CLASS_RT:
@@ -92,6 +87,21 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
 			return -EINVAL;
 	}
 
+	return 0;
+}
+
+SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
+{
+	struct task_struct *p, *g;
+	struct user_struct *user;
+	struct pid *pgrp;
+	kuid_t uid;
+	int ret;
+
+	ret = ioprio_check_cap(ioprio);
+	if (ret)
+		return ret;
+
 	ret = -ESRCH;
 	rcu_read_lock();
 	switch (which) {
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 627efac73e6d..4a28cec49ec3 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -77,4 +77,6 @@ extern int ioprio_best(unsigned short aprio, unsigned short bprio);
 
 extern int set_task_ioprio(struct task_struct *task, int ioprio);
 
+extern int ioprio_check_cap(int ioprio);
+
 #endif
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

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 assignment.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/fs.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7f07977bdfd7..50de40dbbb85 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,19 @@ static inline enum rw_hint file_write_hint(struct file *file)
 
 static inline int iocb_flags(struct file *file);
 
+static inline u16 ki_hint_validate(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_validate(file_write_hint(filp)),
 	};
 }
 
-- 
2.15.1

[PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

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 assignment.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/fs.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7f07977bdfd7..50de40dbbb85 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,19 @@ static inline enum rw_hint file_write_hint(struct file *file)
 
 static inline int iocb_flags(struct file *file);
 
+static inline u16 ki_hint_validate(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_validate(file_write_hint(filp)),
 	};
 }
 
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[PATCH v6 3/5] fs: Add aio iopriority support

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

This is the per-I/O equivalent of the ioprio_set system call.

When IOCB_FLAG_IOPRIO is set on the iocb aio_flags field, then we set the
newly added kiocb ki_ioprio field to the value in the iocb aio_reqprio field.

This patch depends on block: add ioprio_check_cap function.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c         |  3 +++
 fs/aio.c                     | 16 ++++++++++++++++
 include/linux/fs.h           |  3 +++
 include/uapi/linux/aio_abi.h |  1 +
 4 files changed, 23 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e31655d96..dd98dfd97f5e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -76,6 +76,8 @@
 #include <linux/miscdevice.h>
 #include <linux/falloc.h>
 #include <linux/uio.h>
+#include <linux/ioprio.h>
+
 #include "loop.h"
 
 #include <linux/uaccess.h>
@@ -559,6 +561,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
 	cmd->iocb.ki_filp = file;
 	cmd->iocb.ki_complete = lo_rw_aio_complete;
 	cmd->iocb.ki_flags = IOCB_DIRECT;
+	cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
 	if (cmd->css)
 		kthread_associate_blkcg(cmd->css);
 
diff --git a/fs/aio.c b/fs/aio.c
index 755d3f57bcc8..8225013f70f0 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1451,6 +1451,22 @@ static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
 	if (iocb->aio_flags & IOCB_FLAG_RESFD)
 		req->ki_flags |= IOCB_EVENTFD;
 	req->ki_hint = file_write_hint(req->ki_filp);
+	if (iocb->aio_flags & IOCB_FLAG_IOPRIO) {
+		/*
+		 * If the IOCB_FLAG_IOPRIO flag of aio_flags is set, then
+		 * aio_reqprio is interpreted as an I/O scheduling
+		 * class and priority.
+		 */
+		ret = ioprio_check_cap(iocb->aio_reqprio);
+		if (ret) {
+			pr_debug("aio ioprio check cap error\n");
+			return -EINVAL;
+		}
+
+		req->ki_ioprio = iocb->aio_reqprio;
+	} else
+		req->ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
+
 	ret = kiocb_set_rw_flags(req, iocb->aio_rw_flags);
 	if (unlikely(ret))
 		fput(req->ki_filp);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 50de40dbbb85..73b749ed3ea1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -36,6 +36,7 @@
 #include <linux/delayed_call.h>
 #include <linux/uuid.h>
 #include <linux/errseq.h>
+#include <linux/ioprio.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -302,6 +303,7 @@ struct kiocb {
 	void			*private;
 	int			ki_flags;
 	u16			ki_hint;
+	u16			ki_ioprio; /* See linux/ioprio.h */
 } __randomize_layout;
 
 static inline bool is_sync_kiocb(struct kiocb *kiocb)
@@ -1942,6 +1944,7 @@ static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
 		.ki_filp = filp,
 		.ki_flags = iocb_flags(filp),
 		.ki_hint = ki_hint_validate(file_write_hint(filp)),
+		.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0),
 	};
 }
 
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index 2c0a3415beee..d4e768d55d14 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -55,6 +55,7 @@ enum {
  *                   is valid.
  */
 #define IOCB_FLAG_RESFD		(1 << 0)
+#define IOCB_FLAG_IOPRIO	(1 << 1)
 
 /* read() from /dev/aio returns these structures. */
 struct io_event {
-- 
2.15.1

[PATCH v6 3/5] fs: Add aio iopriority support

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

This is the per-I/O equivalent of the ioprio_set system call.

When IOCB_FLAG_IOPRIO is set on the iocb aio_flags field, then we set the
newly added kiocb ki_ioprio field to the value in the iocb aio_reqprio field.

This patch depends on block: add ioprio_check_cap function.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c         |  3 +++
 fs/aio.c                     | 16 ++++++++++++++++
 include/linux/fs.h           |  3 +++
 include/uapi/linux/aio_abi.h |  1 +
 4 files changed, 23 insertions(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 5d4e31655d96..dd98dfd97f5e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -76,6 +76,8 @@
 #include <linux/miscdevice.h>
 #include <linux/falloc.h>
 #include <linux/uio.h>
+#include <linux/ioprio.h>
+
 #include "loop.h"
 
 #include <linux/uaccess.h>
@@ -559,6 +561,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
 	cmd->iocb.ki_filp = file;
 	cmd->iocb.ki_complete = lo_rw_aio_complete;
 	cmd->iocb.ki_flags = IOCB_DIRECT;
+	cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
 	if (cmd->css)
 		kthread_associate_blkcg(cmd->css);
 
diff --git a/fs/aio.c b/fs/aio.c
index 755d3f57bcc8..8225013f70f0 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1451,6 +1451,22 @@ static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
 	if (iocb->aio_flags & IOCB_FLAG_RESFD)
 		req->ki_flags |= IOCB_EVENTFD;
 	req->ki_hint = file_write_hint(req->ki_filp);
+	if (iocb->aio_flags & IOCB_FLAG_IOPRIO) {
+		/*
+		 * If the IOCB_FLAG_IOPRIO flag of aio_flags is set, then
+		 * aio_reqprio is interpreted as an I/O scheduling
+		 * class and priority.
+		 */
+		ret = ioprio_check_cap(iocb->aio_reqprio);
+		if (ret) {
+			pr_debug("aio ioprio check cap error\n");
+			return -EINVAL;
+		}
+
+		req->ki_ioprio = iocb->aio_reqprio;
+	} else
+		req->ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
+
 	ret = kiocb_set_rw_flags(req, iocb->aio_rw_flags);
 	if (unlikely(ret))
 		fput(req->ki_filp);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 50de40dbbb85..73b749ed3ea1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -36,6 +36,7 @@
 #include <linux/delayed_call.h>
 #include <linux/uuid.h>
 #include <linux/errseq.h>
+#include <linux/ioprio.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -302,6 +303,7 @@ struct kiocb {
 	void			*private;
 	int			ki_flags;
 	u16			ki_hint;
+	u16			ki_ioprio; /* See linux/ioprio.h */
 } __randomize_layout;
 
 static inline bool is_sync_kiocb(struct kiocb *kiocb)
@@ -1942,6 +1944,7 @@ static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
 		.ki_filp = filp,
 		.ki_flags = iocb_flags(filp),
 		.ki_hint = ki_hint_validate(file_write_hint(filp)),
+		.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0),
 	};
 }
 
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index 2c0a3415beee..d4e768d55d14 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -55,6 +55,7 @@ enum {
  *                   is valid.
  */
 #define IOCB_FLAG_RESFD		(1 << 0)
+#define IOCB_FLAG_IOPRIO	(1 << 1)
 
 /* read() from /dev/aio returns these structures. */
 struct io_event {
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[PATCH v6 4/5] fs: blkdev set bio prio from kiocb prio

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

Now that kiocb has an ioprio field copy this over to the bio when it is
created from the kiocb.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 fs/block_dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7ec920e27065..11ba99e79d2a 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -216,6 +216,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 	bio.bi_write_hint = iocb->ki_hint;
 	bio.bi_private = current;
 	bio.bi_end_io = blkdev_bio_end_io_simple;
+	bio.bi_ioprio = iocb->ki_ioprio;
 
 	ret = bio_iov_iter_get_pages(&bio, iter);
 	if (unlikely(ret))
@@ -355,6 +356,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
 		bio->bi_write_hint = iocb->ki_hint;
 		bio->bi_private = dio;
 		bio->bi_end_io = blkdev_bio_end_io;
+		bio->bi_ioprio = iocb->ki_ioprio;
 
 		ret = bio_iov_iter_get_pages(bio, iter);
 		if (unlikely(ret)) {
-- 
2.15.1

[PATCH v6 4/5] fs: blkdev set bio prio from kiocb prio

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

Now that kiocb has an ioprio field copy this over to the bio when it is
created from the kiocb.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 fs/block_dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7ec920e27065..11ba99e79d2a 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -216,6 +216,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 	bio.bi_write_hint = iocb->ki_hint;
 	bio.bi_private = current;
 	bio.bi_end_io = blkdev_bio_end_io_simple;
+	bio.bi_ioprio = iocb->ki_ioprio;
 
 	ret = bio_iov_iter_get_pages(&bio, iter);
 	if (unlikely(ret))
@@ -355,6 +356,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
 		bio->bi_write_hint = iocb->ki_hint;
 		bio->bi_private = dio;
 		bio->bi_end_io = blkdev_bio_end_io;
+		bio->bi_ioprio = iocb->ki_ioprio;
 
 		ret = bio_iov_iter_get_pages(bio, iter);
 		if (unlikely(ret)) {
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

[PATCH v6 5/5] fs: iomap dio set bio prio from kiocb prio

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

Now that kiocb has an ioprio field copy this over to the bio when it is
created from the kiocb during direct IO.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/iomap.c b/fs/iomap.c
index afd163586aa0..65aae194aeca 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -919,6 +919,7 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
 		bio->bi_iter.bi_sector =
 			(iomap->addr + pos - iomap->offset) >> 9;
 		bio->bi_write_hint = dio->iocb->ki_hint;
+		bio->bi_ioprio = dio->iocb->ki_ioprio;
 		bio->bi_private = dio;
 		bio->bi_end_io = iomap_dio_bio_end_io;
 
-- 
2.15.1

[PATCH v6 5/5] fs: iomap dio set bio prio from kiocb prio

[adam.manzanares]

From: Adam Manzanares <adam.manzanares@wdc.com>

Now that kiocb has an ioprio field copy this over to the bio when it is
created from the kiocb during direct IO.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/iomap.c b/fs/iomap.c
index afd163586aa0..65aae194aeca 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -919,6 +919,7 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
 		bio->bi_iter.bi_sector =
 			(iomap->addr + pos - iomap->offset) >> 9;
 		bio->bi_write_hint = dio->iocb->ki_hint;
+		bio->bi_ioprio = dio->iocb->ki_ioprio;
 		bio->bi_private = dio;
 		bio->bi_end_io = iomap_dio_bio_end_io;
 
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH v6 4/5] fs: blkdev set bio prio from kiocb prio

[Jeff Moyer]

adam.manzanares@wdc.com writes:

> From: Adam Manzanares <adam.manzanares@wdc.com>
>
> Now that kiocb has an ioprio field copy this over to the bio when it is
> created from the kiocb.
>
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Thanks!
Jeff

> ---
>  fs/block_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 7ec920e27065..11ba99e79d2a 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -216,6 +216,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>  	bio.bi_write_hint = iocb->ki_hint;
>  	bio.bi_private = current;
>  	bio.bi_end_io = blkdev_bio_end_io_simple;
> +	bio.bi_ioprio = iocb->ki_ioprio;
>  
>  	ret = bio_iov_iter_get_pages(&bio, iter);
>  	if (unlikely(ret))
> @@ -355,6 +356,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
>  		bio->bi_write_hint = iocb->ki_hint;
>  		bio->bi_private = dio;
>  		bio->bi_end_io = blkdev_bio_end_io;
> +		bio->bi_ioprio = iocb->ki_ioprio;
>  
>  		ret = bio_iov_iter_get_pages(bio, iter);
>  		if (unlikely(ret)) {

Re: [PATCH v6 4/5] fs: blkdev set bio prio from kiocb prio

[Jeff Moyer]

adam.manzanares@wdc.com writes:

> From: Adam Manzanares <adam.manzanares@wdc.com>
>
> Now that kiocb has an ioprio field copy this over to the bio when it is
> created from the kiocb.
>
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Thanks!
Jeff

> ---
>  fs/block_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 7ec920e27065..11ba99e79d2a 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -216,6 +216,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>  	bio.bi_write_hint = iocb->ki_hint;
>  	bio.bi_private = current;
>  	bio.bi_end_io = blkdev_bio_end_io_simple;
> +	bio.bi_ioprio = iocb->ki_ioprio;
>  
>  	ret = bio_iov_iter_get_pages(&bio, iter);
>  	if (unlikely(ret))
> @@ -355,6 +356,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
>  		bio->bi_write_hint = iocb->ki_hint;
>  		bio->bi_private = dio;
>  		bio->bi_end_io = blkdev_bio_end_io;
> +		bio->bi_ioprio = iocb->ki_ioprio;
>  
>  		ret = bio_iov_iter_get_pages(bio, iter);
>  		if (unlikely(ret)) {

Re: [PATCH v6 4/5] fs: blkdev set bio prio from kiocb prio

[Jeff Moyer]

adam.manzanares@wdc.com writes:

> From: Adam Manzanares <adam.manzanares@wdc.com>
>
> Now that kiocb has an ioprio field copy this over to the bio when it is
> created from the kiocb.
>
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Thanks!
Jeff

> ---
>  fs/block_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 7ec920e27065..11ba99e79d2a 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -216,6 +216,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
>  	bio.bi_write_hint = iocb->ki_hint;
>  	bio.bi_private = current;
>  	bio.bi_end_io = blkdev_bio_end_io_simple;
> +	bio.bi_ioprio = iocb->ki_ioprio;
>  
>  	ret = bio_iov_iter_get_pages(&bio, iter);
>  	if (unlikely(ret))
> @@ -355,6 +356,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
>  		bio->bi_write_hint = iocb->ki_hint;
>  		bio->bi_private = dio;
>  		bio->bi_end_io = blkdev_bio_end_io;
> +		bio->bi_ioprio = iocb->ki_ioprio;
>  
>  		ret = bio_iov_iter_get_pages(bio, iter);
>  		if (unlikely(ret)) {

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Jens Axboe]

On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
> From: Adam Manzanares <adam.manzanares@wdc.com>
> 
> 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 assignment.
> 
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/fs.h | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7f07977bdfd7..50de40dbbb85 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 */

Instead of having to do this and now rely on those now being synced,
how about something like the below.

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..070438d0b62d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
 
 static inline int iocb_flags(struct file *file);
 
+static inline u16 ki_hint_validate(enum rw_hint hint)
+{
+	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
+
+	if (hint <= max_hint)
+		return hint;
+
+	return 0;
+}
+
 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_validate(file_write_hint(filp)),
 	};
 }
 

-- 
Jens Axboe

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Jens Axboe]

On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
> From: Adam Manzanares <adam.manzanares@wdc.com>
> 
> 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 assignment.
> 
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/fs.h | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7f07977bdfd7..50de40dbbb85 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 */

Instead of having to do this and now rely on those now being synced,
how about something like the below.

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..070438d0b62d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
 
 static inline int iocb_flags(struct file *file);
 
+static inline u16 ki_hint_validate(enum rw_hint hint)
+{
+	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
+
+	if (hint <= max_hint)
+		return hint;
+
+	return 0;
+}
+
 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_validate(file_write_hint(filp)),
 	};
 }
 

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Adam Manzanares]

DQoNCk9uIDUvMjIvMTggODozMiBBTSwgSmVucyBBeGJvZSB3cm90ZToNCj4gT24gNS8yMi8xOCA5
OjA3IEFNLCBhZGFtLm1hbnphbmFyZXNAd2RjLmNvbSB3cm90ZToNCj4+IEZyb206IEFkYW0gTWFu
emFuYXJlcyA8YWRhbS5tYW56YW5hcmVzQHdkYy5jb20+DQo+Pg0KPj4gSW4gb3JkZXIgdG8gYXZv
aWQga2lvY2IgYmxvYXQgZm9yIHBlciBjb21tYW5kIGlvcHJpb3JpdHkgc3VwcG9ydCwgcndfaGlu
dA0KPj4gaXMgY29udmVydGVkIGZyb20gZW51bSB0byBhIHUxNi4gQWRkZWQgYSBndWFyZCBhcm91
bmQga2lfaGludCBhc3NpZ25tZW50Lg0KPj4NCj4+IFNpZ25lZC1vZmYtYnk6IEFkYW0gTWFuemFu
YXJlcyA8YWRhbS5tYW56YW5hcmVzQHdkYy5jb20+DQo+PiBSZXZpZXdlZC1ieTogQ2hyaXN0b3Bo
IEhlbGx3aWcgPGhjaEBsc3QuZGU+DQo+PiAtLS0NCj4+ICAgaW5jbHVkZS9saW51eC9mcy5oIHwg
MTMgKysrKysrKysrKystLQ0KPj4gICAxIGZpbGUgY2hhbmdlZCwgMTEgaW5zZXJ0aW9ucygrKSwg
MiBkZWxldGlvbnMoLSkNCj4+DQo+PiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC9mcy5oIGIv
aW5jbHVkZS9saW51eC9mcy5oDQo+PiBpbmRleCA3ZjA3OTc3YmRmZDcuLjUwZGU0MGRiYmI4NSAx
MDA2NDQNCj4+IC0tLSBhL2luY2x1ZGUvbGludXgvZnMuaA0KPj4gKysrIGIvaW5jbHVkZS9saW51
eC9mcy5oDQo+PiBAQCAtMjg0LDYgKzI4NCw4IEBAIGVudW0gcndfaGludCB7DQo+PiAgIAlXUklU
RV9MSUZFX0VYVFJFTUUJPSBSV0hfV1JJVEVfTElGRV9FWFRSRU1FLA0KPj4gICB9Ow0KPj4gICAN
Cj4+ICsjZGVmaW5lIE1BWF9LSV9ISU5UCQkoKDEgPDwgMTYpIC0gMSkgLyoga2lfaGludCB0eXBl
IGlzIHUxNiAqLw0KPiANCj4gSW5zdGVhZCBvZiBoYXZpbmcgdG8gZG8gdGhpcyBhbmQgbm93IHJl
bHkgb24gdGhvc2Ugbm93IGJlaW5nIHN5bmNlZCwNCj4gaG93IGFib3V0IHNvbWV0aGluZyBsaWtl
IHRoZSBiZWxvdy4NCj4gDQo+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4L2ZzLmggYi9pbmNs
dWRlL2xpbnV4L2ZzLmgNCj4gaW5kZXggNzYwZDhkYTFiNmM3Li4wNzA0MzhkMGI2MmQgMTAwNjQ0
DQo+IC0tLSBhL2luY2x1ZGUvbGludXgvZnMuaA0KPiArKysgYi9pbmNsdWRlL2xpbnV4L2ZzLmgN
Cj4gQEAgLTI5OSw3ICsyOTksNyBAQCBzdHJ1Y3Qga2lvY2Igew0KPiAgIAl2b2lkICgqa2lfY29t
cGxldGUpKHN0cnVjdCBraW9jYiAqaW9jYiwgbG9uZyByZXQsIGxvbmcgcmV0Mik7DQo+ICAgCXZv
aWQJCQkqcHJpdmF0ZTsNCj4gICAJaW50CQkJa2lfZmxhZ3M7DQo+IC0JZW51bSByd19oaW50CQlr
aV9oaW50Ow0KPiArCXUxNgkJCWtpX2hpbnQ7DQo+ICAgfSBfX3JhbmRvbWl6ZV9sYXlvdXQ7DQo+
ICAgDQo+ICAgc3RhdGljIGlubGluZSBib29sIGlzX3N5bmNfa2lvY2Ioc3RydWN0IGtpb2NiICpr
aW9jYikNCj4gQEAgLTE5MjcsMTIgKzE5MjcsMjIgQEAgc3RhdGljIGlubGluZSBlbnVtIHJ3X2hp
bnQgZmlsZV93cml0ZV9oaW50KHN0cnVjdCBmaWxlICpmaWxlKQ0KPiAgIA0KPiAgIHN0YXRpYyBp
bmxpbmUgaW50IGlvY2JfZmxhZ3Moc3RydWN0IGZpbGUgKmZpbGUpOw0KPiAgIA0KPiArc3RhdGlj
IGlubGluZSB1MTYga2lfaGludF92YWxpZGF0ZShlbnVtIHJ3X2hpbnQgaGludCkNCj4gK3sNCj4g
Kwl0eXBlb2YoKChzdHJ1Y3Qga2lvY2IgKikwKS0+a2lfaGludCkgbWF4X2hpbnQgPSAtMTsNCj4g
Kw0KPiArCWlmIChoaW50IDw9IG1heF9oaW50KQ0KPiArCQlyZXR1cm4gaGludDsNCj4gKw0KPiAr
CXJldHVybiAwOw0KPiArfQ0KPiArDQo+ICAgc3RhdGljIGlubGluZSB2b2lkIGluaXRfc3luY19r
aW9jYihzdHJ1Y3Qga2lvY2IgKmtpb2NiLCBzdHJ1Y3QgZmlsZSAqZmlscCkNCj4gICB7DQo+ICAg
CSpraW9jYiA9IChzdHJ1Y3Qga2lvY2IpIHsNCj4gICAJCS5raV9maWxwID0gZmlscCwNCj4gICAJ
CS5raV9mbGFncyA9IGlvY2JfZmxhZ3MoZmlscCksDQo+IC0JCS5raV9oaW50ID0gZmlsZV93cml0
ZV9oaW50KGZpbHApLA0KPiArCQkua2lfaGludCA9IGtpX2hpbnRfdmFsaWRhdGUoZmlsZV93cml0
ZV9oaW50KGZpbHApKSwNCj4gICAJfTsNCj4gICB9DQoNCkxvb2tzIGdvb2QuIEknbGwgcmVzdWJt
aXQuDQoNCj4gICANCj4g

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Adam Manzanares]

On 5/22/18 8:32 AM, Jens Axboe wrote:
> On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>
>> 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 assignment.
>>
>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   include/linux/fs.h | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 7f07977bdfd7..50de40dbbb85 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 */
> 
> Instead of having to do this and now rely on those now being synced,
> how about something like the below.
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..070438d0b62d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
>   
>   static inline int iocb_flags(struct file *file);
>   
> +static inline u16 ki_hint_validate(enum rw_hint hint)
> +{
> +	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
> +
> +	if (hint <= max_hint)
> +		return hint;
> +
> +	return 0;
> +}
> +
>   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_validate(file_write_hint(filp)),
>   	};
>   }

Looks good. I'll resubmit.

>   
> 

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Adam Manzanares]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2226 bytes --]



On 5/22/18 8:32 AM, Jens Axboe wrote:
> On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>
>> 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 assignment.
>>
>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   include/linux/fs.h | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 7f07977bdfd7..50de40dbbb85 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 */
> 
> Instead of having to do this and now rely on those now being synced,
> how about something like the below.
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..070438d0b62d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
>   
>   static inline int iocb_flags(struct file *file);
>   
> +static inline u16 ki_hint_validate(enum rw_hint hint)
> +{
> +	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
> +
> +	if (hint <= max_hint)
> +		return hint;
> +
> +	return 0;
> +}
> +
>   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_validate(file_write_hint(filp)),
>   	};
>   }

Looks good. I'll resubmit.

>   
> N‹§²æìr¸›zǧu©ž²Æ {\b­†éì¹»\x1c®&Þ–)îŨ¨Š{ayº\x1dÊÚ&j:+v‰¨’öœ’Šà\x16Šæ¢·¢ú(œ¸§»\x10\b:Çž†Ûiÿü0ÂKÚrJ+ƒö¢£ðèž×¦j)Z†·Ÿ

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Goldwyn Rodrigues]

On 05/22/2018 10:32 AM, Jens Axboe wrote:
> On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>
>> 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 assignment.
>>
>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  include/linux/fs.h | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 7f07977bdfd7..50de40dbbb85 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 */
> 
> Instead of having to do this and now rely on those now being synced,
> how about something like the below.
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..070438d0b62d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
>  
>  static inline int iocb_flags(struct file *file);
>  
> +static inline u16 ki_hint_validate(enum rw_hint hint)
> +{
> +	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;

This looks complex to me. Would force a reader to lookback at what
datatype ki_hint is. I'd prefer to declare it as u16 max_hint = -1, or
even the previous #define MAX_KI_HINT format is easier to read. Just a
program reading style you are comfortable with though.


-- 
Goldwyn

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Goldwyn Rodrigues]

On 05/22/2018 10:32 AM, Jens Axboe wrote:
> On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>
>> 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 assignment.
>>
>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  include/linux/fs.h | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 7f07977bdfd7..50de40dbbb85 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 */
> 
> Instead of having to do this and now rely on those now being synced,
> how about something like the below.
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..070438d0b62d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
>  
>  static inline int iocb_flags(struct file *file);
>  
> +static inline u16 ki_hint_validate(enum rw_hint hint)
> +{
> +	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;

This looks complex to me. Would force a reader to lookback at what
datatype ki_hint is. I'd prefer to declare it as u16 max_hint = -1, or
even the previous #define MAX_KI_HINT format is easier to read. Just a
program reading style you are comfortable with though.


-- 
Goldwyn

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Jens Axboe]

On 5/22/18 10:24 AM, Goldwyn Rodrigues wrote:
> 
> 
> On 05/22/2018 10:32 AM, Jens Axboe wrote:
>> On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
>>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>>
>>> 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 assignment.
>>>
>>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>  include/linux/fs.h | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 7f07977bdfd7..50de40dbbb85 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 */
>>
>> Instead of having to do this and now rely on those now being synced,
>> how about something like the below.
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 760d8da1b6c7..070438d0b62d 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
>>  
>>  static inline int iocb_flags(struct file *file);
>>  
>> +static inline u16 ki_hint_validate(enum rw_hint hint)
>> +{
>> +	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
> 
> This looks complex to me. Would force a reader to lookback at what
> datatype ki_hint is. I'd prefer to declare it as u16 max_hint = -1, or
> even the previous #define MAX_KI_HINT format is easier to read. Just a
> program reading style you are comfortable with though.

How is it complex? It's defining a type that'll be the same as ki_hint
in the kiocb, which is _exactly_ what we care about. Any sort of other
definition will rely on those two locations now being synced. The
above will never break.

So I strongly disagree. The above will _never_ require the reader to
figure out what the type is. Any other variant will _always_ require
the reader to check if they are the same.

-- 
Jens Axboe

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Jens Axboe]

On 5/22/18 10:24 AM, Goldwyn Rodrigues wrote:
> 
> 
> On 05/22/2018 10:32 AM, Jens Axboe wrote:
>> On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
>>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>>
>>> 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 assignment.
>>>
>>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>  include/linux/fs.h | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 7f07977bdfd7..50de40dbbb85 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 */
>>
>> Instead of having to do this and now rely on those now being synced,
>> how about something like the below.
>>
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 760d8da1b6c7..070438d0b62d 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
>>  
>>  static inline int iocb_flags(struct file *file);
>>  
>> +static inline u16 ki_hint_validate(enum rw_hint hint)
>> +{
>> +	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
> 
> This looks complex to me. Would force a reader to lookback at what
> datatype ki_hint is. I'd prefer to declare it as u16 max_hint = -1, or
> even the previous #define MAX_KI_HINT format is easier to read. Just a
> program reading style you are comfortable with though.

How is it complex? It's defining a type that'll be the same as ki_hint
in the kiocb, which is _exactly_ what we care about. Any sort of other
definition will rely on those two locations now being synced. The
above will never break.

So I strongly disagree. The above will _never_ require the reader to
figure out what the type is. Any other variant will _always_ require
the reader to check if they are the same.

-- 
Jens Axboe

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Adam Manzanares]

DQoNCk9uIDUvMjIvMTggOTozMCBBTSwgSmVucyBBeGJvZSB3cm90ZToNCj4gT24gNS8yMi8xOCAx
MDoyNCBBTSwgR29sZHd5biBSb2RyaWd1ZXMgd3JvdGU6DQo+Pg0KPj4NCj4+IE9uIDA1LzIyLzIw
MTggMTA6MzIgQU0sIEplbnMgQXhib2Ugd3JvdGU6DQo+Pj4gT24gNS8yMi8xOCA5OjA3IEFNLCBh
ZGFtLm1hbnphbmFyZXNAd2RjLmNvbSB3cm90ZToNCj4+Pj4gRnJvbTogQWRhbSBNYW56YW5hcmVz
IDxhZGFtLm1hbnphbmFyZXNAd2RjLmNvbT4NCj4+Pj4NCj4+Pj4gSW4gb3JkZXIgdG8gYXZvaWQg
a2lvY2IgYmxvYXQgZm9yIHBlciBjb21tYW5kIGlvcHJpb3JpdHkgc3VwcG9ydCwgcndfaGludA0K
Pj4+PiBpcyBjb252ZXJ0ZWQgZnJvbSBlbnVtIHRvIGEgdTE2LiBBZGRlZCBhIGd1YXJkIGFyb3Vu
ZCBraV9oaW50IGFzc2lnbm1lbnQuDQo+Pj4+DQo+Pj4+IFNpZ25lZC1vZmYtYnk6IEFkYW0gTWFu
emFuYXJlcyA8YWRhbS5tYW56YW5hcmVzQHdkYy5jb20+DQo+Pj4+IFJldmlld2VkLWJ5OiBDaHJp
c3RvcGggSGVsbHdpZyA8aGNoQGxzdC5kZT4NCj4+Pj4gLS0tDQo+Pj4+ICAgaW5jbHVkZS9saW51
eC9mcy5oIHwgMTMgKysrKysrKysrKystLQ0KPj4+PiAgIDEgZmlsZSBjaGFuZ2VkLCAxMSBpbnNl
cnRpb25zKCspLCAyIGRlbGV0aW9ucygtKQ0KPj4+Pg0KPj4+PiBkaWZmIC0tZ2l0IGEvaW5jbHVk
ZS9saW51eC9mcy5oIGIvaW5jbHVkZS9saW51eC9mcy5oDQo+Pj4+IGluZGV4IDdmMDc5NzdiZGZk
Ny4uNTBkZTQwZGJiYjg1IDEwMDY0NA0KPj4+PiAtLS0gYS9pbmNsdWRlL2xpbnV4L2ZzLmgNCj4+
Pj4gKysrIGIvaW5jbHVkZS9saW51eC9mcy5oDQo+Pj4+IEBAIC0yODQsNiArMjg0LDggQEAgZW51
bSByd19oaW50IHsNCj4+Pj4gICAJV1JJVEVfTElGRV9FWFRSRU1FCT0gUldIX1dSSVRFX0xJRkVf
RVhUUkVNRSwNCj4+Pj4gICB9Ow0KPj4+PiAgIA0KPj4+PiArI2RlZmluZSBNQVhfS0lfSElOVAkJ
KCgxIDw8IDE2KSAtIDEpIC8qIGtpX2hpbnQgdHlwZSBpcyB1MTYgKi8NCj4+Pg0KPj4+IEluc3Rl
YWQgb2YgaGF2aW5nIHRvIGRvIHRoaXMgYW5kIG5vdyByZWx5IG9uIHRob3NlIG5vdyBiZWluZyBz
eW5jZWQsDQo+Pj4gaG93IGFib3V0IHNvbWV0aGluZyBsaWtlIHRoZSBiZWxvdy4NCj4+Pg0KPj4+
IGRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4L2ZzLmggYi9pbmNsdWRlL2xpbnV4L2ZzLmgNCj4+
PiBpbmRleCA3NjBkOGRhMWI2YzcuLjA3MDQzOGQwYjYyZCAxMDA2NDQNCj4+PiAtLS0gYS9pbmNs
dWRlL2xpbnV4L2ZzLmgNCj4+PiArKysgYi9pbmNsdWRlL2xpbnV4L2ZzLmgNCj4+PiBAQCAtMjk5
LDcgKzI5OSw3IEBAIHN0cnVjdCBraW9jYiB7DQo+Pj4gICAJdm9pZCAoKmtpX2NvbXBsZXRlKShz
dHJ1Y3Qga2lvY2IgKmlvY2IsIGxvbmcgcmV0LCBsb25nIHJldDIpOw0KPj4+ICAgCXZvaWQJCQkq
cHJpdmF0ZTsNCj4+PiAgIAlpbnQJCQlraV9mbGFnczsNCj4+PiAtCWVudW0gcndfaGludAkJa2lf
aGludDsNCj4+PiArCXUxNgkJCWtpX2hpbnQ7DQo+Pj4gICB9IF9fcmFuZG9taXplX2xheW91dDsN
Cj4+PiAgIA0KPj4+ICAgc3RhdGljIGlubGluZSBib29sIGlzX3N5bmNfa2lvY2Ioc3RydWN0IGtp
b2NiICpraW9jYikNCj4+PiBAQCAtMTkyNywxMiArMTkyNywyMiBAQCBzdGF0aWMgaW5saW5lIGVu
dW0gcndfaGludCBmaWxlX3dyaXRlX2hpbnQoc3RydWN0IGZpbGUgKmZpbGUpDQo+Pj4gICANCj4+
PiAgIHN0YXRpYyBpbmxpbmUgaW50IGlvY2JfZmxhZ3Moc3RydWN0IGZpbGUgKmZpbGUpOw0KPj4+
ICAgDQo+Pj4gK3N0YXRpYyBpbmxpbmUgdTE2IGtpX2hpbnRfdmFsaWRhdGUoZW51bSByd19oaW50
IGhpbnQpDQo+Pj4gK3sNCj4+PiArCXR5cGVvZigoKHN0cnVjdCBraW9jYiAqKTApLT5raV9oaW50
KSBtYXhfaGludCA9IC0xOw0KPj4NCj4+IFRoaXMgbG9va3MgY29tcGxleCB0byBtZS4gV291bGQg
Zm9yY2UgYSByZWFkZXIgdG8gbG9va2JhY2sgYXQgd2hhdA0KPj4gZGF0YXR5cGUga2lfaGludCBp
cy4gSSdkIHByZWZlciB0byBkZWNsYXJlIGl0IGFzIHUxNiBtYXhfaGludCA9IC0xLCBvcg0KPj4g
ZXZlbiB0aGUgcHJldmlvdXMgI2RlZmluZSBNQVhfS0lfSElOVCBmb3JtYXQgaXMgZWFzaWVyIHRv
IHJlYWQuIEp1c3QgYQ0KPj4gcHJvZ3JhbSByZWFkaW5nIHN0eWxlIHlvdSBhcmUgY29tZm9ydGFi
bGUgd2l0aCB0aG91Z2guDQo+IA0KPiBIb3cgaXMgaXQgY29tcGxleD8gSXQncyBkZWZpbmluZyBh
IHR5cGUgdGhhdCdsbCBiZSB0aGUgc2FtZSBhcyBraV9oaW50DQo+IGluIHRoZSBraW9jYiwgd2hp
Y2ggaXMgX2V4YWN0bHlfIHdoYXQgd2UgY2FyZSBhYm91dC4gQW55IHNvcnQgb2Ygb3RoZXINCj4g
ZGVmaW5pdGlvbiB3aWxsIHJlbHkgb24gdGhvc2UgdHdvIGxvY2F0aW9ucyBub3cgYmVpbmcgc3lu
Y2VkLiBUaGUNCj4gYWJvdmUgd2lsbCBuZXZlciBicmVhay4NCj4gDQo+IFNvIEkgc3Ryb25nbHkg
ZGlzYWdyZWUuIFRoZSBhYm92ZSB3aWxsIF9uZXZlcl8gcmVxdWlyZSB0aGUgcmVhZGVyIHRvDQo+
IGZpZ3VyZSBvdXQgd2hhdCB0aGUgdHlwZSBpcy4gQW55IG90aGVyIHZhcmlhbnQgd2lsbCBfYWx3
YXlzXyByZXF1aXJlDQo+IHRoZSByZWFkZXIgdG8gY2hlY2sgaWYgdGhleSBhcmUgdGhlIHNhbWUu
DQo+IA0KDQpJIGRvIHRoaW5rIHRoZSBwcmV2aW91cyBjb2RlIHdhcyBhIGJpdCBlYXNpZXIgdG8g
cGFyc2UgYXQgZmlyc3QgZ2xhbmNlLCANCmJ1dCB0aGF0IGlzIG91dHdlaWdoZWQgYnkgdGhlIGZh
Y3QgdGhhdCB0aGUgdmFsaWRhdGUgZnVuY3Rpb24gaXMgbm93IA0KZGlyZWN0bHkgdGllZCB0byB0
aGUga2lvY2Iga2lfaGludCB0eXBlLg0KDQpJIGFsc28gbWlzc2VkIG9uZSBzcG90IHdoZXJlIEkg
c2hvdWxkIGhhdmUgdXNlZCBraV9oaW50X3ZhbGlkYXRlLiBXaWxsIA0KcmVzZW5kIHNvb24u

Re: [PATCH v6 2/5] fs: Convert kiocb rw_hint from enum to u16

[Adam Manzanares]

On 5/22/18 9:30 AM, Jens Axboe wrote:
> On 5/22/18 10:24 AM, Goldwyn Rodrigues wrote:
>>
>>
>> On 05/22/2018 10:32 AM, Jens Axboe wrote:
>>> On 5/22/18 9:07 AM, adam.manzanares@wdc.com wrote:
>>>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>>>
>>>> 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 assignment.
>>>>
>>>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>> ---
>>>>   include/linux/fs.h | 13 +++++++++++--
>>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>> index 7f07977bdfd7..50de40dbbb85 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 */
>>>
>>> Instead of having to do this and now rely on those now being synced,
>>> how about something like the below.
>>>
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 760d8da1b6c7..070438d0b62d 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -299,7 +299,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 +1927,22 @@ static inline enum rw_hint file_write_hint(struct file *file)
>>>   
>>>   static inline int iocb_flags(struct file *file);
>>>   
>>> +static inline u16 ki_hint_validate(enum rw_hint hint)
>>> +{
>>> +	typeof(((struct kiocb *)0)->ki_hint) max_hint = -1;
>>
>> This looks complex to me. Would force a reader to lookback at what
>> datatype ki_hint is. I'd prefer to declare it as u16 max_hint = -1, or
>> even the previous #define MAX_KI_HINT format is easier to read. Just a
>> program reading style you are comfortable with though.
> 
> How is it complex? It's defining a type that'll be the same as ki_hint
> in the kiocb, which is _exactly_ what we care about. Any sort of other
> definition will rely on those two locations now being synced. The
> above will never break.
> 
> So I strongly disagree. The above will _never_ require the reader to
> figure out what the type is. Any other variant will _always_ require
> the reader to check if they are the same.
> 

I do think the previous code was a bit easier to parse at first glance, 
but that is outweighed by the fact that the validate function is now 
directly tied to the kiocb ki_hint type.

I also missed one spot where I should have used ki_hint_validate. Will 
resend soon.