* [PATCH 0/3] Enabling ATA Command Priorities
@ 2016-09-27 18:14 Adam Manzanares
2016-09-27 18:14 ` [PATCH 1/3] block: Add iocontext priority to request Adam Manzanares
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Adam Manzanares @ 2016-09-27 18:14 UTC (permalink / raw)
To: axboe, tj; +Cc: linux-block, linux-ide, Adam Manzanares
This patch builds ATA commands with high priority if the iocontext
of a process is set to real time. The goal of the patch is to
improve tail latencies of workloads that use higher queue depths.
Adam Manzanares (3):
block: Add iocontext priority to request
ata: Enabling ATA Command Priorities
ata: ATA Command Priority Disabled By Default
drivers/ata/libahci.c | 1 +
drivers/ata/libata-core.c | 35 +++++++++++++++++++++-
drivers/ata/libata-scsi.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/ata/libata.h | 2 +-
include/linux/ata.h | 6 ++++
include/linux/libata.h | 27 +++++++++++++++++
6 files changed, 142 insertions(+), 3 deletions(-)
--
2.1.4
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:
This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] block: Add iocontext priority to request 2016-09-27 18:14 [PATCH 0/3] Enabling ATA Command Priorities Adam Manzanares @ 2016-09-27 18:14 ` Adam Manzanares 2016-09-29 8:40 ` Tejun Heo 2016-09-27 18:14 ` [PATCH 2/3] ata: Enabling ATA Command Priorities Adam Manzanares ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Adam Manzanares @ 2016-09-27 18:14 UTC (permalink / raw) To: axboe, tj; +Cc: linux-block, linux-ide, Adam Manzanares Patch adds association between iocontext and a request. Signed-off-by: Adam Manzanares <adam.manzanares@hgst.com> --- block/blk-core.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/block/blk-core.c b/block/blk-core.c index 36c7ac3..9c6d733 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -33,6 +33,7 @@ #include <linux/ratelimit.h> #include <linux/pm_runtime.h> #include <linux/blk-cgroup.h> +#include <linux/ioprio.h> #define CREATE_TRACE_POINTS #include <trace/events/block.h> @@ -1648,6 +1649,7 @@ out: void init_request_from_bio(struct request *req, struct bio *bio) { + struct io_context *ioc = rq_ioc(bio); req->cmd_type = REQ_TYPE_FS; req->cmd_flags |= bio->bi_opf & REQ_COMMON_MASK; @@ -1657,6 +1659,9 @@ void init_request_from_bio(struct request *req, struct bio *bio) req->errors = 0; req->__sector = bio->bi_iter.bi_sector; req->ioprio = bio_prio(bio); + if (ioc) + req->ioprio = ioprio_best(req->ioprio, ioc->ioprio); + blk_rq_bio_prep(req->q, req, bio); } -- 2.1.4 Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer: This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] block: Add iocontext priority to request 2016-09-27 18:14 ` [PATCH 1/3] block: Add iocontext priority to request Adam Manzanares @ 2016-09-29 8:40 ` Tejun Heo 2016-09-30 16:02 ` Adam Manzanares 0 siblings, 1 reply; 14+ messages in thread From: Tejun Heo @ 2016-09-29 8:40 UTC (permalink / raw) To: Adam Manzanares; +Cc: axboe, linux-block, linux-ide Hello, On Tue, Sep 27, 2016 at 11:14:54AM -0700, Adam Manzanares wrote: > Patch adds association between iocontext and a request. > > Signed-off-by: Adam Manzanares <adam.manzanares@hgst.com> Can you please describe how this may impact existing usages? Thanks. -- tejun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] block: Add iocontext priority to request 2016-09-29 8:40 ` Tejun Heo @ 2016-09-30 16:02 ` Adam Manzanares 2016-10-02 8:53 ` Tejun Heo 0 siblings, 1 reply; 14+ messages in thread From: Adam Manzanares @ 2016-09-30 16:02 UTC (permalink / raw) To: Tejun Heo; +Cc: axboe, linux-block, linux-ide SGVsbG8gVGVqdW4sCgpUaGUgMDkvMjkvMjAxNiAxMDo0MCwgVGVqdW4gSGVvIHdyb3RlOgo+IEhl bGxvLAo+IAo+IE9uIFR1ZSwgU2VwIDI3LCAyMDE2IGF0IDExOjE0OjU0QU0gLTA3MDAsIEFkYW0g TWFuemFuYXJlcyB3cm90ZToKPiA+IFBhdGNoIGFkZHMgYXNzb2NpYXRpb24gYmV0d2VlbiBpb2Nv bnRleHQgYW5kIGEgcmVxdWVzdC4KPiA+IAo+ID4gU2lnbmVkLW9mZi1ieTogQWRhbSBNYW56YW5h cmVzIDxhZGFtLm1hbnphbmFyZXNAaGdzdC5jb20+Cj4gCj4gQ2FuIHlvdSBwbGVhc2UgZGVzY3Jp YmUgaG93IHRoaXMgbWF5IGltcGFjdCBleGlzdGluZyB1c2FnZXM/Cj4KSSdsbCBzdGFydCB3aXRo IHRoZSBjaGFuZ2VzIEkgbWFkZSBhbmQgd29yayBteSB3YXkgdGhyb3VnaCBhIGdyZXAgb2YgICAg ICAgICAgICAKaW9wcmlvLiBQbGVhc2UgYWRkIG9yIGNvcnJlY3QgYW55IG9mIHRoZSBhc3N1bXB0 aW9ucyBJIGhhdmUgbWFkZS4gICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAK SW4gYmxrLWNvcmUsIHRoZSBiZWhhdmlvciBiZWZvcmUgdGhlIHBhdGNoIGlzIHRvIGdldCB0aGUg aW9wcmlvIGZvciB0aGUgcmVxdWVzdCAKZnJvbSB0aGUgYmlvLiBUaGUgb25seSByZWZlcmVuY2Vz IEkgZm91bmQgdG8gYmlvX3NldF9wcmlvIGFyZSBpbiBiY2FjaGUuIEJvdGggICAKb2YgdGhlc2Ug cmVmZXJlbmNlcyBhcmUgaW4gbG93IHByaW9yaXR5IG9wZXJhdGlvbnMgKGdjLCBiZyB3cml0ZWJh Y2spIHNvIHRoZSAgICAKaW9wcmlvcml0eSBvZiB0aGUgYmlvIGlzIHNldCB0byBJT19QUklPQ0xB U1NfSURMRSBpbiB0aGVzZSBjYXNlcy4gICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAKQSBrZXJuZWwgdGhyZWFkIGlzIHVzZWQgdG8gc3VibWl0IHRoZXNlIGJpb3Mgc28gdGhl IGlvcHJpbyBpcyBnb2luZyB0byBjb21lICAgICAKZnJvbSB0aGUgY3VycmVudCBydW5uaW5nIHRh c2sgaWYgdGhlIGlvY29udGV4dCBleGlzdHMuIFRoaXMgY291bGQgYmUgYSBwcm9ibGVtICAKaWYg d2UgaGF2ZSBzZXQgYSB0YXNrIHdpdGggaGlnaCBwcmlvcml0eSBhbmQgc29tZSBiYWNrZ3JvdW5k IHdvcmsgZW5kcyB1cCAgICAgICAKZ2V0dGluZyBnZW5lcmF0ZWQgaW4gdGhlIGJjYWNoZSBsYXll ci4gSSBwcm9wb3NlIHRoYXQgd2UgY2hlY2sgaWYgdGhlICAgICAgICAgICAKaW9wcmlvcml0eSBv ZiB0aGUgYmlvIGlzIHZhbGlkIGFuZCBpZiBzbywgdGhlbiB3ZSBrZWVwIHRoZSBwcmlvcmlydHkg ZnJvbSB0aGUgICAKYmlvLiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAKVGhlIHNlY29uZCBhcmVhIHRoYXQgSSBzZWUgYSBwb3RlbnRpYWwgcHJvYmxlbSBpcyBpbiB0 aGUgbWVyZ2luZyBjb2RlIGNvZGUgaW4gICAKYmxrLWNvcmUgd2hlbiBhIGJpbyBpcyBxdWV1ZWQu IElmIHRoZXJlIGlzIGEgcmVxdWVzdCB0aGF0IGlzIG1lcmdlYWJsZSB0aGVuICAgICAKdGhlIG1l cmdlIGNvZGUgdGFrZXMgdGhlIGhpZ2hlc3QgcHJpb3JpdHkgb2YgdGhlIGJpbyBhbmQgdGhlIHJl cXVlc3QuIFRoaXMgICAgICAKY291bGQgd2lwZSBvdXQgdGhlIHZhbHVlcyBzZXQgYnkgYmlvX3Nl dF9wcmlvLiBJIHRoaW5rIGl0IHdvdWxkIGJlICAgICAgICAgICAgICAKYmVzdCB0byBzZXQgdGhl IHJlcXVlc3QgYXMgbm9uIG1lcmdlYWJsZSB3aGVuIHdlIHNlZSB0aGF0IGl0IGlzIGEgaGlnaCAg ICAgICAgICAKcHJpb3JpdHkgSU8gcmVxdWVzdC4gICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAK VGhlIHRoaXJkIGFyZWEgdGhhdCBpcyBvZiBpbnRlcmVzdCBpcyBpbiB0aGUgQ0ZRIHNjaGVkdWxl ciBhbmQgdGhlIGlvcHJpbyBpcyAgICAKb25seSB1c2VkIGluIHRoZSBjYXNlIG9mIGFzeW5jIElP IGFuZCBJIGZvdW5kIHRoYXQgdGhlIHByaW9yaXR5IGlzIG9ubHkgICAgICAgICAKb2J0YWluZWQg ZnJvbSB0aGUgdGFzayBhbmQgbm90IGZyb20gdGhlIHJlcXVlc3QuIFRoaXMgbGVhZHMgbWUgdG8g YmVsaWV2ZSB0aGF0ICAKdGhlIGNoYW5nZXMgbWFkZSBpbiB0aGUgYmxrLWNvcmUgdG8gYWRkIHRo ZSBwcmlvcml0eSB0byB0aGUgcmVxdWVzdCB3aWxsIG5vdCAgICAKaW1wYWN0IHRoZSBDRlEgc2No ZWR1bGVyLiAKClRoZSBmb3VydGggYXJlYSB0aGF0IG1pZ2h0IGJlIGNvbmNlcm5pbmcgaXMgdGhl IGRyaXZlcnMuIHZpcnRpb19ibG9jayBjb3BpZXMgICAgCnRoZSByZXF1ZXN0IHByaW9yaXR5IGlu dG8gYSB2aXJ0dWFsIGJsb2NrIHJlcXVlc3QuIEkgYW0gYXNzdW1pbmcgdGhhdCB0aGlzICAgICAg CmV2ZW50dWFsbHkgbWFrZXMgaXQgdG8gYW5vdGhlciBkZXZpY2UgZHJpdmVyIHNvIHdlIGRvbid0 IG5lZWQgdG8gd29ycnkgYWJvdXQgICAgCnRoaXMuIG51bGwgYmxvY2sgZGV2aWNlIGRyaXZlciBh bHNvIHVzZXMgdGhlIGlvcHJpbywgYnV0IHRoaXMgaXMgYWxzbyBub3QgYSAgICAgCmNvbmNlcm4u IGxpZ2h0bnZtIGFsc28gc2V0cyB0aGUgaW9wcmlvIHRvIGJ1aWxkIGEgcmVxdWVzdCB0aGF0IGlz IHBhc3NlZCBvbnRvICAgCmFub3RoZXIgZHJpdmVyLiBUaGUgbGFzdCBkcml2ZXIgdGhhdCB1c2Vz IHRoZSByZXF1ZXN0IGlvcHJpbyBpcyB0aGUgZnVzaW9uICAgICAgCm1wdHNhcyBkcml2ZXIgYW5k IEkgZG9uJ3QgdW5kZXJzdGFuZCBob3cgaXQgaXMgdXNpbmcgdGhlIGlvcHJpby4gRnJvbSB3aGF0 IEkgICAgCmNhbiB0ZWxsIGl0IGlzIHRha2luZyBhIHJlcXVlc3Qgb2YgSU9QUklPX0NMQVNTX05P TkUgd2l0aCBkYXRhIG9mIDB4NyBhbmQgICAgICAgCmNhbGxpbmcgdGhpcyBoaWdoIHByaW9yaXR5 IElPLiBUaGlzIGNvdWxkIGJlIGltcGFjdGVkIGJ5IHRoZSBjb2RlIEkgaGF2ZSAgICAgICAgCnBy b3Bvc2VkLCBidXQgSSBiZWxpZXZlIHRoZSBhdXRob3JzIGludGVuZGVkIHRvIHRyZWF0IHRoaXMg cGFydGljdWxhciBpb3ByaW8gICAgCnZhbHVlIGFzIGhpZ2ggcHJpb3JpdHkuIFRoZSBkcml2ZXIg d2lsbCBwYXNzIHRoZSByZXF1ZXN0IHRvIHRoZSBkZXZpY2UgICAgICAgICAgCndpdGggaGlnaCBw cmlvcml0eSBpZiB0aGUgYXBwcm9wcmlhdGUgaW9wcmlvIHZhbHVlcyBpcyBzZWVuIG9uIHRoZSBy ZXF1ZXN0LiAgICAgCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgClRoZSBmaWZ0aCBhcmVhIHRoYXQg SSBub3RpY2VkIG1heSBiZSBpbXBhY3RlZCBpcyBmaWxlIHN5c3RlbXMuIGJ0cmZzIHVzZXMgbG93 ICAgCnByaW9yaXR5IElPIGZvciByZWFkIGFoZWFkLiBFeHQ0IHVzZXMgaW9wcmlvIGZvciBqb3Vy bmFsaW5nLiBCb3RoIG9mIHRoZXNlICAgICAgCmlzc3VlcyBhcmUgbm90IGEgcHJvYmxlbSBiZWNh dXNlIHRoZSBpb3ByaW8gaXMgc2V0IG9uIHRoZSB0YXNrIGFuZCBub3Qgb24gYSAgICAgCmJpby4K Cj4gVGhhbmtzLgo+IAo+IC0tIAo+IHRlanVuCgpUYWtlIGNhcmUsCkFkYW0KV2VzdGVybiBEaWdp dGFsIENvcnBvcmF0aW9uIChhbmQgaXRzIHN1YnNpZGlhcmllcykgRS1tYWlsIENvbmZpZGVudGlh bGl0eSBOb3RpY2UgJiBEaXNjbGFpbWVyOgoKVGhpcyBlLW1haWwgYW5kIGFueSBmaWxlcyB0cmFu c21pdHRlZCB3aXRoIGl0IG1heSBjb250YWluIGNvbmZpZGVudGlhbCBvciBsZWdhbGx5IHByaXZp bGVnZWQgaW5mb3JtYXRpb24gb2YgV0RDIGFuZC9vciBpdHMgYWZmaWxpYXRlcywgYW5kIGFyZSBp bnRlbmRlZCBzb2xlbHkgZm9yIHRoZSB1c2Ugb2YgdGhlIGluZGl2aWR1YWwgb3IgZW50aXR5IHRv IHdoaWNoIHRoZXkgYXJlIGFkZHJlc3NlZC4gSWYgeW91IGFyZSBub3QgdGhlIGludGVuZGVkIHJl Y2lwaWVudCwgYW55IGRpc2Nsb3N1cmUsIGNvcHlpbmcsIGRpc3RyaWJ1dGlvbiBvciBhbnkgYWN0 aW9uIHRha2VuIG9yIG9taXR0ZWQgdG8gYmUgdGFrZW4gaW4gcmVsaWFuY2Ugb24gaXQsIGlzIHBy b2hpYml0ZWQuIElmIHlvdSBoYXZlIHJlY2VpdmVkIHRoaXMgZS1tYWlsIGluIGVycm9yLCBwbGVh c2Ugbm90aWZ5IHRoZSBzZW5kZXIgaW1tZWRpYXRlbHkgYW5kIGRlbGV0ZSB0aGUgZS1tYWlsIGlu IGl0cyBlbnRpcmV0eSBmcm9tIHlvdXIgc3lzdGVtLgo= ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] block: Add iocontext priority to request 2016-09-30 16:02 ` Adam Manzanares @ 2016-10-02 8:53 ` Tejun Heo 2016-10-04 15:49 ` Adam Manzanares 0 siblings, 1 reply; 14+ messages in thread From: Tejun Heo @ 2016-10-02 8:53 UTC (permalink / raw) To: Adam Manzanares; +Cc: axboe, linux-block, linux-ide Hello, Adam. On Fri, Sep 30, 2016 at 09:02:17AM -0700, Adam Manzanares wrote: > I'll start with the changes I made and work my way through a grep of > ioprio. Please add or correct any of the assumptions I have made. Well, it looks like you're the one who's most familiar with ioprio handling at this point. :) > In blk-core, the behavior before the patch is to get the ioprio for the request > from the bio. The only references I found to bio_set_prio are in bcache. Both > of these references are in low priority operations (gc, bg writeback) so the > iopriority of the bio is set to IO_PRIOCLASS_IDLE in these cases. > > A kernel thread is used to submit these bios so the ioprio is going to come > from the current running task if the iocontext exists. This could be a problem > if we have set a task with high priority and some background work ends up > getting generated in the bcache layer. I propose that we check if the > iopriority of the bio is valid and if so, then we keep the priorirty from the > bio. I wonder whether the right thing to do is adding bio->bi_ioprio which is initialized on bio submission and carried through req->ioprio. > The second area that I see a potential problem is in the merging code code in > blk-core when a bio is queued. If there is a request that is mergeable then > the merge code takes the highest priority of the bio and the request. This > could wipe out the values set by bio_set_prio. I think it would be > best to set the request as non mergeable when we see that it is a high > priority IO request. The current behavior should be fine for most non-pathological cases but I have no objection to not merging ios with differing priorities. > The third area that is of interest is in the CFQ scheduler and the ioprio is > only used in the case of async IO and I found that the priority is only > obtained from the task and not from the request. This leads me to believe that > the changes made in the blk-core to add the priority to the request will not > impact the CFQ scheduler. > > The fourth area that might be concerning is the drivers. virtio_block copies > the request priority into a virtual block request. I am assuming that this > eventually makes it to another device driver so we don't need to worry about > this. null block device driver also uses the ioprio, but this is also not a > concern. lightnvm also sets the ioprio to build a request that is passed onto > another driver. The last driver that uses the request ioprio is the fusion > mptsas driver and I don't understand how it is using the ioprio. From what I > can tell it is taking a request of IOPRIO_CLASS_NONE with data of 0x7 and > calling this high priority IO. This could be impacted by the code I have > proposed, but I believe the authors intended to treat this particular ioprio > value as high priority. The driver will pass the request to the device > with high priority if the appropriate ioprio values is seen on the request. > > The fifth area that I noticed may be impacted is file systems. btrfs uses low > priority IO for read ahead. Ext4 uses ioprio for journaling. Both of these > issues are not a problem because the ioprio is set on the task and not on a > bio. Yeah, looks good to me. Care to include a brief summary of expected (non)impacts in the patch description? Thanks. -- tejun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] block: Add iocontext priority to request 2016-10-02 8:53 ` Tejun Heo @ 2016-10-04 15:49 ` Adam Manzanares 2016-10-04 20:52 ` Tejun Heo 0 siblings, 1 reply; 14+ messages in thread From: Adam Manzanares @ 2016-10-04 15:49 UTC (permalink / raw) To: Tejun Heo; +Cc: axboe, linux-block, linux-ide SGVsbG8gVGVqdW4sCgoxMC8wMi8yMDE2IDEwOjUzLCBUZWp1biBIZW8gd3JvdGU6Cj4gSGVsbG8s IEFkYW0uCj4gCj4gT24gRnJpLCBTZXAgMzAsIDIwMTYgYXQgMDk6MDI6MTdBTSAtMDcwMCwgQWRh bSBNYW56YW5hcmVzIHdyb3RlOgo+ID4gSSdsbCBzdGFydCB3aXRoIHRoZSBjaGFuZ2VzIEkgbWFk ZSBhbmQgd29yayBteSB3YXkgdGhyb3VnaCBhIGdyZXAgb2YgICAgICAgICAgICAKPiA+IGlvcHJp by4gUGxlYXNlIGFkZCBvciBjb3JyZWN0IGFueSBvZiB0aGUgYXNzdW1wdGlvbnMgSSBoYXZlIG1h ZGUuICAgICAgICAgICAgICAgCj4gCj4gV2VsbCwgaXQgbG9va3MgbGlrZSB5b3UncmUgdGhlIG9u ZSB3aG8ncyBtb3N0IGZhbWlsaWFyIHdpdGggaW9wcmlvCj4gaGFuZGxpbmcgYXQgdGhpcyBwb2lu dC4gOikKPiAKPiA+IEluIGJsay1jb3JlLCB0aGUgYmVoYXZpb3IgYmVmb3JlIHRoZSBwYXRjaCBp cyB0byBnZXQgdGhlIGlvcHJpbyBmb3IgdGhlIHJlcXVlc3QgCj4gPiBmcm9tIHRoZSBiaW8uIFRo ZSBvbmx5IHJlZmVyZW5jZXMgSSBmb3VuZCB0byBiaW9fc2V0X3ByaW8gYXJlIGluIGJjYWNoZS4g Qm90aCAgIAo+ID4gb2YgdGhlc2UgcmVmZXJlbmNlcyBhcmUgaW4gbG93IHByaW9yaXR5IG9wZXJh dGlvbnMgKGdjLCBiZyB3cml0ZWJhY2spIHNvIHRoZSAgICAKPiA+IGlvcHJpb3JpdHkgb2YgdGhl IGJpbyBpcyBzZXQgdG8gSU9fUFJJT0NMQVNTX0lETEUgaW4gdGhlc2UgY2FzZXMuICAgICAgICAg ICAgICAgCj4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAo+ID4gQSBrZXJuZWwgdGhyZWFkIGlz IHVzZWQgdG8gc3VibWl0IHRoZXNlIGJpb3Mgc28gdGhlIGlvcHJpbyBpcyBnb2luZyB0byBjb21l ICAgICAKPiA+IGZyb20gdGhlIGN1cnJlbnQgcnVubmluZyB0YXNrIGlmIHRoZSBpb2NvbnRleHQg ZXhpc3RzLiBUaGlzIGNvdWxkIGJlIGEgcHJvYmxlbSAgCj4gPiBpZiB3ZSBoYXZlIHNldCBhIHRh c2sgd2l0aCBoaWdoIHByaW9yaXR5IGFuZCBzb21lIGJhY2tncm91bmQgd29yayBlbmRzIHVwICAg ICAgIAo+ID4gZ2V0dGluZyBnZW5lcmF0ZWQgaW4gdGhlIGJjYWNoZSBsYXllci4gSSBwcm9wb3Nl IHRoYXQgd2UgY2hlY2sgaWYgdGhlICAgICAgICAgICAKPiA+IGlvcHJpb3JpdHkgb2YgdGhlIGJp byBpcyB2YWxpZCBhbmQgaWYgc28sIHRoZW4gd2Uga2VlcCB0aGUgcHJpb3JpcnR5IGZyb20gdGhl ICAgCj4gPiBiaW8uICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIAo+IAo+IEkgd29uZGVyIHdoZXRoZXIgdGhl IHJpZ2h0IHRoaW5nIHRvIGRvIGlzIGFkZGluZyBiaW8tPmJpX2lvcHJpbyB3aGljaAo+IGlzIGlu aXRpYWxpemVkIG9uIGJpbyBzdWJtaXNzaW9uIGFuZCBjYXJyaWVkIHRocm91Z2ggcmVxLT5pb3By aW8uCj4gCgpJIGxvb2tlZCBhcm91bmQgYW5kIHRob3VnaHQgYWJvdXQgdGhpcyBhbmQgSSdtIG5v dCBzdXJlIGlmIHRoaXMgd2lsbCBoZWxwLiAKSSBkdWcgaW50byB0aGUgYmlvIHN1Ym1pc3Npb24g Y29kZSBhbmQgSSB0aG91Z2h0IGdlbmVyaWNfbWFrZV9yZXF1ZXN0IHdhcyAKdGhlIGJlc3QgcGxh Y2UgdG8gc2F2ZSB0aGUgaW9wcmlvIGluZm9ybWF0aW9uLiBUaGlzIGlzIHF1aXRlIGNsb3NlIGlu IAp0aGUgY2FsbCBzdGFjayB0byBpbml0X3JlcXVlc3RfZnJvbSBiaW8uIEJjYWNoZSBzZXRzIHRo ZSBiaW8gcHJpb3JpdHkgYmVmb3JlIAp0aGUgc3VibWlzc2lvbiwgc28gd2Ugd291bGQgaGF2ZSB0 byBjaGVjayB0byBzZWUgaWYgdGhlIGJpbyBwcmlvcml0eSB3YXMgCnZhbGlkIG9uIGJpbyBzdWJt aXNzaW9uIGxlYXZpbmcgdXMgd2l0aCB0aGUgc2FtZSBwcm9ibGVtLiBMZWF2aW5nIHRoZSAKcHJp b3JpdHkgaW4gdGhlIHVwcGVyIGJpdHMgb2YgYmlvLT5iaV9ydyBpcyBmaW5lIHdpdGggbWUuIEl0 IG1heSBoZWxwIHRvIApoYXZlIHRoZSBiaW8tPmJpX2lvcHJpbyBmb3IgY2xhcml0eSwgYnV0IEkg dGhpbmsgd2Ugd2lsbCBzdGlsbCBmYWNlIHRoZSAKaXNzdWUgb2YgaGF2aW5nIHRvIGNoZWNrIGlm IHRoaXMgdmFsdWUgaXMgc2V0IHdoZW4gd2Ugc3VibWl0IHRoZSBiaW8gb3IgCmluaXQgdGhlIHJl cXVlc3Qgc28gSSdtIGxlYW5pbmcgdG93YXJkcyBsZWF2aW5nIGl0IGFzIGlzLgoKCj4gPiBUaGUg c2Vjb25kIGFyZWEgdGhhdCBJIHNlZSBhIHBvdGVudGlhbCBwcm9ibGVtIGlzIGluIHRoZSBtZXJn aW5nIGNvZGUgY29kZSBpbiAgIAo+ID4gYmxrLWNvcmUgd2hlbiBhIGJpbyBpcyBxdWV1ZWQuIElm IHRoZXJlIGlzIGEgcmVxdWVzdCB0aGF0IGlzIG1lcmdlYWJsZSB0aGVuICAgICAKPiA+IHRoZSBt ZXJnZSBjb2RlIHRha2VzIHRoZSBoaWdoZXN0IHByaW9yaXR5IG9mIHRoZSBiaW8gYW5kIHRoZSBy ZXF1ZXN0LiBUaGlzICAgICAgCj4gPiBjb3VsZCB3aXBlIG91dCB0aGUgdmFsdWVzIHNldCBieSBi aW9fc2V0X3ByaW8uIEkgdGhpbmsgaXQgd291bGQgYmUgICAgICAgICAgICAgIAo+ID4gYmVzdCB0 byBzZXQgdGhlIHJlcXVlc3QgYXMgbm9uIG1lcmdlYWJsZSB3aGVuIHdlIHNlZSB0aGF0IGl0IGlz IGEgaGlnaCAgICAgICAgICAKPiA+IHByaW9yaXR5IElPIHJlcXVlc3QuICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgCj4gCj4gVGhlIGN1 cnJlbnQgYmVoYXZpb3Igc2hvdWxkIGJlIGZpbmUgZm9yIG1vc3Qgbm9uLXBhdGhvbG9naWNhbCBj YXNlcwo+IGJ1dCBJIGhhdmUgbm8gb2JqZWN0aW9uIHRvIG5vdCBtZXJnaW5nIGlvcyB3aXRoIGRp ZmZlcmluZyBwcmlvcml0aWVzLgo+IAo+ID4gVGhlIHRoaXJkIGFyZWEgdGhhdCBpcyBvZiBpbnRl cmVzdCBpcyBpbiB0aGUgQ0ZRIHNjaGVkdWxlciBhbmQgdGhlIGlvcHJpbyBpcyAgICAKPiA+IG9u bHkgdXNlZCBpbiB0aGUgY2FzZSBvZiBhc3luYyBJTyBhbmQgSSBmb3VuZCB0aGF0IHRoZSBwcmlv cml0eSBpcyBvbmx5ICAgICAgICAgCj4gPiBvYnRhaW5lZCBmcm9tIHRoZSB0YXNrIGFuZCBub3Qg ZnJvbSB0aGUgcmVxdWVzdC4gVGhpcyBsZWFkcyBtZSB0byBiZWxpZXZlIHRoYXQgIAo+ID4gdGhl IGNoYW5nZXMgbWFkZSBpbiB0aGUgYmxrLWNvcmUgdG8gYWRkIHRoZSBwcmlvcml0eSB0byB0aGUg cmVxdWVzdCB3aWxsIG5vdCAgICAKPiA+IGltcGFjdCB0aGUgQ0ZRIHNjaGVkdWxlci4gCj4gPgo+ ID4gVGhlIGZvdXJ0aCBhcmVhIHRoYXQgbWlnaHQgYmUgY29uY2VybmluZyBpcyB0aGUgZHJpdmVy cy4gdmlydGlvX2Jsb2NrIGNvcGllcyAgICAKPiA+IHRoZSByZXF1ZXN0IHByaW9yaXR5IGludG8g YSB2aXJ0dWFsIGJsb2NrIHJlcXVlc3QuIEkgYW0gYXNzdW1pbmcgdGhhdCB0aGlzICAgICAgCj4g PiBldmVudHVhbGx5IG1ha2VzIGl0IHRvIGFub3RoZXIgZGV2aWNlIGRyaXZlciBzbyB3ZSBkb24n dCBuZWVkIHRvIHdvcnJ5IGFib3V0ICAgIAo+ID4gdGhpcy4gbnVsbCBibG9jayBkZXZpY2UgZHJp dmVyIGFsc28gdXNlcyB0aGUgaW9wcmlvLCBidXQgdGhpcyBpcyBhbHNvIG5vdCBhICAgICAKPiA+ IGNvbmNlcm4uIGxpZ2h0bnZtIGFsc28gc2V0cyB0aGUgaW9wcmlvIHRvIGJ1aWxkIGEgcmVxdWVz dCB0aGF0IGlzIHBhc3NlZCBvbnRvICAgCj4gPiBhbm90aGVyIGRyaXZlci4gVGhlIGxhc3QgZHJp dmVyIHRoYXQgdXNlcyB0aGUgcmVxdWVzdCBpb3ByaW8gaXMgdGhlIGZ1c2lvbiAgICAgIAo+ID4g bXB0c2FzIGRyaXZlciBhbmQgSSBkb24ndCB1bmRlcnN0YW5kIGhvdyBpdCBpcyB1c2luZyB0aGUg aW9wcmlvLiBGcm9tIHdoYXQgSSAgICAKPiA+IGNhbiB0ZWxsIGl0IGlzIHRha2luZyBhIHJlcXVl c3Qgb2YgSU9QUklPX0NMQVNTX05PTkUgd2l0aCBkYXRhIG9mIDB4NyBhbmQgICAgICAgCj4gPiBj YWxsaW5nIHRoaXMgaGlnaCBwcmlvcml0eSBJTy4gVGhpcyBjb3VsZCBiZSBpbXBhY3RlZCBieSB0 aGUgY29kZSBJIGhhdmUgICAgICAgIAo+ID4gcHJvcG9zZWQsIGJ1dCBJIGJlbGlldmUgdGhlIGF1 dGhvcnMgaW50ZW5kZWQgdG8gdHJlYXQgdGhpcyBwYXJ0aWN1bGFyIGlvcHJpbyAgICAKPiA+IHZh bHVlIGFzIGhpZ2ggcHJpb3JpdHkuIFRoZSBkcml2ZXIgd2lsbCBwYXNzIHRoZSByZXF1ZXN0IHRv IHRoZSBkZXZpY2UgICAgICAgICAgCj4gPiB3aXRoIGhpZ2ggcHJpb3JpdHkgaWYgdGhlIGFwcHJv cHJpYXRlIGlvcHJpbyB2YWx1ZXMgaXMgc2VlbiBvbiB0aGUgcmVxdWVzdC4gICAgIAo+ID4gICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAKPiA+IFRoZSBmaWZ0aCBhcmVhIHRoYXQgSSBub3RpY2VkIG1h eSBiZSBpbXBhY3RlZCBpcyBmaWxlIHN5c3RlbXMuIGJ0cmZzIHVzZXMgbG93ICAgCj4gPiBwcmlv cml0eSBJTyBmb3IgcmVhZCBhaGVhZC4gRXh0NCB1c2VzIGlvcHJpbyBmb3Igam91cm5hbGluZy4g Qm90aCBvZiB0aGVzZSAgICAgIAo+ID4gaXNzdWVzIGFyZSBub3QgYSBwcm9ibGVtIGJlY2F1c2Ug dGhlIGlvcHJpbyBpcyBzZXQgb24gdGhlIHRhc2sgYW5kIG5vdCBvbiBhICAgICAKPiA+IGJpby4K PiAKPiBZZWFoLCBsb29rcyBnb29kIHRvIG1lLiAgQ2FyZSB0byBpbmNsdWRlIGEgYnJpZWYgc3Vt bWFyeSBvZiBleHBlY3RlZAo+IChub24paW1wYWN0cyBpbiB0aGUgcGF0Y2ggZGVzY3JpcHRpb24/ Cj4gCkknbSBnb2luZyB0byBzZW5kIG91dCBhbiB1cGRhdGVkIHNlcmllcyBvZiBwYXRjaGVzIHN1 bW1hcml6aW5nIHNvbWUgb2YgdGhpcyAKZGlzY3Vzc2lvbiBhbmQgSSdsbCBhbHNvIGluY2x1ZGUg c29tZSBwZXJmb3JtYW5jZSByZXN1bHRzIHRoYXQgd2UgaGF2ZSAKbWVhc3VyZWQuCgo+IFRoYW5r cy4KPiAKPiAtLSAKPiB0ZWp1bgoKVGFrZSBjYXJlLApBZGFtCldlc3Rlcm4gRGlnaXRhbCBDb3Jw b3JhdGlvbiAoYW5kIGl0cyBzdWJzaWRpYXJpZXMpIEUtbWFpbCBDb25maWRlbnRpYWxpdHkgTm90 aWNlICYgRGlzY2xhaW1lcjoKClRoaXMgZS1tYWlsIGFuZCBhbnkgZmlsZXMgdHJhbnNtaXR0ZWQg d2l0aCBpdCBtYXkgY29udGFpbiBjb25maWRlbnRpYWwgb3IgbGVnYWxseSBwcml2aWxlZ2VkIGlu Zm9ybWF0aW9uIG9mIFdEQyBhbmQvb3IgaXRzIGFmZmlsaWF0ZXMsIGFuZCBhcmUgaW50ZW5kZWQg c29sZWx5IGZvciB0aGUgdXNlIG9mIHRoZSBpbmRpdmlkdWFsIG9yIGVudGl0eSB0byB3aGljaCB0 aGV5IGFyZSBhZGRyZXNzZWQuIElmIHlvdSBhcmUgbm90IHRoZSBpbnRlbmRlZCByZWNpcGllbnQs IGFueSBkaXNjbG9zdXJlLCBjb3B5aW5nLCBkaXN0cmlidXRpb24gb3IgYW55IGFjdGlvbiB0YWtl biBvciBvbWl0dGVkIHRvIGJlIHRha2VuIGluIHJlbGlhbmNlIG9uIGl0LCBpcyBwcm9oaWJpdGVk LiBJZiB5b3UgaGF2ZSByZWNlaXZlZCB0aGlzIGUtbWFpbCBpbiBlcnJvciwgcGxlYXNlIG5vdGlm eSB0aGUgc2VuZGVyIGltbWVkaWF0ZWx5IGFuZCBkZWxldGUgdGhlIGUtbWFpbCBpbiBpdHMgZW50 aXJldHkgZnJvbSB5b3VyIHN5c3RlbS4K ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] block: Add iocontext priority to request 2016-10-04 15:49 ` Adam Manzanares @ 2016-10-04 20:52 ` Tejun Heo 0 siblings, 0 replies; 14+ messages in thread From: Tejun Heo @ 2016-10-04 20:52 UTC (permalink / raw) To: Adam Manzanares; +Cc: axboe, linux-block, linux-ide Hello, Adam. On Tue, Oct 04, 2016 at 08:49:18AM -0700, Adam Manzanares wrote: > > I wonder whether the right thing to do is adding bio->bi_ioprio which > > is initialized on bio submission and carried through req->ioprio. > > I looked around and thought about this and I'm not sure if this will help. > I dug into the bio submission code and I thought generic_make_request was > the best place to save the ioprio information. This is quite close in > the call stack to init_request_from bio. Bcache sets the bio priority before > the submission, so we would have to check to see if the bio priority was > valid on bio submission leaving us with the same problem. Leaving the > priority in the upper bits of bio->bi_rw is fine with me. It may help to > have the bio->bi_ioprio for clarity, but I think we will still face the > issue of having to check if this value is set when we submit the bio or > init the request so I'm leaning towards leaving it as is. I see. Thanks for looking into it. It's icky that we don't have a clear path of propagating ioprio but let's save that for another day. -- tejun ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] ata: Enabling ATA Command Priorities 2016-09-27 18:14 [PATCH 0/3] Enabling ATA Command Priorities Adam Manzanares 2016-09-27 18:14 ` [PATCH 1/3] block: Add iocontext priority to request Adam Manzanares @ 2016-09-27 18:14 ` Adam Manzanares 2016-09-29 8:45 ` Tejun Heo 2016-09-27 18:14 ` [PATCH 3/3] ata: ATA Command Priority Disabled By Default Adam Manzanares 2016-09-28 2:06 ` [PATCH 0/3] Enabling ATA Command Priorities Christoph Hellwig 3 siblings, 1 reply; 14+ messages in thread From: Adam Manzanares @ 2016-09-27 18:14 UTC (permalink / raw) To: axboe, tj; +Cc: linux-block, linux-ide, Adam Manzanares This patch checks to see if an ATA device supports NCQ command priorities. If so and the user has specified an iocontext that indicates IO_PRIO_CLASS_RT then we build a tf with a high priority command. Signed-off-by: Adam Manzanares <adam.manzanares@hgst.com> --- drivers/ata/libata-core.c | 35 ++++++++++++++++++++++++++++++++++- drivers/ata/libata-scsi.c | 6 +++++- drivers/ata/libata.h | 2 +- include/linux/ata.h | 6 ++++++ include/linux/libata.h | 19 +++++++++++++++++++ 5 files changed, 65 insertions(+), 3 deletions(-) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 223a770..181b530 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -739,6 +739,7 @@ u64 ata_tf_read_block(const struct ata_taskfile *tf, struct ata_device *dev) * @n_block: Number of blocks * @tf_flags: RW/FUA etc... * @tag: tag + * @class: IO priority class * * LOCKING: * None. @@ -753,7 +754,7 @@ u64 ata_tf_read_block(const struct ata_taskfile *tf, struct ata_device *dev) */ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev, u64 block, u32 n_block, unsigned int tf_flags, - unsigned int tag) + unsigned int tag, int class) { tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; tf->flags |= tf_flags; @@ -785,6 +786,12 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev, tf->device = ATA_LBA; if (tf->flags & ATA_TFLAG_FUA) tf->device |= 1 << 7; + + if (ata_ncq_prio_enabled(dev)) { + if (class == IOPRIO_CLASS_RT) + tf->hob_nsect |= ATA_PRIO_HIGH << + ATA_SHIFT_PRIO; + } } else if (dev->flags & ATA_DFLAG_LBA) { tf->flags |= ATA_TFLAG_LBA; @@ -2156,6 +2163,30 @@ static void ata_dev_config_ncq_non_data(struct ata_device *dev) } } +static void ata_dev_config_ncq_prio(struct ata_device *dev) +{ + struct ata_port *ap = dev->link->ap; + unsigned int err_mask; + + err_mask = ata_read_log_page(dev, + ATA_LOG_SATA_ID_DEV_DATA, + ATA_LOG_SATA_SETTINGS, + ap->sector_buf, + 1); + if (err_mask) { + ata_dev_dbg(dev, + "failed to get Identify Device data, Emask 0x%x\n", + err_mask); + return; + } + + if (ap->sector_buf[ATA_LOG_NCQ_PRIO_OFFSET] & BIT(3)) + dev->flags |= ATA_DFLAG_NCQ_PRIO; + else + ata_dev_dbg(dev, "SATA page does not support priority\n"); + +} + static int ata_dev_config_ncq(struct ata_device *dev, char *desc, size_t desc_sz) { @@ -2205,6 +2236,8 @@ static int ata_dev_config_ncq(struct ata_device *dev, ata_dev_config_ncq_send_recv(dev); if (ata_id_has_ncq_non_data(dev->id)) ata_dev_config_ncq_non_data(dev); + if (ata_id_has_ncq_prio(dev->id)) + ata_dev_config_ncq_prio(dev); } return 0; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index e207b33..18629e8 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -50,6 +50,7 @@ #include <linux/uaccess.h> #include <linux/suspend.h> #include <asm/unaligned.h> +#include <linux/ioprio.h> #include "libata.h" #include "libata-transport.h" @@ -1757,6 +1758,8 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc) { struct scsi_cmnd *scmd = qc->scsicmd; const u8 *cdb = scmd->cmnd; + struct request *rq = scmd->request; + int class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq)); unsigned int tf_flags = 0; u64 block; u32 n_block; @@ -1823,7 +1826,8 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc) qc->nbytes = n_block * scmd->device->sector_size; rc = ata_build_rw_tf(&qc->tf, qc->dev, block, n_block, tf_flags, - qc->tag); + qc->tag, class); + if (likely(rc == 0)) return 0; diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 3b301a4..8f3a559 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -66,7 +66,7 @@ extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf); extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag); extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev, u64 block, u32 n_block, unsigned int tf_flags, - unsigned int tag); + unsigned int tag, int class); extern u64 ata_tf_read_block(const struct ata_taskfile *tf, struct ata_device *dev); extern unsigned ata_exec_internal(struct ata_device *dev, diff --git a/include/linux/ata.h b/include/linux/ata.h index adbc812..ebe4c3b 100644 --- a/include/linux/ata.h +++ b/include/linux/ata.h @@ -347,6 +347,7 @@ enum { ATA_LOG_DEVSLP_DETO = 0x01, ATA_LOG_DEVSLP_VALID = 0x07, ATA_LOG_DEVSLP_VALID_MASK = 0x80, + ATA_LOG_NCQ_PRIO_OFFSET = 0x09, /* NCQ send and receive log */ ATA_LOG_NCQ_SEND_RECV_SUBCMDS_OFFSET = 0x00, @@ -897,6 +898,11 @@ static inline bool ata_id_has_ncq_non_data(const u16 *id) return id[ATA_ID_SATA_CAPABILITY_2] & BIT(5); } +static inline bool ata_id_has_ncq_prio(const u16 *id) +{ + return id[ATA_ID_SATA_CAPABILITY] & BIT(12); +} + static inline bool ata_id_has_trim(const u16 *id) { if (ata_id_major_version(id) >= 7 && diff --git a/include/linux/libata.h b/include/linux/libata.h index e37d4f9..a3c66852 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -165,6 +165,7 @@ enum { ATA_DFLAG_NO_UNLOAD = (1 << 17), /* device doesn't support unload */ ATA_DFLAG_UNLOCK_HPA = (1 << 18), /* unlock HPA */ ATA_DFLAG_NCQ_SEND_RECV = (1 << 19), /* device supports NCQ SEND and RECV */ + ATA_DFLAG_NCQ_PRIO = (1 << 20), /* device supports NCQ priority */ ATA_DFLAG_INIT_MASK = (1 << 24) - 1, ATA_DFLAG_DETACH = (1 << 24), @@ -341,7 +342,9 @@ enum { ATA_SHIFT_PIO = 0, ATA_SHIFT_MWDMA = ATA_SHIFT_PIO + ATA_NR_PIO_MODES, ATA_SHIFT_UDMA = ATA_SHIFT_MWDMA + ATA_NR_MWDMA_MODES, + ATA_SHIFT_PRIO = 6, + ATA_PRIO_HIGH = 2, /* size of buffer to pad xfers ending on unaligned boundaries */ ATA_DMA_PAD_SZ = 4, @@ -1609,6 +1612,22 @@ static inline int ata_ncq_enabled(struct ata_device *dev) ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ; } +/** + * ata_ncq_prio_enabled - Test whether NCQ prio is enabled + * @dev: ATA device to test for + * + * LOCKING: + * spin_lock_irqsave(host lock) + * + * RETURNS: + * 1 if NCQ prio is enabled for @dev, 0 otherwise. + */ +static inline int ata_ncq_prio_enabled(struct ata_device *dev) +{ + return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF | + ATA_DFLAG_NCQ_PRIO)) == ATA_DFLAG_NCQ_PRIO; +} + static inline bool ata_fpdma_dsm_supported(struct ata_device *dev) { return (dev->flags & ATA_DFLAG_NCQ_SEND_RECV) && -- 2.1.4 Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer: This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] ata: Enabling ATA Command Priorities 2016-09-27 18:14 ` [PATCH 2/3] ata: Enabling ATA Command Priorities Adam Manzanares @ 2016-09-29 8:45 ` Tejun Heo 2016-09-30 16:04 ` Adam Manzanares 0 siblings, 1 reply; 14+ messages in thread From: Tejun Heo @ 2016-09-29 8:45 UTC (permalink / raw) To: Adam Manzanares; +Cc: axboe, linux-block, linux-ide Hello, On Tue, Sep 27, 2016 at 11:14:55AM -0700, Adam Manzanares wrote: > +/** > + * ata_ncq_prio_enabled - Test whether NCQ prio is enabled > + * @dev: ATA device to test for > + * > + * LOCKING: > + * spin_lock_irqsave(host lock) > + * > + * RETURNS: > + * 1 if NCQ prio is enabled for @dev, 0 otherwise. > + */ > +static inline int ata_ncq_prio_enabled(struct ata_device *dev) > +{ > + return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF | > + ATA_DFLAG_NCQ_PRIO)) == ATA_DFLAG_NCQ_PRIO; I'm not sure this needs to test PIO and NCQ_OFF. This functions pretty much can assume that it'd be only called in NCQ context, no? Thanks. -- tejun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] ata: Enabling ATA Command Priorities 2016-09-29 8:45 ` Tejun Heo @ 2016-09-30 16:04 ` Adam Manzanares 0 siblings, 0 replies; 14+ messages in thread From: Adam Manzanares @ 2016-09-30 16:04 UTC (permalink / raw) To: Tejun Heo; +Cc: axboe, linux-block, linux-ide The 09/29/2016 10:45, Tejun Heo wrote: > Hello, > > On Tue, Sep 27, 2016 at 11:14:55AM -0700, Adam Manzanares wrote: > > +/** > > + * ata_ncq_prio_enabled - Test whether NCQ prio is enabled > > + * @dev: ATA device to test for > > + * > > + * LOCKING: > > + * spin_lock_irqsave(host lock) > > + * > > + * RETURNS: > > + * 1 if NCQ prio is enabled for @dev, 0 otherwise. > > + */ > > +static inline int ata_ncq_prio_enabled(struct ata_device *dev) > > +{ > > + return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF | > > + ATA_DFLAG_NCQ_PRIO)) == ATA_DFLAG_NCQ_PRIO; > > I'm not sure this needs to test PIO and NCQ_OFF. This functions > pretty much can assume that it'd be only called in NCQ context, no? > This should only be called in the NCQ context so these checks are redundant I'll clean this up in the next version of the patches. > Thanks. > > -- > tejun Take care, Adam ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] ata: ATA Command Priority Disabled By Default 2016-09-27 18:14 [PATCH 0/3] Enabling ATA Command Priorities Adam Manzanares 2016-09-27 18:14 ` [PATCH 1/3] block: Add iocontext priority to request Adam Manzanares 2016-09-27 18:14 ` [PATCH 2/3] ata: Enabling ATA Command Priorities Adam Manzanares @ 2016-09-27 18:14 ` Adam Manzanares 2016-09-28 2:06 ` [PATCH 0/3] Enabling ATA Command Priorities Christoph Hellwig 3 siblings, 0 replies; 14+ messages in thread From: Adam Manzanares @ 2016-09-27 18:14 UTC (permalink / raw) To: axboe, tj; +Cc: linux-block, linux-ide, Adam Manzanares Add a sysfs entry to turn on priority information being passed to a ATA device. By default this feature is turned off. This patch depends on ata: Enabling ATA Command Priorities Signed-off-by: Adam Manzanares <adam.manzanares@hgst.com> --- drivers/ata/libahci.c | 1 + drivers/ata/libata-core.c | 2 +- drivers/ata/libata-scsi.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/libata.h | 8 ++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index dcf2c72..383adf7 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -140,6 +140,7 @@ EXPORT_SYMBOL_GPL(ahci_shost_attrs); struct device_attribute *ahci_sdev_attrs[] = { &dev_attr_sw_activity, &dev_attr_unload_heads, + &dev_attr_enable_prio, NULL }; EXPORT_SYMBOL_GPL(ahci_sdev_attrs); diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 181b530..d0cf987 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -787,7 +787,7 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev, if (tf->flags & ATA_TFLAG_FUA) tf->device |= 1 << 7; - if (ata_ncq_prio_enabled(dev)) { + if (ata_ncq_prio_enabled(dev) && ata_prio_enabled(dev)) { if (class == IOPRIO_CLASS_RT) tf->hob_nsect |= ATA_PRIO_HIGH << ATA_SHIFT_PRIO; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 18629e8..10ba118 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -271,6 +271,73 @@ DEVICE_ATTR(unload_heads, S_IRUGO | S_IWUSR, ata_scsi_park_show, ata_scsi_park_store); EXPORT_SYMBOL_GPL(dev_attr_unload_heads); +static ssize_t ata_enable_prio_show(struct device *device, + struct device_attribute *attr, char *buf) +{ + struct scsi_device *sdev = to_scsi_device(device); + struct ata_port *ap; + struct ata_device *dev; + int rc = 0; + int enable_prio; + + ap = ata_shost_to_port(sdev->host); + + spin_lock_irq(ap->lock); + dev = ata_scsi_find_dev(ap, sdev); + if (!dev) { + rc = -ENODEV; + goto unlock; + } + + enable_prio = ata_prio_enabled(dev); + +unlock: + spin_unlock_irq(ap->lock); + + return rc ? rc : snprintf(buf, 20, "%u\n", enable_prio); +} + +static ssize_t ata_enable_prio_store(struct device *device, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct scsi_device *sdev = to_scsi_device(device); + struct ata_port *ap; + struct ata_device *dev; + long int input; + unsigned long flags; + int rc; + + rc = kstrtol(buf, 10, &input); + if (rc) + return rc; + if ((input < 0) || (input > 1)) + return -EINVAL; + + ap = ata_shost_to_port(sdev->host); + + spin_lock_irqsave(ap->lock, flags); + dev = ata_scsi_find_dev(ap, sdev); + if (unlikely(!dev)) { + rc = -ENODEV; + goto unlock; + } + + if (input) + dev->flags |= ATA_DFLAG_ENABLE_PRIO; + else + dev->flags &= ~ATA_DFLAG_ENABLE_PRIO; + +unlock: + spin_unlock_irqrestore(ap->lock, flags); + + return rc ? rc : len; +} + +DEVICE_ATTR(enable_prio, S_IRUGO | S_IWUSR, + ata_enable_prio_show, ata_enable_prio_store); +EXPORT_SYMBOL_GPL(dev_attr_enable_prio); + void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq) { @@ -402,6 +469,7 @@ EXPORT_SYMBOL_GPL(dev_attr_sw_activity); struct device_attribute *ata_common_sdev_attrs[] = { &dev_attr_unload_heads, + &dev_attr_enable_prio, NULL }; EXPORT_SYMBOL_GPL(ata_common_sdev_attrs); diff --git a/include/linux/libata.h b/include/linux/libata.h index a3c66852..804c4c6 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -166,6 +166,7 @@ enum { ATA_DFLAG_UNLOCK_HPA = (1 << 18), /* unlock HPA */ ATA_DFLAG_NCQ_SEND_RECV = (1 << 19), /* device supports NCQ SEND and RECV */ ATA_DFLAG_NCQ_PRIO = (1 << 20), /* device supports NCQ priority */ + ATA_DFLAG_ENABLE_PRIO = (1 << 21), /* User enable device priority */ ATA_DFLAG_INIT_MASK = (1 << 24) - 1, ATA_DFLAG_DETACH = (1 << 24), @@ -544,6 +545,7 @@ typedef void (*ata_postreset_fn_t)(struct ata_link *link, unsigned int *classes) extern struct device_attribute dev_attr_link_power_management_policy; extern struct device_attribute dev_attr_unload_heads; +extern struct device_attribute dev_attr_enable_prio; extern struct device_attribute dev_attr_em_message_type; extern struct device_attribute dev_attr_em_message; extern struct device_attribute dev_attr_sw_activity; @@ -1628,6 +1630,12 @@ static inline int ata_ncq_prio_enabled(struct ata_device *dev) ATA_DFLAG_NCQ_PRIO)) == ATA_DFLAG_NCQ_PRIO; } +static inline int ata_prio_enabled(struct ata_device *dev) +{ + return ((dev->flags & ATA_DFLAG_ENABLE_PRIO) == + ATA_DFLAG_ENABLE_PRIO); +} + static inline bool ata_fpdma_dsm_supported(struct ata_device *dev) { return (dev->flags & ATA_DFLAG_NCQ_SEND_RECV) && -- 2.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] Enabling ATA Command Priorities 2016-09-27 18:14 [PATCH 0/3] Enabling ATA Command Priorities Adam Manzanares ` (2 preceding siblings ...) 2016-09-27 18:14 ` [PATCH 3/3] ata: ATA Command Priority Disabled By Default Adam Manzanares @ 2016-09-28 2:06 ` Christoph Hellwig 2016-09-28 3:43 ` Adam Manzanares 3 siblings, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2016-09-28 2:06 UTC (permalink / raw) To: Adam Manzanares; +Cc: axboe, tj, linux-block, linux-ide The series looks fine to me: Reviewed-by: Christoph Hellwig <hch@lst.de> The only question is if we need to bother with the last patch to make the feature conditional at all, given that we both need hardware support and applications opting into using I/O priorities to even use it. But if you feel it's safer that way the unable certainly doesn't hurt. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] Enabling ATA Command Priorities 2016-09-28 2:06 ` [PATCH 0/3] Enabling ATA Command Priorities Christoph Hellwig @ 2016-09-28 3:43 ` Adam Manzanares 2016-09-29 8:48 ` tj 0 siblings, 1 reply; 14+ messages in thread From: Adam Manzanares @ 2016-09-28 3:43 UTC (permalink / raw) To: Christoph Hellwig Cc: axboe@kernel.dk, tj@kernel.org, linux-block@vger.kernel.org, linux-ide@vger.kernel.org SSBwcmVmZXIgaGF2aW5nIHRoZSBmZWF0dXJlIGNvbmRpdGlvbmFsIHNvIHlvdSBjYW4gdXNlIHRo ZSBDRlEgc2NoZWR1bGVyIHdpdGggSS9PIHByaW9yaXRpZXMgYXMgaXMuIElmIHlvdSBkZWNpZGUg dG8gZW5hYmxlIHRoZSBmZWF0dXJlIHRoZW4gdGhlIHByaW9yaXRpZXMgd2lsbCBiZSBwYXNzZWQg ZG93biB0byB0aGUgZHJpdmUgaW4gYWRkaXRpb24gdG8gdGhlIHdvcmsgdGhhdCB0aGUgQ0ZRIHNj aGVkdWxlciBkb2VzLiBTaW5jZSB0aGlzIGZlYXR1cmUgbWF5IGNoYW5nZSB0aGUgdXNlciBwZXJj ZWl2ZWQgcGVyZm9ybWFuY2Ugb2YgdGhlIGRldmljZSBJIHdhbnQgdG8gbWFrZSBzdXJlIHRoZXkg a25vdyB3aGF0IHRoZXkgYXJlIGdldHRpbmcgaW50by4NCg0KDQoNCg0KDQpPbiA5LzI3LzE2LCA3 OjA2IFBNLCAiQ2hyaXN0b3BoIEhlbGx3aWciIDxoY2hAaW5mcmFkZWFkLm9yZz4gd3JvdGU6DQoN Cj5UaGUgc2VyaWVzIGxvb2tzIGZpbmUgdG8gbWU6DQo+DQo+UmV2aWV3ZWQtYnk6IENocmlzdG9w aCBIZWxsd2lnIDxoY2hAbHN0LmRlPg0KPg0KPlRoZSBvbmx5IHF1ZXN0aW9uIGlzIGlmIHdlIG5l ZWQgdG8gYm90aGVyIHdpdGggdGhlIGxhc3QgcGF0Y2ggdG8NCj5tYWtlIHRoZSBmZWF0dXJlIGNv bmRpdGlvbmFsIGF0IGFsbCwgZ2l2ZW4gdGhhdCB3ZSBib3RoIG5lZWQgaGFyZHdhcmUNCj5zdXBw b3J0IGFuZCBhcHBsaWNhdGlvbnMgb3B0aW5nIGludG8gdXNpbmcgSS9PIHByaW9yaXRpZXMgdG8g ZXZlbiB1c2UNCj5pdC4gIEJ1dCBpZiB5b3UgZmVlbCBpdCdzIHNhZmVyIHRoYXQgd2F5IHRoZSB1 bmFibGUgY2VydGFpbmx5IGRvZXNuJ3QNCj5odXJ0Lg0KV2VzdGVybiBEaWdpdGFsIENvcnBvcmF0 aW9uIChhbmQgaXRzIHN1YnNpZGlhcmllcykgRS1tYWlsIENvbmZpZGVudGlhbGl0eSBOb3RpY2Ug JiBEaXNjbGFpbWVyOgoKVGhpcyBlLW1haWwgYW5kIGFueSBmaWxlcyB0cmFuc21pdHRlZCB3aXRo IGl0IG1heSBjb250YWluIGNvbmZpZGVudGlhbCBvciBsZWdhbGx5IHByaXZpbGVnZWQgaW5mb3Jt YXRpb24gb2YgV0RDIGFuZC9vciBpdHMgYWZmaWxpYXRlcywgYW5kIGFyZSBpbnRlbmRlZCBzb2xl bHkgZm9yIHRoZSB1c2Ugb2YgdGhlIGluZGl2aWR1YWwgb3IgZW50aXR5IHRvIHdoaWNoIHRoZXkg YXJlIGFkZHJlc3NlZC4gSWYgeW91IGFyZSBub3QgdGhlIGludGVuZGVkIHJlY2lwaWVudCwgYW55 IGRpc2Nsb3N1cmUsIGNvcHlpbmcsIGRpc3RyaWJ1dGlvbiBvciBhbnkgYWN0aW9uIHRha2VuIG9y IG9taXR0ZWQgdG8gYmUgdGFrZW4gaW4gcmVsaWFuY2Ugb24gaXQsIGlzIHByb2hpYml0ZWQuIElm IHlvdSBoYXZlIHJlY2VpdmVkIHRoaXMgZS1tYWlsIGluIGVycm9yLCBwbGVhc2Ugbm90aWZ5IHRo ZSBzZW5kZXIgaW1tZWRpYXRlbHkgYW5kIGRlbGV0ZSB0aGUgZS1tYWlsIGluIGl0cyBlbnRpcmV0 eSBmcm9tIHlvdXIgc3lzdGVtLgo= ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] Enabling ATA Command Priorities 2016-09-28 3:43 ` Adam Manzanares @ 2016-09-29 8:48 ` tj 0 siblings, 0 replies; 14+ messages in thread From: tj @ 2016-09-29 8:48 UTC (permalink / raw) To: Adam Manzanares Cc: Christoph Hellwig, axboe@kernel.dk, linux-block@vger.kernel.org, linux-ide@vger.kernel.org Hello, On Wed, Sep 28, 2016 at 03:43:32AM +0000, Adam Manzanares wrote: > I prefer having the feature conditional so you can use the CFQ > scheduler with I/O priorities as is. If you decide to enable the > feature then the priorities will be passed down to the drive in > addition to the work that the CFQ scheduler does. Since this feature > may change the user perceived performance of the device I want to > make sure they know what they are getting into. Yeah, I prefer to have it behind an explicit flag given the history of optional ATA features. The feature is unlikely to matter to a lot of people and is almost bound to break existing RT prio usages. Thanks. -- tejun ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-10-04 20:52 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-27 18:14 [PATCH 0/3] Enabling ATA Command Priorities Adam Manzanares 2016-09-27 18:14 ` [PATCH 1/3] block: Add iocontext priority to request Adam Manzanares 2016-09-29 8:40 ` Tejun Heo 2016-09-30 16:02 ` Adam Manzanares 2016-10-02 8:53 ` Tejun Heo 2016-10-04 15:49 ` Adam Manzanares 2016-10-04 20:52 ` Tejun Heo 2016-09-27 18:14 ` [PATCH 2/3] ata: Enabling ATA Command Priorities Adam Manzanares 2016-09-29 8:45 ` Tejun Heo 2016-09-30 16:04 ` Adam Manzanares 2016-09-27 18:14 ` [PATCH 3/3] ata: ATA Command Priority Disabled By Default Adam Manzanares 2016-09-28 2:06 ` [PATCH 0/3] Enabling ATA Command Priorities Christoph Hellwig 2016-09-28 3:43 ` Adam Manzanares 2016-09-29 8:48 ` tj
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).