[PATCH v2] fs: Add aio iopriority support for block_dev

[PATCH v2] fs: Add aio iopriority support for block_dev

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

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.

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

Given that WRR support for NVME devices has patches floating around and it was
discussed at LSFMM, we may soon have a lower latency storage device that can 
make use of iopriorities. A per command iopriority interface seems timely 
given these developments. 

If we want to avoid bloating struct kiocb, I suggest we turn the private field 
into a union of the private and ki_ioprio field. It seems like the users of 
the private field all use it at a point where we can yank the priority from 
the kiocb before the private field is used. Comments and suggestions welcome.

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

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 fs/aio.c                     | 10 ++++++++++
 fs/block_dev.c               |  2 ++
 include/linux/fs.h           |  2 ++
 include/uapi/linux/aio_abi.h |  1 +
 4 files changed, 15 insertions(+)

diff --git a/fs/aio.c b/fs/aio.c
index 88d7927ffbc6..f36636d8ff2c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1597,6 +1597,16 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 		req->common.ki_flags |= IOCB_EVENTFD;
 	}
 
+	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.
+		 */
+		req->common.ki_ioprio = iocb->aio_reqprio;
+		req->common.ki_flags |= IOCB_IOPRIO;
+	}
+
 	ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
 	if (unlikely(ret)) {
 		pr_debug("EINVAL: aio_rw_flags\n");
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7ec920e27065..970bef79caa6 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -355,6 +355,8 @@ __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;
+		if (iocb->ki_flags & IOCB_IOPRIO)
+			bio->bi_ioprio = iocb->ki_ioprio;
 
 		ret = bio_iov_iter_get_pages(bio, iter);
 		if (unlikely(ret)) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..ab63ce720305 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)
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index a04adbc70ddf..d4593a6062ef 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -54,6 +54,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 v2] fs: Add aio iopriority support for block_dev

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

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.

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

Given that WRR support for NVME devices has patches floating around and it was
discussed at LSFMM, we may soon have a lower latency storage device that can 
make use of iopriorities. A per command iopriority interface seems timely 
given these developments. 

If we want to avoid bloating struct kiocb, I suggest we turn the private field 
into a union of the private and ki_ioprio field. It seems like the users of 
the private field all use it at a point where we can yank the priority from 
the kiocb before the private field is used. Comments and suggestions welcome.

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

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 fs/aio.c                     | 10 ++++++++++
 fs/block_dev.c               |  2 ++
 include/linux/fs.h           |  2 ++
 include/uapi/linux/aio_abi.h |  1 +
 4 files changed, 15 insertions(+)

diff --git a/fs/aio.c b/fs/aio.c
index 88d7927ffbc6..f36636d8ff2c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1597,6 +1597,16 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
 		req->common.ki_flags |= IOCB_EVENTFD;
 	}
 
+	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.
+		 */
+		req->common.ki_ioprio = iocb->aio_reqprio;
+		req->common.ki_flags |= IOCB_IOPRIO;
+	}
+
 	ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
 	if (unlikely(ret)) {
 		pr_debug("EINVAL: aio_rw_flags\n");
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7ec920e27065..970bef79caa6 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -355,6 +355,8 @@ __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;
+		if (iocb->ki_flags & IOCB_IOPRIO)
+			bio->bi_ioprio = iocb->ki_ioprio;
 
 		ret = bio_iov_iter_get_pages(bio, iter);
 		if (unlikely(ret)) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..ab63ce720305 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)
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index a04adbc70ddf..d4593a6062ef 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -54,6 +54,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

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Matthew Wilcox]

On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
> If we want to avoid bloating struct kiocb, I suggest we turn the private field 
> into a union of the private and ki_ioprio field. It seems like the users of 
> the private field all use it at a point where we can yank the priority from 
> the kiocb before the private field is used. Comments and suggestions welcome.

Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
32 bits of ki_hint.  (currently defined values are 1-5)

> @@ -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)

--
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 v2] fs: Add aio iopriority support for block_dev

[Matthew Wilcox]

On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
> If we want to avoid bloating struct kiocb, I suggest we turn the private field 
> into a union of the private and ki_ioprio field. It seems like the users of 
> the private field all use it at a point where we can yank the priority from 
> the kiocb before the private field is used. Comments and suggestions welcome.

Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
32 bits of ki_hint.  (currently defined values are 1-5)

> @@ -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)

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Adam Manzanares]

DQoNCk9uIDUvMy8xOCAxMTozMyBBTSwgTWF0dGhldyBXaWxjb3ggd3JvdGU6DQo+IE9uIFRodSwg
TWF5IDAzLCAyMDE4IGF0IDExOjIxOjE0QU0gLTA3MDAsIGFkYW0ubWFuemFuYXJlc0B3ZGMuY29t
IHdyb3RlOg0KPj4gSWYgd2Ugd2FudCB0byBhdm9pZCBibG9hdGluZyBzdHJ1Y3Qga2lvY2IsIEkg
c3VnZ2VzdCB3ZSB0dXJuIHRoZSBwcml2YXRlIGZpZWxkDQo+PiBpbnRvIGEgdW5pb24gb2YgdGhl
IHByaXZhdGUgYW5kIGtpX2lvcHJpbyBmaWVsZC4gSXQgc2VlbXMgbGlrZSB0aGUgdXNlcnMgb2YN
Cj4+IHRoZSBwcml2YXRlIGZpZWxkIGFsbCB1c2UgaXQgYXQgYSBwb2ludCB3aGVyZSB3ZSBjYW4g
eWFuayB0aGUgcHJpb3JpdHkgZnJvbQ0KPj4gdGhlIGtpb2NiIGJlZm9yZSB0aGUgcHJpdmF0ZSBm
aWVsZCBpcyB1c2VkLiBDb21tZW50cyBhbmQgc3VnZ2VzdGlvbnMgd2VsY29tZS4NCj4gDQo+IE9y
IHdlIGNvdWxkIGp1c3QgbWFrZSBraV9oaW50IGEgdTggb3IgdTE2IC4uLiBzZWVtcyB1bmxpa2Vs
eSB3ZSdsbCBuZWVkDQo+IDMyIGJpdHMgb2Yga2lfaGludC4gIChjdXJyZW50bHkgZGVmaW5lZCB2
YWx1ZXMgYXJlIDEtNSkNCg0KSSBsaWtlIHRoZSBhcHByb2FjaCBvZiB1c2luZyBhIHUxNiBmb3Ig
dGhlIGtpX2hpbnQuIEknbGwgdXBkYXRlIGFuZCANCnJlc3VibWl0Lg0KDQo+PiBAQCAtMzAwLDYg
KzMwMSw3IEBAIHN0cnVjdCBraW9jYiB7DQo+PiAgIAl2b2lkCQkJKnByaXZhdGU7DQo+PiAgIAlp
bnQJCQlraV9mbGFnczsNCj4+ICAgCWVudW0gcndfaGludAkJa2lfaGludDsNCj4+ICsJdTE2CQkJ
a2lfaW9wcmlvOyAvKiBTZWUgbGludXgvaW9wcmlvLmggKi8NCj4+ICAgfSBfX3JhbmRvbWl6ZV9s
YXlvdXQ7DQo+PiAgIA0KPj4gICBzdGF0aWMgaW5saW5lIGJvb2wgaXNfc3luY19raW9jYihzdHJ1
Y3Qga2lvY2IgKmtpb2NiKQ==

--
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 v2] fs: Add aio iopriority support for block_dev

[Adam Manzanares]

On 5/3/18 11:33 AM, Matthew Wilcox wrote:
> On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
>> If we want to avoid bloating struct kiocb, I suggest we turn the private field
>> into a union of the private and ki_ioprio field. It seems like the users of
>> the private field all use it at a point where we can yank the priority from
>> the kiocb before the private field is used. Comments and suggestions welcome.
> 
> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
> 32 bits of ki_hint.  (currently defined values are 1-5)

I like the approach of using a u16 for the ki_hint. I'll update and 
resubmit.

>> @@ -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)

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Jens Axboe]

On 5/3/18 2:15 PM, Adam Manzanares wrote:
> 
> 
> On 5/3/18 11:33 AM, Matthew Wilcox wrote:
>> On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
>>> If we want to avoid bloating struct kiocb, I suggest we turn the private field
>>> into a union of the private and ki_ioprio field. It seems like the users of
>>> the private field all use it at a point where we can yank the priority from
>>> the kiocb before the private field is used. Comments and suggestions welcome.
>>
>> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
>> 32 bits of ki_hint.  (currently defined values are 1-5)
> 
> I like the approach of using a u16 for the ki_hint. I'll update and 
> resubmit.

It's intended to be a mask. If you do shrink it for now, then we need some
guard code to ensure it can always carry what it needs to.

-- 
Jens Axboe

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Jens Axboe]

On 5/3/18 2:15 PM, Adam Manzanares wrote:
> 
> 
> On 5/3/18 11:33 AM, Matthew Wilcox wrote:
>> On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
>>> If we want to avoid bloating struct kiocb, I suggest we turn the private field
>>> into a union of the private and ki_ioprio field. It seems like the users of
>>> the private field all use it at a point where we can yank the priority from
>>> the kiocb before the private field is used. Comments and suggestions welcome.
>>
>> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
>> 32 bits of ki_hint.  (currently defined values are 1-5)
> 
> I like the approach of using a u16 for the ki_hint. I'll update and 
> resubmit.

It's intended to be a mask. If you do shrink it for now, then we need some
guard code to ensure it can always carry what it needs to.

-- 
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 v2] fs: Add aio iopriority support for block_dev

[Adam Manzanares]

DQoNCk9uIDUvMy8xOCAxOjI0IFBNLCBKZW5zIEF4Ym9lIHdyb3RlOg0KPiBPbiA1LzMvMTggMjox
NSBQTSwgQWRhbSBNYW56YW5hcmVzIHdyb3RlOg0KPj4NCj4+DQo+PiBPbiA1LzMvMTggMTE6MzMg
QU0sIE1hdHRoZXcgV2lsY294IHdyb3RlOg0KPj4+IE9uIFRodSwgTWF5IDAzLCAyMDE4IGF0IDEx
OjIxOjE0QU0gLTA3MDAsIGFkYW0ubWFuemFuYXJlc0B3ZGMuY29tIHdyb3RlOg0KPj4+PiBJZiB3
ZSB3YW50IHRvIGF2b2lkIGJsb2F0aW5nIHN0cnVjdCBraW9jYiwgSSBzdWdnZXN0IHdlIHR1cm4g
dGhlIHByaXZhdGUgZmllbGQNCj4+Pj4gaW50byBhIHVuaW9uIG9mIHRoZSBwcml2YXRlIGFuZCBr
aV9pb3ByaW8gZmllbGQuIEl0IHNlZW1zIGxpa2UgdGhlIHVzZXJzIG9mDQo+Pj4+IHRoZSBwcml2
YXRlIGZpZWxkIGFsbCB1c2UgaXQgYXQgYSBwb2ludCB3aGVyZSB3ZSBjYW4geWFuayB0aGUgcHJp
b3JpdHkgZnJvbQ0KPj4+PiB0aGUga2lvY2IgYmVmb3JlIHRoZSBwcml2YXRlIGZpZWxkIGlzIHVz
ZWQuIENvbW1lbnRzIGFuZCBzdWdnZXN0aW9ucyB3ZWxjb21lLg0KPj4+DQo+Pj4gT3Igd2UgY291
bGQganVzdCBtYWtlIGtpX2hpbnQgYSB1OCBvciB1MTYgLi4uIHNlZW1zIHVubGlrZWx5IHdlJ2xs
IG5lZWQNCj4+PiAzMiBiaXRzIG9mIGtpX2hpbnQuICAoY3VycmVudGx5IGRlZmluZWQgdmFsdWVz
IGFyZSAxLTUpDQo+Pg0KPj4gSSBsaWtlIHRoZSBhcHByb2FjaCBvZiB1c2luZyBhIHUxNiBmb3Ig
dGhlIGtpX2hpbnQuIEknbGwgdXBkYXRlIGFuZA0KPj4gcmVzdWJtaXQuDQo+IA0KPiBJdCdzIGlu
dGVuZGVkIHRvIGJlIGEgbWFzay4gSWYgeW91IGRvIHNocmluayBpdCBmb3Igbm93LCB0aGVuIHdl
IG5lZWQgc29tZQ0KPiBndWFyZCBjb2RlIHRvIGVuc3VyZSBpdCBjYW4gYWx3YXlzIGNhcnJ5IHdo
YXQgaXQgbmVlZHMgdG8uDQo+IA0KDQpHb3QgaXQsIEknbGwgYWRkIHRoZSBndWFyZCB0byByd19o
aW50X3ZhbGlkIGFsb25nIHdpdGggYSBjb21tZW50IGFib3V0IA0KYmVpbmcgbGltaXRlZCBieSB0
aGUgc2l6ZSBvZiBraV9oaW50IGluIGNhc2Ugd2UgZ2V0IHRvIGEgc2l0dWF0aW9uIHdoZXJlIA0K
MTYgYml0cyBpcyBub3QgZW5vdWdoLg0K

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Adam Manzanares]

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



On 5/3/18 1:24 PM, Jens Axboe wrote:
> On 5/3/18 2:15 PM, Adam Manzanares wrote:
>>
>>
>> On 5/3/18 11:33 AM, Matthew Wilcox wrote:
>>> On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
>>>> If we want to avoid bloating struct kiocb, I suggest we turn the private field
>>>> into a union of the private and ki_ioprio field. It seems like the users of
>>>> the private field all use it at a point where we can yank the priority from
>>>> the kiocb before the private field is used. Comments and suggestions welcome.
>>>
>>> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
>>> 32 bits of ki_hint.  (currently defined values are 1-5)
>>
>> I like the approach of using a u16 for the ki_hint. I'll update and
>> resubmit.
> 
> It's intended to be a mask. If you do shrink it for now, then we need some
> guard code to ensure it can always carry what it needs to.
> 

Got it, I'll add the guard to rw_hint_valid along with a comment about 
being limited by the size of ki_hint in case we get to a situation where 
16 bits is not enough.
N‹§²æìr¸›zǧu©ž²Æ {\b­†éì¹»\x1c®&Þ–)îŨ¨Š{ayº\x1dÊÚ&j:+v‰¨’öœ’Šà\x16Šæ¢·¢ú(œ¸§»\x10\b:Çž†Ûiÿü0ÂKÚrJ+ƒö¢£ðèž×¦j)Z†·Ÿ

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Adam Manzanares]

On 5/3/18 1:24 PM, Jens Axboe wrote:
> On 5/3/18 2:15 PM, Adam Manzanares wrote:
>>
>>
>> On 5/3/18 11:33 AM, Matthew Wilcox wrote:
>>> On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
>>>> If we want to avoid bloating struct kiocb, I suggest we turn the private field
>>>> into a union of the private and ki_ioprio field. It seems like the users of
>>>> the private field all use it at a point where we can yank the priority from
>>>> the kiocb before the private field is used. Comments and suggestions welcome.
>>>
>>> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
>>> 32 bits of ki_hint.  (currently defined values are 1-5)
>>
>> I like the approach of using a u16 for the ki_hint. I'll update and
>> resubmit.
> 
> It's intended to be a mask. If you do shrink it for now, then we need some
> guard code to ensure it can always carry what it needs to.
> 

Got it, I'll add the guard to rw_hint_valid along with a comment about 
being limited by the size of ki_hint in case we get to a situation where 
16 bits is not enough.

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Jens Axboe]

On 5/3/18 2:58 PM, Adam Manzanares wrote:
> 
> 
> On 5/3/18 1:24 PM, Jens Axboe wrote:
>> On 5/3/18 2:15 PM, Adam Manzanares wrote:
>>>
>>>
>>> On 5/3/18 11:33 AM, Matthew Wilcox wrote:
>>>> On Thu, May 03, 2018 at 11:21:14AM -0700, adam.manzanares@wdc.com wrote:
>>>>> If we want to avoid bloating struct kiocb, I suggest we turn the private field
>>>>> into a union of the private and ki_ioprio field. It seems like the users of
>>>>> the private field all use it at a point where we can yank the priority from
>>>>> the kiocb before the private field is used. Comments and suggestions welcome.
>>>>
>>>> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
>>>> 32 bits of ki_hint.  (currently defined values are 1-5)
>>>
>>> I like the approach of using a u16 for the ki_hint. I'll update and
>>> resubmit.
>>
>> It's intended to be a mask. If you do shrink it for now, then we need some
>> guard code to ensure it can always carry what it needs to.
>>
> 
> Got it, I'll add the guard to rw_hint_valid along with a comment about 
> being limited by the size of ki_hint in case we get to a situation where 
> 16 bits is not enough.

Other way around - the API should not be limited by the fact that some
smaller type was chosen for an internal structure. Hence the guard/check
should not be in rw_hint_valid, but rather around where you assign
ki_hint. If/when we do extend the read/write hints, then we'll
potentially need to bump ki_hint to accommodate it.

-- 
Jens Axboe

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Matthew Wilcox]

On Thu, May 03, 2018 at 02:24:58PM -0600, Jens Axboe wrote:
> On 5/3/18 2:15 PM, Adam Manzanares wrote:
> > On 5/3/18 11:33 AM, Matthew Wilcox wrote:
> >> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
> >> 32 bits of ki_hint.  (currently defined values are 1-5)
> > 
> > I like the approach of using a u16 for the ki_hint. I'll update and 
> > resubmit.
> 
> It's intended to be a mask. If you do shrink it for now, then we need some
> guard code to ensure it can always carry what it needs to.

ummm ...

enum rw_hint {
        WRITE_LIFE_NOT_SET      = 0,
        WRITE_LIFE_NONE         = RWH_WRITE_LIFE_NONE,
        WRITE_LIFE_SHORT        = RWH_WRITE_LIFE_SHORT,
...

                .ki_hint = file_write_hint(filp),

static inline enum rw_hint file_write_hint(struct file *file)

#define RWF_WRITE_LIFE_NOT_SET  0
#define RWH_WRITE_LIFE_NONE     1
#define RWH_WRITE_LIFE_SHORT    2
#define RWH_WRITE_LIFE_MEDIUM   3
#define RWH_WRITE_LIFE_LONG     4
#define RWH_WRITE_LIFE_EXTREME  5

It doesn't look like it's being used as a mask.

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Matthew Wilcox]

On Thu, May 03, 2018 at 02:24:58PM -0600, Jens Axboe wrote:
> On 5/3/18 2:15 PM, Adam Manzanares wrote:
> > On 5/3/18 11:33 AM, Matthew Wilcox wrote:
> >> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
> >> 32 bits of ki_hint.  (currently defined values are 1-5)
> > 
> > I like the approach of using a u16 for the ki_hint. I'll update and 
> > resubmit.
> 
> It's intended to be a mask. If you do shrink it for now, then we need some
> guard code to ensure it can always carry what it needs to.

ummm ...

enum rw_hint {
        WRITE_LIFE_NOT_SET      = 0,
        WRITE_LIFE_NONE         = RWH_WRITE_LIFE_NONE,
        WRITE_LIFE_SHORT        = RWH_WRITE_LIFE_SHORT,
...

                .ki_hint = file_write_hint(filp),

static inline enum rw_hint file_write_hint(struct file *file)

#define RWF_WRITE_LIFE_NOT_SET  0
#define RWH_WRITE_LIFE_NONE     1
#define RWH_WRITE_LIFE_SHORT    2
#define RWH_WRITE_LIFE_MEDIUM   3
#define RWH_WRITE_LIFE_LONG     4
#define RWH_WRITE_LIFE_EXTREME  5

It doesn't look like it's being used as a mask.

--
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 v2] fs: Add aio iopriority support for block_dev

[Jens Axboe]

On 5/3/18 4:43 PM, Matthew Wilcox wrote:
> On Thu, May 03, 2018 at 02:24:58PM -0600, Jens Axboe wrote:
>> On 5/3/18 2:15 PM, Adam Manzanares wrote:
>>> On 5/3/18 11:33 AM, Matthew Wilcox wrote:
>>>> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
>>>> 32 bits of ki_hint.  (currently defined values are 1-5)
>>>
>>> I like the approach of using a u16 for the ki_hint. I'll update and 
>>> resubmit.
>>
>> It's intended to be a mask. If you do shrink it for now, then we need some
>> guard code to ensure it can always carry what it needs to.
> 
> ummm ...
> 
> enum rw_hint {
>         WRITE_LIFE_NOT_SET      = 0,
>         WRITE_LIFE_NONE         = RWH_WRITE_LIFE_NONE,
>         WRITE_LIFE_SHORT        = RWH_WRITE_LIFE_SHORT,
> ...
> 
>                 .ki_hint = file_write_hint(filp),
> 
> static inline enum rw_hint file_write_hint(struct file *file)
> 
> #define RWF_WRITE_LIFE_NOT_SET  0
> #define RWH_WRITE_LIFE_NONE     1
> #define RWH_WRITE_LIFE_SHORT    2
> #define RWH_WRITE_LIFE_MEDIUM   3
> #define RWH_WRITE_LIFE_LONG     4
> #define RWH_WRITE_LIFE_EXTREME  5
> 
> It doesn't look like it's being used as a mask.

Right, currently it only supports the life time hint. I'm saying the
intent is, for when it's being expanded to cover other hints, is to
split up the value into maskable sections.

-- 
Jens Axboe

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Jens Axboe]

On 5/3/18 4:43 PM, Matthew Wilcox wrote:
> On Thu, May 03, 2018 at 02:24:58PM -0600, Jens Axboe wrote:
>> On 5/3/18 2:15 PM, Adam Manzanares wrote:
>>> On 5/3/18 11:33 AM, Matthew Wilcox wrote:
>>>> Or we could just make ki_hint a u8 or u16 ... seems unlikely we'll need
>>>> 32 bits of ki_hint.  (currently defined values are 1-5)
>>>
>>> I like the approach of using a u16 for the ki_hint. I'll update and 
>>> resubmit.
>>
>> It's intended to be a mask. If you do shrink it for now, then we need some
>> guard code to ensure it can always carry what it needs to.
> 
> ummm ...
> 
> enum rw_hint {
>         WRITE_LIFE_NOT_SET      = 0,
>         WRITE_LIFE_NONE         = RWH_WRITE_LIFE_NONE,
>         WRITE_LIFE_SHORT        = RWH_WRITE_LIFE_SHORT,
> ...
> 
>                 .ki_hint = file_write_hint(filp),
> 
> static inline enum rw_hint file_write_hint(struct file *file)
> 
> #define RWF_WRITE_LIFE_NOT_SET  0
> #define RWH_WRITE_LIFE_NONE     1
> #define RWH_WRITE_LIFE_SHORT    2
> #define RWH_WRITE_LIFE_MEDIUM   3
> #define RWH_WRITE_LIFE_LONG     4
> #define RWH_WRITE_LIFE_EXTREME  5
> 
> It doesn't look like it's being used as a mask.

Right, currently it only supports the life time hint. I'm saying the
intent is, for when it's being expanded to cover other hints, is to
split up the value into maskable sections.

-- 
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 v2] fs: Add aio iopriority support for block_dev

[Jeff Moyer]

Hi, Adam,

adam.manzanares@wdc.com writes:

> 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.
>
> 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.
>
> See the following link for performance implications on a SATA HDD:
> https://lkml.org/lkml/2016/12/6/495
>
> Given that WRR support for NVME devices has patches floating around and it was
> discussed at LSFMM, we may soon have a lower latency storage device that can 
> make use of iopriorities. A per command iopriority interface seems timely 
> given these developments. 
>
> If we want to avoid bloating struct kiocb, I suggest we turn the private field 
> into a union of the private and ki_ioprio field. It seems like the users of 
> the private field all use it at a point where we can yank the priority from 
> the kiocb before the private field is used. Comments and suggestions welcome.

The ioprio_set system call requires CAP_SYS_ADMIN for setting
IOPRIO_CLASS_RT.  I think we need similar checks here.

-Jeff

>
> v2: merge patches
>     use IOCB_FLAG_IOPRIO
>     validate intended use with IOCB_IOPRIO
>     add linux-api and linux-block to cc
>
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> ---
>  fs/aio.c                     | 10 ++++++++++
>  fs/block_dev.c               |  2 ++
>  include/linux/fs.h           |  2 ++
>  include/uapi/linux/aio_abi.h |  1 +
>  4 files changed, 15 insertions(+)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 88d7927ffbc6..f36636d8ff2c 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1597,6 +1597,16 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
>  		req->common.ki_flags |= IOCB_EVENTFD;
>  	}
>  
> +	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.
> +		 */
> +		req->common.ki_ioprio = iocb->aio_reqprio;
> +		req->common.ki_flags |= IOCB_IOPRIO;
> +	}
> +
>  	ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
>  	if (unlikely(ret)) {
>  		pr_debug("EINVAL: aio_rw_flags\n");
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 7ec920e27065..970bef79caa6 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -355,6 +355,8 @@ __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;
> +		if (iocb->ki_flags & IOCB_IOPRIO)
> +			bio->bi_ioprio = iocb->ki_ioprio;
>  
>  		ret = bio_iov_iter_get_pages(bio, iter);
>  		if (unlikely(ret)) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..ab63ce720305 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)
> diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
> index a04adbc70ddf..d4593a6062ef 100644
> --- a/include/uapi/linux/aio_abi.h
> +++ b/include/uapi/linux/aio_abi.h
> @@ -54,6 +54,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 {

--
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 v2] fs: Add aio iopriority support for block_dev

[Jeff Moyer]

Hi, Adam,

adam.manzanares@wdc.com writes:

> 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.
>
> 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.
>
> See the following link for performance implications on a SATA HDD:
> https://lkml.org/lkml/2016/12/6/495
>
> Given that WRR support for NVME devices has patches floating around and it was
> discussed at LSFMM, we may soon have a lower latency storage device that can 
> make use of iopriorities. A per command iopriority interface seems timely 
> given these developments. 
>
> If we want to avoid bloating struct kiocb, I suggest we turn the private field 
> into a union of the private and ki_ioprio field. It seems like the users of 
> the private field all use it at a point where we can yank the priority from 
> the kiocb before the private field is used. Comments and suggestions welcome.

The ioprio_set system call requires CAP_SYS_ADMIN for setting
IOPRIO_CLASS_RT.  I think we need similar checks here.

-Jeff

>
> v2: merge patches
>     use IOCB_FLAG_IOPRIO
>     validate intended use with IOCB_IOPRIO
>     add linux-api and linux-block to cc
>
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> ---
>  fs/aio.c                     | 10 ++++++++++
>  fs/block_dev.c               |  2 ++
>  include/linux/fs.h           |  2 ++
>  include/uapi/linux/aio_abi.h |  1 +
>  4 files changed, 15 insertions(+)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 88d7927ffbc6..f36636d8ff2c 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1597,6 +1597,16 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
>  		req->common.ki_flags |= IOCB_EVENTFD;
>  	}
>  
> +	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.
> +		 */
> +		req->common.ki_ioprio = iocb->aio_reqprio;
> +		req->common.ki_flags |= IOCB_IOPRIO;
> +	}
> +
>  	ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
>  	if (unlikely(ret)) {
>  		pr_debug("EINVAL: aio_rw_flags\n");
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 7ec920e27065..970bef79caa6 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -355,6 +355,8 @@ __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;
> +		if (iocb->ki_flags & IOCB_IOPRIO)
> +			bio->bi_ioprio = iocb->ki_ioprio;
>  
>  		ret = bio_iov_iter_get_pages(bio, iter);
>  		if (unlikely(ret)) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..ab63ce720305 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)
> diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
> index a04adbc70ddf..d4593a6062ef 100644
> --- a/include/uapi/linux/aio_abi.h
> +++ b/include/uapi/linux/aio_abi.h
> @@ -54,6 +54,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 {

--
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 v2] fs: Add aio iopriority support for block_dev

[Jeff Moyer]

Hi, Adam,

adam.manzanares@wdc.com writes:

> 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.
>
> 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.
>
> See the following link for performance implications on a SATA HDD:
> https://lkml.org/lkml/2016/12/6/495
>
> Given that WRR support for NVME devices has patches floating around and it was
> discussed at LSFMM, we may soon have a lower latency storage device that can 
> make use of iopriorities. A per command iopriority interface seems timely 
> given these developments. 
>
> If we want to avoid bloating struct kiocb, I suggest we turn the private field 
> into a union of the private and ki_ioprio field. It seems like the users of 
> the private field all use it at a point where we can yank the priority from 
> the kiocb before the private field is used. Comments and suggestions welcome.

The ioprio_set system call requires CAP_SYS_ADMIN for setting
IOPRIO_CLASS_RT.  I think we need similar checks here.

-Jeff

>
> v2: merge patches
>     use IOCB_FLAG_IOPRIO
>     validate intended use with IOCB_IOPRIO
>     add linux-api and linux-block to cc
>
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> ---
>  fs/aio.c                     | 10 ++++++++++
>  fs/block_dev.c               |  2 ++
>  include/linux/fs.h           |  2 ++
>  include/uapi/linux/aio_abi.h |  1 +
>  4 files changed, 15 insertions(+)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 88d7927ffbc6..f36636d8ff2c 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1597,6 +1597,16 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
>  		req->common.ki_flags |= IOCB_EVENTFD;
>  	}
>  
> +	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.
> +		 */
> +		req->common.ki_ioprio = iocb->aio_reqprio;
> +		req->common.ki_flags |= IOCB_IOPRIO;
> +	}
> +
>  	ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
>  	if (unlikely(ret)) {
>  		pr_debug("EINVAL: aio_rw_flags\n");
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 7ec920e27065..970bef79caa6 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -355,6 +355,8 @@ __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;
> +		if (iocb->ki_flags & IOCB_IOPRIO)
> +			bio->bi_ioprio = iocb->ki_ioprio;
>  
>  		ret = bio_iov_iter_get_pages(bio, iter);
>  		if (unlikely(ret)) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 760d8da1b6c7..ab63ce720305 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)
> diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
> index a04adbc70ddf..d4593a6062ef 100644
> --- a/include/uapi/linux/aio_abi.h
> +++ b/include/uapi/linux/aio_abi.h
> @@ -54,6 +54,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 {

Re: [PATCH v2] fs: Add aio iopriority support for block_dev

[Adam Manzanares]

DQoNCk9uIDUvMy8xOCAxMTozNiBBTSwgSmVmZiBNb3llciB3cm90ZToNCj4gSGksIEFkYW0sDQoN
CkhlbGxvIEplZmYsDQoNCj4gDQo+IGFkYW0ubWFuemFuYXJlc0B3ZGMuY29tIHdyaXRlczoNCj4g
DQo+PiBGcm9tOiBBZGFtIE1hbnphbmFyZXMgPGFkYW0ubWFuemFuYXJlc0B3ZGMuY29tPg0KPj4N
Cj4+IFRoaXMgaXMgdGhlIHBlci1JL08gZXF1aXZhbGVudCBvZiB0aGUgaW9wcmlvX3NldCBzeXN0
ZW0gY2FsbC4NCj4+DQo+PiBXaGVuIElPQ0JfRkxBR19JT1BSSU8gaXMgc2V0IG9uIHRoZSBpb2Ni
IGFpb19mbGFncyBmaWVsZCwgdGhlbiB3ZSBzZXQgdGhlDQo+PiBuZXdseSBhZGRlZCBraW9jYiBr
aV9pb3ByaW8gZmllbGQgdG8gdGhlIHZhbHVlIGluIHRoZSBpb2NiIGFpb19yZXFwcmlvIGZpZWxk
Lg0KPj4NCj4+IFdoZW4gYSBiaW8gaXMgY3JlYXRlZCBmb3IgYW4gYWlvIHJlcXVlc3QgYnkgdGhl
IGJsb2NrIGRldiB3ZSBzZXQgdGhlIHByaW9yaXR5DQo+PiB2YWx1ZSBvZiB0aGUgYmlvIHRvIHRo
ZSB1c2VyIHN1cHBsaWVkIHZhbHVlLg0KPj4NCj4+IFNlZSB0aGUgZm9sbG93aW5nIGxpbmsgZm9y
IHBlcmZvcm1hbmNlIGltcGxpY2F0aW9ucyBvbiBhIFNBVEEgSEREOg0KPj4gaHR0cHM6Ly9sa21s
Lm9yZy9sa21sLzIwMTYvMTIvNi80OTUNCj4+DQo+PiBHaXZlbiB0aGF0IFdSUiBzdXBwb3J0IGZv
ciBOVk1FIGRldmljZXMgaGFzIHBhdGNoZXMgZmxvYXRpbmcgYXJvdW5kIGFuZCBpdCB3YXMNCj4+
IGRpc2N1c3NlZCBhdCBMU0ZNTSwgd2UgbWF5IHNvb24gaGF2ZSBhIGxvd2VyIGxhdGVuY3kgc3Rv
cmFnZSBkZXZpY2UgdGhhdCBjYW4NCj4+IG1ha2UgdXNlIG9mIGlvcHJpb3JpdGllcy4gQSBwZXIg
Y29tbWFuZCBpb3ByaW9yaXR5IGludGVyZmFjZSBzZWVtcyB0aW1lbHkNCj4+IGdpdmVuIHRoZXNl
IGRldmVsb3BtZW50cy4NCj4+DQo+PiBJZiB3ZSB3YW50IHRvIGF2b2lkIGJsb2F0aW5nIHN0cnVj
dCBraW9jYiwgSSBzdWdnZXN0IHdlIHR1cm4gdGhlIHByaXZhdGUgZmllbGQNCj4+IGludG8gYSB1
bmlvbiBvZiB0aGUgcHJpdmF0ZSBhbmQga2lfaW9wcmlvIGZpZWxkLiBJdCBzZWVtcyBsaWtlIHRo
ZSB1c2VycyBvZg0KPj4gdGhlIHByaXZhdGUgZmllbGQgYWxsIHVzZSBpdCBhdCBhIHBvaW50IHdo
ZXJlIHdlIGNhbiB5YW5rIHRoZSBwcmlvcml0eSBmcm9tDQo+PiB0aGUga2lvY2IgYmVmb3JlIHRo
ZSBwcml2YXRlIGZpZWxkIGlzIHVzZWQuIENvbW1lbnRzIGFuZCBzdWdnZXN0aW9ucyB3ZWxjb21l
Lg0KPiANCj4gVGhlIGlvcHJpb19zZXQgc3lzdGVtIGNhbGwgcmVxdWlyZXMgQ0FQX1NZU19BRE1J
TiBmb3Igc2V0dGluZw0KPiBJT1BSSU9fQ0xBU1NfUlQuICBJIHRoaW5rIHdlIG5lZWQgc2ltaWxh
ciBjaGVja3MgaGVyZS4NCg0KSSBmb3Jnb3QgaG93IGRhbmdlcm91cyBJT1BSSU9fQ0xBU1NfUlQg
Y2FuIGJlIDopLiBJIHdpbGwgbWFrZSBhIG5ldyANCmZ1bmN0aW9uIGJhc2VkIG9uIHRoZSBjaGVj
a3MgaW4gaW9wcmlvLmMgYW5kIHJldXNlLg0KDQpUaGFua3MsDQpBZGFtDQoNCj4gDQo+IC1KZWZm
DQo+IA0KPj4NCj4+IHYyOiBtZXJnZSBwYXRjaGVzDQo+PiAgICAgIHVzZSBJT0NCX0ZMQUdfSU9Q
UklPDQo+PiAgICAgIHZhbGlkYXRlIGludGVuZGVkIHVzZSB3aXRoIElPQ0JfSU9QUklPDQo+PiAg
ICAgIGFkZCBsaW51eC1hcGkgYW5kIGxpbnV4LWJsb2NrIHRvIGNjDQo+Pg0KPj4gU2lnbmVkLW9m
Zi1ieTogQWRhbSBNYW56YW5hcmVzIDxhZGFtLm1hbnphbmFyZXNAd2RjLmNvbT4NCj4+IC0tLQ0K
Pj4gICBmcy9haW8uYyAgICAgICAgICAgICAgICAgICAgIHwgMTAgKysrKysrKysrKw0KPj4gICBm
cy9ibG9ja19kZXYuYyAgICAgICAgICAgICAgIHwgIDIgKysNCj4+ICAgaW5jbHVkZS9saW51eC9m
cy5oICAgICAgICAgICB8ICAyICsrDQo+PiAgIGluY2x1ZGUvdWFwaS9saW51eC9haW9fYWJpLmgg
fCAgMSArDQo+PiAgIDQgZmlsZXMgY2hhbmdlZCwgMTUgaW5zZXJ0aW9ucygrKQ0KPj4NCj4+IGRp
ZmYgLS1naXQgYS9mcy9haW8uYyBiL2ZzL2Fpby5jDQo+PiBpbmRleCA4OGQ3OTI3ZmZiYzYuLmYz
NjYzNmQ4ZmYyYyAxMDA2NDQNCj4+IC0tLSBhL2ZzL2Fpby5jDQo+PiArKysgYi9mcy9haW8uYw0K
Pj4gQEAgLTE1OTcsNiArMTU5NywxNiBAQCBzdGF0aWMgaW50IGlvX3N1Ym1pdF9vbmUoc3RydWN0
IGtpb2N0eCAqY3R4LCBzdHJ1Y3QgaW9jYiBfX3VzZXIgKnVzZXJfaW9jYiwNCj4+ICAgCQlyZXEt
PmNvbW1vbi5raV9mbGFncyB8PSBJT0NCX0VWRU5URkQ7DQo+PiAgIAl9DQo+PiAgIA0KPj4gKwlp
ZiAoaW9jYi0+YWlvX2ZsYWdzICYgSU9DQl9GTEFHX0lPUFJJTykgew0KPj4gKwkJLyoNCj4+ICsJ
CSAqIElmIHRoZSBJT0NCX0ZMQUdfSU9QUklPIGZsYWcgb2YgYWlvX2ZsYWdzIGlzIHNldCwgdGhl
bg0KPj4gKwkJICogYWlvX3JlcXByaW8gaXMgaW50ZXJwcmV0ZWQgYXMgYW4gSS9PIHNjaGVkdWxp
bmcNCj4+ICsJCSAqIGNsYXNzIGFuZCBwcmlvcml0eS4NCj4+ICsJCSAqLw0KPj4gKwkJcmVxLT5j
b21tb24ua2lfaW9wcmlvID0gaW9jYi0+YWlvX3JlcXByaW87DQo+PiArCQlyZXEtPmNvbW1vbi5r
aV9mbGFncyB8PSBJT0NCX0lPUFJJTzsNCj4+ICsJfQ0KPj4gKw0KPj4gICAJcmV0ID0ga2lvY2Jf
c2V0X3J3X2ZsYWdzKCZyZXEtPmNvbW1vbiwgaW9jYi0+YWlvX3J3X2ZsYWdzKTsNCj4+ICAgCWlm
ICh1bmxpa2VseShyZXQpKSB7DQo+PiAgIAkJcHJfZGVidWcoIkVJTlZBTDogYWlvX3J3X2ZsYWdz
XG4iKTsNCj4+IGRpZmYgLS1naXQgYS9mcy9ibG9ja19kZXYuYyBiL2ZzL2Jsb2NrX2Rldi5jDQo+
PiBpbmRleCA3ZWM5MjBlMjcwNjUuLjk3MGJlZjc5Y2FhNiAxMDA2NDQNCj4+IC0tLSBhL2ZzL2Js
b2NrX2Rldi5jDQo+PiArKysgYi9mcy9ibG9ja19kZXYuYw0KPj4gQEAgLTM1NSw2ICszNTUsOCBA
QCBfX2Jsa2Rldl9kaXJlY3RfSU8oc3RydWN0IGtpb2NiICppb2NiLCBzdHJ1Y3QgaW92X2l0ZXIg
Kml0ZXIsIGludCBucl9wYWdlcykNCj4+ICAgCQliaW8tPmJpX3dyaXRlX2hpbnQgPSBpb2NiLT5r
aV9oaW50Ow0KPj4gICAJCWJpby0+YmlfcHJpdmF0ZSA9IGRpbzsNCj4+ICAgCQliaW8tPmJpX2Vu
ZF9pbyA9IGJsa2Rldl9iaW9fZW5kX2lvOw0KPj4gKwkJaWYgKGlvY2ItPmtpX2ZsYWdzICYgSU9D
Ql9JT1BSSU8pDQo+PiArCQkJYmlvLT5iaV9pb3ByaW8gPSBpb2NiLT5raV9pb3ByaW87DQo+PiAg
IA0KPj4gICAJCXJldCA9IGJpb19pb3ZfaXRlcl9nZXRfcGFnZXMoYmlvLCBpdGVyKTsNCj4+ICAg
CQlpZiAodW5saWtlbHkocmV0KSkgew0KPj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGludXgvZnMu
aCBiL2luY2x1ZGUvbGludXgvZnMuaA0KPj4gaW5kZXggNzYwZDhkYTFiNmM3Li5hYjYzY2U3MjAz
MDUgMTAwNjQ0DQo+PiAtLS0gYS9pbmNsdWRlL2xpbnV4L2ZzLmgNCj4+ICsrKyBiL2luY2x1ZGUv
bGludXgvZnMuaA0KPj4gQEAgLTI5Miw2ICsyOTIsNyBAQCBlbnVtIHJ3X2hpbnQgew0KPj4gICAj
ZGVmaW5lIElPQ0JfU1lOQwkJKDEgPDwgNSkNCj4+ICAgI2RlZmluZSBJT0NCX1dSSVRFCQkoMSA8
PCA2KQ0KPj4gICAjZGVmaW5lIElPQ0JfTk9XQUlUCQkoMSA8PCA3KQ0KPj4gKyNkZWZpbmUgSU9D
Ql9JT1BSSU8JCSgxIDw8IDgpDQo+PiAgIA0KPj4gICBzdHJ1Y3Qga2lvY2Igew0KPj4gICAJc3Ry
dWN0IGZpbGUJCSpraV9maWxwOw0KPj4gQEAgLTMwMCw2ICszMDEsNyBAQCBzdHJ1Y3Qga2lvY2Ig
ew0KPj4gICAJdm9pZAkJCSpwcml2YXRlOw0KPj4gICAJaW50CQkJa2lfZmxhZ3M7DQo+PiAgIAll
bnVtIHJ3X2hpbnQJCWtpX2hpbnQ7DQo+PiArCXUxNgkJCWtpX2lvcHJpbzsgLyogU2VlIGxpbnV4
L2lvcHJpby5oICovDQo+PiAgIH0gX19yYW5kb21pemVfbGF5b3V0Ow0KPj4gICANCj4+ICAgc3Rh
dGljIGlubGluZSBib29sIGlzX3N5bmNfa2lvY2Ioc3RydWN0IGtpb2NiICpraW9jYikNCj4+IGRp
ZmYgLS1naXQgYS9pbmNsdWRlL3VhcGkvbGludXgvYWlvX2FiaS5oIGIvaW5jbHVkZS91YXBpL2xp
bnV4L2Fpb19hYmkuaA0KPj4gaW5kZXggYTA0YWRiYzcwZGRmLi5kNDU5M2E2MDYyZWYgMTAwNjQ0
DQo+PiAtLS0gYS9pbmNsdWRlL3VhcGkvbGludXgvYWlvX2FiaS5oDQo+PiArKysgYi9pbmNsdWRl
L3VhcGkvbGludXgvYWlvX2FiaS5oDQo+PiBAQCAtNTQsNiArNTQsNyBAQCBlbnVtIHsNCj4+ICAg
ICogICAgICAgICAgICAgICAgICAgaXMgdmFsaWQuDQo+PiAgICAqLw0KPj4gICAjZGVmaW5lIElP
Q0JfRkxBR19SRVNGRAkJKDEgPDwgMCkNCj4+ICsjZGVmaW5lIElPQ0JfRkxBR19JT1BSSU8JKDEg
PDwgMSkNCj4+ICAgDQo+PiAgIC8qIHJlYWQoKSBmcm9tIC9kZXYvYWlvIHJldHVybnMgdGhlc2Ug
c3RydWN0dXJlcy4gKi8NCj4+ICAgc3RydWN0IGlvX2V2ZW50IHs=

--
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 v2] fs: Add aio iopriority support for block_dev

[Adam Manzanares]

On 5/3/18 11:36 AM, Jeff Moyer wrote:
> Hi, Adam,

Hello Jeff,

> 
> adam.manzanares@wdc.com writes:
> 
>> 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.
>>
>> 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.
>>
>> See the following link for performance implications on a SATA HDD:
>> https://lkml.org/lkml/2016/12/6/495
>>
>> Given that WRR support for NVME devices has patches floating around and it was
>> discussed at LSFMM, we may soon have a lower latency storage device that can
>> make use of iopriorities. A per command iopriority interface seems timely
>> given these developments.
>>
>> If we want to avoid bloating struct kiocb, I suggest we turn the private field
>> into a union of the private and ki_ioprio field. It seems like the users of
>> the private field all use it at a point where we can yank the priority from
>> the kiocb before the private field is used. Comments and suggestions welcome.
> 
> The ioprio_set system call requires CAP_SYS_ADMIN for setting
> IOPRIO_CLASS_RT.  I think we need similar checks here.

I forgot how dangerous IOPRIO_CLASS_RT can be :). I will make a new 
function based on the checks in ioprio.c and reuse.

Thanks,
Adam

> 
> -Jeff
> 
>>
>> v2: merge patches
>>      use IOCB_FLAG_IOPRIO
>>      validate intended use with IOCB_IOPRIO
>>      add linux-api and linux-block to cc
>>
>> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
>> ---
>>   fs/aio.c                     | 10 ++++++++++
>>   fs/block_dev.c               |  2 ++
>>   include/linux/fs.h           |  2 ++
>>   include/uapi/linux/aio_abi.h |  1 +
>>   4 files changed, 15 insertions(+)
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index 88d7927ffbc6..f36636d8ff2c 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -1597,6 +1597,16 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
>>   		req->common.ki_flags |= IOCB_EVENTFD;
>>   	}
>>   
>> +	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.
>> +		 */
>> +		req->common.ki_ioprio = iocb->aio_reqprio;
>> +		req->common.ki_flags |= IOCB_IOPRIO;
>> +	}
>> +
>>   	ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
>>   	if (unlikely(ret)) {
>>   		pr_debug("EINVAL: aio_rw_flags\n");
>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>> index 7ec920e27065..970bef79caa6 100644
>> --- a/fs/block_dev.c
>> +++ b/fs/block_dev.c
>> @@ -355,6 +355,8 @@ __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;
>> +		if (iocb->ki_flags & IOCB_IOPRIO)
>> +			bio->bi_ioprio = iocb->ki_ioprio;
>>   
>>   		ret = bio_iov_iter_get_pages(bio, iter);
>>   		if (unlikely(ret)) {
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 760d8da1b6c7..ab63ce720305 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)
>> diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
>> index a04adbc70ddf..d4593a6062ef 100644
>> --- a/include/uapi/linux/aio_abi.h
>> +++ b/include/uapi/linux/aio_abi.h
>> @@ -54,6 +54,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 {