[PATCH 0/2] AIO add per-command iopriority

[PATCH 0/2] AIO add per-command iopriority

[adam.manzanares]

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

This patchset interprets the aio_reqprio field of an iocb as a 
per-command value iff the RWF_IOPRIO flag is set on the iocb. 
This feature is implemented for a block device, but could also be
leveraged by any consumers of the iocb.

See the following link for performance implications on a SATA HDD:
https://lkml.org/lkml/2016/12/6/495

Adam Manzanares (2):
  fs: add RWF_IOPRIO
  fs: Add aio priority support for block_dev

 fs/aio.c                | 9 +++++++++
 fs/block_dev.c          | 1 +
 include/linux/fs.h      | 4 ++++
 include/uapi/linux/fs.h | 5 ++++-
 4 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.15.1

[PATCH 1/2] fs: add RWF_IOPRIO

[adam.manzanares]

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

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

When the RWF_IOPRIO flag is set then the aio_reqprio field of the iocb
is interpreted as an I/O scheduling class and priority.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 include/linux/fs.h      | 4 ++++
 include/uapi/linux/fs.h | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..32614eb72a4a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -292,6 +292,7 @@ enum rw_hint {
 #define IOCB_SYNC		(1 << 5)
 #define IOCB_WRITE		(1 << 6)
 #define IOCB_NOWAIT		(1 << 7)
+#define IOCB_IOPRIO		(1 << 8)
 
 struct kiocb {
 	struct file		*ki_filp;
@@ -300,6 +301,7 @@ struct kiocb {
 	void			*private;
 	int			ki_flags;
 	enum rw_hint		ki_hint;
+	u16			ki_ioprio; /* See linux/ioprio.h */
 } __randomize_layout;
 
 static inline bool is_sync_kiocb(struct kiocb *kiocb)
@@ -3243,6 +3245,8 @@ static inline int kiocb_set_rw_flags(struct kiocb *ki, rwf_t flags)
 		ki->ki_flags |= (IOCB_DSYNC | IOCB_SYNC);
 	if (flags & RWF_APPEND)
 		ki->ki_flags |= IOCB_APPEND;
+	if (flags & RWF_IOPRIO)
+		ki->ki_flags |= IOCB_IOPRIO;
 	return 0;
 }
 
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index d2a8313fabd7..c8f69a00b566 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -380,8 +380,11 @@ typedef int __bitwise __kernel_rwf_t;
 /* per-IO O_APPEND */
 #define RWF_APPEND	((__force __kernel_rwf_t)0x00000010)
 
+/* per-IO IOPRIO */
+#define RWF_IOPRIO	((__force __kernel_rwf_t)0x00000020)
+
 /* mask of flags supported by the kernel */
 #define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\
-			 RWF_APPEND)
+			 RWF_APPEND | RWF_IOPRIO)
 
 #endif /* _UAPI_LINUX_FS_H */
-- 
2.15.1

[PATCH 2/2] fs: Add aio priority support for block_dev

[adam.manzanares]

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

When the IOCB_IOPRIO flag is set because the user supplied iocb
has the RWF_IOPRIO flag is set then we set the priority value of
the kiocb from the iocb.

When a bio is created for an aio request by the block dev we set the
priority value of the bio to the user supplied value.

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

diff --git a/fs/aio.c b/fs/aio.c
index 88d7927ffbc6..47591f9e7ba3 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1603,6 +1603,15 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 		goto out_put_req;
 	}
 
+	if (req->common.ki_flags & IOCB_IOPRIO)
+		/*
+		 * The IOCB_IOPRIO flag is set when the user supplied iocb
+		 * aio_rw_flag field has the RWF_IOPRIO flag set. If so,
+		 * aio_reqprio is interpreted as a I/O scheduling class and
+		 * priority.
+		 */
+		req->common.ki_ioprio = iocb->aio_reqprio;
+
 	ret = put_user(KIOCB_KEY, &user_iocb->aio_key);
 	if (unlikely(ret)) {
 		pr_debug("EFAULT: aio_key\n");
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7ec920e27065..da1e94d2bb75 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -355,6 +355,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

Re: [PATCH 1/2] fs: add RWF_IOPRIO

[Christoph Hellwig]

On Mon, Apr 30, 2018 at 09:57:39AM -0700, adam.manzanares@wdc.com wrote:
> From: Adam Manzanares <adam.manzanares@wdc.com>
> 
> This is the per-I/O equivalent of the ioprio_set system call.
> 
> When the RWF_IOPRIO flag is set then the aio_reqprio field of the iocb
> is interpreted as an I/O scheduling class and priority.

I think this belongs into the IOCB_FLAG_* flags namespace for aio_flags
field as it isn't a field valid for plain read/write.

Also you probably want to merge both patches as they only really
make sense together.

Re: [PATCH 2/2] fs: Add aio priority support for block_dev

[Christoph Hellwig]

> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1603,6 +1603,15 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
>  		goto out_put_req;
>  	}
>  
> +	if (req->common.ki_flags & IOCB_IOPRIO)
> +		/*
> +		 * The IOCB_IOPRIO flag is set when the user supplied iocb
> +		 * aio_rw_flag field has the RWF_IOPRIO flag set. If so,
> +		 * aio_reqprio is interpreted as a I/O scheduling class and
> +		 * priority.
> +		 */
> +		req->common.ki_ioprio = iocb->aio_reqprio;

Do we need any validation of the field here?

The only other thing I am a bit worried about is bloating struct kiocb
with a field for a relatively uncommon feature, but I can't really
see any much better way to pass it.

Re: [PATCH 1/2] fs: add RWF_IOPRIO

[Adam Manzanares]

On 5/2/18 10:32 AM, Christoph Hellwig wrote:
> On Mon, Apr 30, 2018 at 09:57:39AM -0700, adam.manzanares@wdc.com wrote:
>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>
>> This is the per-I/O equivalent of the ioprio_set system call.
>>
>> When the RWF_IOPRIO flag is set then the aio_reqprio field of the iocb
>> is interpreted as an I/O scheduling class and priority.
> 
> I think this belongs into the IOCB_FLAG_* flags namespace for aio_flags
> field as it isn't a field valid for plain read/write.
> 
> Also you probably want to merge both patches as they only really
> make sense together.
> 

I will make the changes and send out a v2.

Re: [PATCH 2/2] fs: Add aio priority support for block_dev

[Adam Manzanares]

On 5/2/18 10:33 AM, Christoph Hellwig wrote:
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -1603,6 +1603,15 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
>>   		goto out_put_req;
>>   	}
>>   
>> +	if (req->common.ki_flags & IOCB_IOPRIO)
>> +		/*
>> +		 * The IOCB_IOPRIO flag is set when the user supplied iocb
>> +		 * aio_rw_flag field has the RWF_IOPRIO flag set. If so,
>> +		 * aio_reqprio is interpreted as a I/O scheduling class and
>> +		 * priority.
>> +		 */
>> +		req->common.ki_ioprio = iocb->aio_reqprio;
> 
> Do we need any validation of the field here?

With this patch the only consumer of the ki_ioprio field is a block 
device, which will eventually hit blk_init_request_from_bio where a
validation of the ioprio is hit.

I was hoping to get a simple patch in for the block device case and then
go back and look at other consumers of the kiocb and add ioprio support.
At that point it may be worth it to pull a check into this code.

> 
> The only other thing I am a bit worried about is bloating struct kiocb
> with a field for a relatively uncommon feature, but I can't really
> see any much better way to pass it.
> 

I'll look more closely at reusing existing fields for the next patch 
submission. I am hoping that the feature will be used more often given 
that WRR for NVME should be coming soon.

Thanks,
Adam