From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "tj@kernel.org" <tj@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"peterz@infradead.org" <peterz@infradead.org>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"kernel-team@fb.com" <kernel-team@fb.com>,
"oleg@redhat.com" <oleg@redhat.com>, "hch@lst.de" <hch@lst.de>,
"axboe@kernel.dk" <axboe@kernel.dk>,
"jianchao.w.wang@oracle.com" <jianchao.w.wang@oracle.com>,
"osandov@fb.com" <osandov@fb.com>
Subject: Re: [PATCH 2/6] blk-mq: replace timeout synchronization with a RCU and generation based scheme
Date: Thu, 14 Dec 2017 21:13:32 +0000 [thread overview]
Message-ID: <1513286010.2475.58.camel@wdc.com> (raw)
In-Reply-To: <20171214191935.GB3919388@devbig577.frc2.facebook.com>
T24gVGh1LCAyMDE3LTEyLTE0IGF0IDExOjE5IC0wODAwLCB0akBrZXJuZWwub3JnIHdyb3RlOg0K
PiBPbiBUaHUsIERlYyAxNCwgMjAxNyBhdCAwNjo1MToxMVBNICswMDAwLCBCYXJ0IFZhbiBBc3Nj
aGUgd3JvdGU6DQo+ID4gT24gVHVlLCAyMDE3LTEyLTEyIGF0IDExOjAxIC0wODAwLCBUZWp1biBI
ZW8gd3JvdGU6DQo+ID4gPiAtLS0gYS9ibG9jay9ibGstY29yZS5jDQo+ID4gPiArKysgYi9ibG9j
ay9ibGstY29yZS5jDQo+ID4gPiBAQCAtMTI2LDYgKzEyNiw4IEBAIHZvaWQgYmxrX3JxX2luaXQo
c3RydWN0IHJlcXVlc3RfcXVldWUgKnEsIHN0cnVjdCByZXF1ZXN0ICpycSkNCj4gPiA+ICAJcnEt
PnN0YXJ0X3RpbWUgPSBqaWZmaWVzOw0KPiA+ID4gIAlzZXRfc3RhcnRfdGltZV9ucyhycSk7DQo+
ID4gPiAgCXJxLT5wYXJ0ID0gTlVMTDsNCj4gPiA+ICsJc2VxY291bnRfaW5pdCgmcnEtPmdzdGF0
ZV9zZXEpOw0KPiA+ID4gKwl1NjRfc3RhdHNfaW5pdCgmcnEtPmFib3J0ZWRfZ3N0YXRlX3N5bmMp
Ow0KPiA+ID4gIH0NCj4gPiA+ICBFWFBPUlRfU1lNQk9MKGJsa19ycV9pbml0KTsNCj4gPiANCj4g
PiBTb3JyeSBidXQgdGhlIGFib3ZlIGNoYW5nZSBsb29rcyB1Z2x5IHRvIG1lLiBNeSB1bmRlcnN0
YW5kaW5nIGlzIHRoYXQgDQo+ID4gYmxrX3JxX2luaXQoKSBpcyBvbmx5IHVzZWQgaW5zaWRlIHRo
ZSBibG9jayBsYXllciB0byBpbml0aWFsaXplIGxlZ2FjeSBibG9jaw0KPiA+IGxheWVyIHJlcXVl
c3RzIHdoaWxlIGdzdGF0ZV9zZXEgYW5kIGFib3J0ZWRfZ3N0YXRlX3N5bmMgYXJlIG9ubHkgcmVs
ZXZhbnQNCj4gPiBmb3IgYmxrLW1xIHJlcXVlc3RzLiBXb3VsZG4ndCBpdCBiZSBiZXR0ZXIgdG8g
YXZvaWQgdGhhdCBibGtfcnFfaW5pdCgpIGlzDQo+ID4gY2FsbGVkIGZvciBibGstbXEgcmVxdWVz
dHMgc3VjaCB0aGF0IHRoZSBhYm92ZSBjaGFuZ2UgY2FuIGJlIGxlZnQgb3V0PyBUaGUNCj4gPiBv
bmx5IGNhbGxlcnMgb3V0c2lkZSB0aGUgYmxvY2sgbGF5ZXIgY29yZSBvZiBibGtfcnFfaW5pdCgp
IEkga25vdyBvZiBhcmUNCj4gPiBpZGVfcHJlcF9zZW5zZSgpIGFuZCBzY3NpX2lvY3RsX3Jlc2V0
KCkuIEkgY2FuIGhlbHAgd2l0aCBjb252ZXJ0aW5nIHRoZSBTQ1NJDQo+ID4gY29kZSBpZiB5b3Ug
d2FudC4NCj4gDQo+IFRoaXMgaXMgYWxzbyB1c2VkIGJ5IGZsdXNoIHBhdGguICBXZSBwcm9iYWJs
eSBzaG91bGQgY2xlYW4gdGhhdCB1cCwNCj4gYnV0IGxldCdzIHdvcnJ5IGFib3V0IHRoYXQgbGF0
ZXIgY3V6IGZsdXNoIGhhbmRsaW5nIGhhcyBlbm91Z2ggb2YgaXRzDQo+IG93biBjb21wbGljYXRp
b25zLg0KDQpXZSBtYXkgaGF2ZSBhIGRpZmZlcmVudCBvcGluaW9uIGFib3V0IHRoaXMgYnV0IEkg
dGhpbmsgaXQgaXMgbW9yZSB0aGFuIGENCmRldGFpbC4gVGhpcyBwYXRjaCBuZWVkcyBnc3RhdGVf
c2VxIGFuZCBhYm9ydGVkX2dzdGF0ZV9zeW5jIHRvIGJlIHByZXNlcnZlZA0KYWNyb3NzIHJlcXVl
c3Qgc3RhdGUgdHJhbnNpdGlvbnMgZnJvbSBNUV9SUV9JTl9GTElHSFQgdG8gTVJfUlFfSURMRS4N
CmJsa19tcV9pbml0X3JlcXVlc3QoKSBpcyBjYWxsZWQgYXQgcmVxdWVzdCBhbGxvY2F0aW9uIHRp
bWUgc28gaXQncyB0aGUgcmlnaHQNCmNvbnRleHQgdG8gaW5pdGlhbGl6ZSBnc3RhdGVfc2VxIGFu
ZCBhYm9ydGVkX2dzdGF0ZV9zeW5jLiBibGtfcnFfaW5pdCgpDQpob3dldmVyIGlzIGNhbGxlZCBi
ZWZvcmUgYSBldmVyeSB1c2Ugb2YgYSByZXF1ZXN0LiBTb3JyeSBidXQgSSdtIG5vdA0KZW50aHVz
aWFzdCBhYm91dCB0aGUgY29kZSBpbiBibGtfcnFfaW5pdCgpIHRoYXQgcmVpbml0aWFsaXplcyBz
dGF0ZQ0KaW5mb3JtYXRpb24gdGhhdCBzaG91bGQgc3Vydml2ZSByZXF1ZXN0IHJldXNlLg0KDQpC
YXJ0Lg==
WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "tj@kernel.org" <tj@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"peterz@infradead.org" <peterz@infradead.org>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"kernel-team@fb.com" <kernel-team@fb.com>,
"oleg@redhat.com" <oleg@redhat.com>, "hch@lst.de" <hch@lst.de>,
"axboe@kernel.dk" <axboe@kernel.dk>,
"jianchao.w.wang@oracle.com" <jianchao.w.wang@oracle.com>,
"osandov@fb.com" <osandov@fb.com>
Subject: Re: [PATCH 2/6] blk-mq: replace timeout synchronization with a RCU and generation based scheme
Date: Thu, 14 Dec 2017 21:13:32 +0000 [thread overview]
Message-ID: <1513286010.2475.58.camel@wdc.com> (raw)
In-Reply-To: <20171214191935.GB3919388@devbig577.frc2.facebook.com>
On Thu, 2017-12-14 at 11:19 -0800, tj@kernel.org wrote:
> On Thu, Dec 14, 2017 at 06:51:11PM +0000, Bart Van Assche wrote:
> > On Tue, 2017-12-12 at 11:01 -0800, Tejun Heo wrote:
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -126,6 +126,8 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
> > > rq->start_time = jiffies;
> > > set_start_time_ns(rq);
> > > rq->part = NULL;
> > > + seqcount_init(&rq->gstate_seq);
> > > + u64_stats_init(&rq->aborted_gstate_sync);
> > > }
> > > EXPORT_SYMBOL(blk_rq_init);
> >
> > Sorry but the above change looks ugly to me. My understanding is that
> > blk_rq_init() is only used inside the block layer to initialize legacy block
> > layer requests while gstate_seq and aborted_gstate_sync are only relevant
> > for blk-mq requests. Wouldn't it be better to avoid that blk_rq_init() is
> > called for blk-mq requests such that the above change can be left out? The
> > only callers outside the block layer core of blk_rq_init() I know of are
> > ide_prep_sense() and scsi_ioctl_reset(). I can help with converting the SCSI
> > code if you want.
>
> This is also used by flush path. We probably should clean that up,
> but let's worry about that later cuz flush handling has enough of its
> own complications.
We may have a different opinion about this but I think it is more than a
detail. This patch needs gstate_seq and aborted_gstate_sync to be preserved
across request state transitions from MQ_RQ_IN_FLIGHT to MR_RQ_IDLE.
blk_mq_init_request() is called at request allocation time so it's the right
context to initialize gstate_seq and aborted_gstate_sync. blk_rq_init()
however is called before a every use of a request. Sorry but I'm not
enthusiast about the code in blk_rq_init() that reinitializes state
information that should survive request reuse.
Bart.
next prev parent reply other threads:[~2017-12-14 21:13 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-12 19:01 [PATCHSET v2] blk-mq: reimplement timeout handling Tejun Heo
2017-12-12 19:01 ` [PATCH 1/6] blk-mq: protect completion path with RCU Tejun Heo
2017-12-13 3:30 ` jianchao.wang
2017-12-13 16:13 ` Tejun Heo
2017-12-14 2:09 ` jianchao.wang
2017-12-14 17:01 ` Bart Van Assche
2017-12-14 17:01 ` Bart Van Assche
2017-12-14 18:14 ` tj
2017-12-12 19:01 ` [PATCH 2/6] blk-mq: replace timeout synchronization with a RCU and generation based scheme Tejun Heo
2017-12-12 21:37 ` Bart Van Assche
2017-12-12 21:37 ` Bart Van Assche
2017-12-12 21:44 ` tj
2017-12-13 5:07 ` jianchao.wang
2017-12-13 16:13 ` Tejun Heo
2017-12-14 18:51 ` Bart Van Assche
2017-12-14 18:51 ` Bart Van Assche
2017-12-14 19:19 ` tj
2017-12-14 21:13 ` Bart Van Assche [this message]
2017-12-14 21:13 ` Bart Van Assche
2017-12-15 13:30 ` tj
2017-12-14 20:20 ` Peter Zijlstra
2017-12-14 21:42 ` Bart Van Assche
2017-12-14 21:42 ` Bart Van Assche
2017-12-14 21:54 ` Peter Zijlstra
2017-12-15 2:12 ` jianchao.wang
2017-12-15 7:31 ` Peter Zijlstra
2017-12-15 15:14 ` jianchao.wang
2017-12-15 2:39 ` Mike Galbraith
2017-12-15 2:39 ` Mike Galbraith
2017-12-15 13:50 ` tj
2017-12-12 19:01 ` [PATCH 3/6] blk-mq: use blk_mq_rq_state() instead of testing REQ_ATOM_COMPLETE Tejun Heo
2017-12-12 19:01 ` [PATCH 4/6] blk-mq: make blk_abort_request() trigger timeout path Tejun Heo
2017-12-14 18:56 ` Bart Van Assche
2017-12-14 18:56 ` Bart Van Assche
2017-12-14 19:26 ` tj
2017-12-12 19:01 ` [PATCH 5/6] blk-mq: remove REQ_ATOM_COMPLETE usages from blk-mq Tejun Heo
2017-12-12 19:01 ` [PATCH 6/6] blk-mq: remove REQ_ATOM_STARTED Tejun Heo
2017-12-12 22:20 ` Bart Van Assche
2017-12-12 22:20 ` Bart Van Assche
2017-12-12 22:22 ` tj
2017-12-12 20:23 ` [PATCHSET v2] blk-mq: reimplement timeout handling Jens Axboe
2017-12-12 21:40 ` Tejun Heo
2017-12-20 23:41 ` Bart Van Assche
2017-12-20 23:41 ` Bart Van Assche
2017-12-21 0:08 ` tj
2017-12-21 1:00 ` Bart Van Assche
2017-12-21 1:00 ` Bart Van Assche
-- strict thread matches above, loose matches on Subject: below --
2017-12-09 19:25 [PATCHSET] " Tejun Heo
2017-12-09 19:25 ` [PATCH 2/6] blk-mq: replace timeout synchronization with a RCU and generation based scheme Tejun Heo
2017-12-12 10:09 ` Peter Zijlstra
2017-12-12 18:02 ` Tejun Heo
2017-12-12 10:10 ` Peter Zijlstra
2017-12-12 18:03 ` Tejun Heo
2017-12-12 11:56 ` Peter Zijlstra
2017-12-12 18:04 ` Tejun Heo
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=1513286010.2475.58.camel@wdc.com \
--to=bart.vanassche@wdc.com \
--cc=axboe@kernel.dk \
--cc=hch@lst.de \
--cc=jianchao.w.wang@oracle.com \
--cc=kernel-team@fb.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=osandov@fb.com \
--cc=peterz@infradead.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.