From: Adam Manzanares <adam.manzanares@hgst.com>
To: Tejun Heo <tj@kernel.org>
Cc: <axboe@kernel.dk>, <linux-block@vger.kernel.org>,
<linux-ide@vger.kernel.org>
Subject: Re: [PATCH 1/3] block: Add iocontext priority to request
Date: Fri, 30 Sep 2016 09:02:17 -0700 [thread overview]
Message-ID: <20160930160216.GA13637@hgst.com> (raw)
In-Reply-To: <20160929084059.GB11087@mtj.duckdns.org>
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=
WARNING: multiple messages have this Message-ID (diff)
From: Adam Manzanares <adam.manzanares@hgst.com>
To: Tejun Heo <tj@kernel.org>
Cc: axboe@kernel.dk, linux-block@vger.kernel.org, linux-ide@vger.kernel.org
Subject: Re: [PATCH 1/3] block: Add iocontext priority to request
Date: Fri, 30 Sep 2016 09:02:17 -0700 [thread overview]
Message-ID: <20160930160216.GA13637@hgst.com> (raw)
In-Reply-To: <20160929084059.GB11087@mtj.duckdns.org>
Hello Tejun,
The 09/29/2016 10:40, Tejun Heo wrote:
> 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?
>
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.
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.
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 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.
> Thanks.
>
> --
> tejun
Take care,
Adam
next prev parent reply other threads:[~2016-09-30 16:02 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-27 18:14 [PATCH 0/3] Enabling ATA Command Priorities Adam Manzanares
2016-09-27 18:14 ` 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:40 ` Tejun Heo
2016-09-30 16:02 ` Adam Manzanares [this message]
2016-09-30 16:02 ` Adam Manzanares
2016-10-02 8:53 ` Tejun Heo
2016-10-04 15:49 ` Adam Manzanares
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-27 18:14 ` Adam Manzanares
2016-09-29 8:45 ` Tejun Heo
2016-09-30 16:04 ` Adam Manzanares
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-27 18:14 ` 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-28 3:43 ` Adam Manzanares
2016-09-29 8:48 ` tj
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160930160216.GA13637@hgst.com \
--to=adam.manzanares@hgst.com \
--cc=axboe@kernel.dk \
--cc=linux-block@vger.kernel.org \
--cc=linux-ide@vger.kernel.org \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.