All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "sagi@grimberg.me" <sagi@grimberg.me>,
	"axboe@kernel.dk" <axboe@kernel.dk>
Cc: "hch@lst.de" <hch@lst.de>, "tj@kernel.org" <tj@kernel.org>,
	"israelr@mellanox.com" <israelr@mellanox.com>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"maxg@mellanox.com" <maxg@mellanox.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"ming.lei@redhat.com" <ming.lei@redhat.com>
Subject: Re: [PATCH v5] blk-mq: Avoid that a completion can be ignored for BLK_EH_RESET_TIMER
Date: Wed, 11 Apr 2018 13:34:51 +0000	[thread overview]
Message-ID: <6684d4b3049397cbbabbee3008f96d80b48c0a17.camel@wdc.com> (raw)
In-Reply-To: <255b84ff-3fe5-1269-70a8-3ab5cc89c1ef@grimberg.me>

T24gV2VkLCAyMDE4LTA0LTExIGF0IDE2OjE5ICswMzAwLCBTYWdpIEdyaW1iZXJnIHdyb3RlOg0K
PiA+ICAgc3RhdGljIHZvaWQgX19ibGtfbXFfcmVxdWV1ZV9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0
ICpycSkNCj4gPiAgIHsNCj4gPiAgIAlzdHJ1Y3QgcmVxdWVzdF9xdWV1ZSAqcSA9IHJxLT5xOw0K
PiA+ICsJZW51bSBtcV9ycV9zdGF0ZSBvbGRfc3RhdGUgPSBibGtfbXFfcnFfc3RhdGUocnEpOw0K
PiA+ICAgDQo+ID4gICAJYmxrX21xX3B1dF9kcml2ZXJfdGFnKHJxKTsNCj4gPiAgIA0KPiA+ICAg
CXRyYWNlX2Jsb2NrX3JxX3JlcXVldWUocSwgcnEpOw0KPiA+ICAgCXdidF9yZXF1ZXVlKHEtPnJx
X3diLCAmcnEtPmlzc3VlX3N0YXQpOw0KPiA+ICAgDQo+ID4gLQlpZiAoYmxrX21xX3JxX3N0YXRl
KHJxKSAhPSBNUV9SUV9JRExFKSB7DQo+ID4gLQkJYmxrX21xX3JxX3VwZGF0ZV9zdGF0ZShycSwg
TVFfUlFfSURMRSk7DQo+ID4gKwlpZiAob2xkX3N0YXRlICE9IE1RX1JRX0lETEUpIHsNCj4gPiAr
CQlpZiAoIWJsa19tcV9jaGFuZ2VfcnFfc3RhdGUocnEsIG9sZF9zdGF0ZSwgTVFfUlFfSURMRSkp
DQo+ID4gKwkJCVdBUk5fT05fT05DRSh0cnVlKTsNCj4gPiAgIAkJaWYgKHEtPmRtYV9kcmFpbl9z
aXplICYmIGJsa19ycV9ieXRlcyhycSkpDQo+ID4gICAJCQlycS0+bnJfcGh5c19zZWdtZW50cy0t
Ow0KPiA+ICAgCX0NCj4gDQo+IENhbiB5b3UgZXhwbGFpbiB3aHkgd2FzIG9sZF9zdGF0ZSBrZXB0
IGFzIGEgbG9jYWwgdmFyaWFibGU/DQoNCkhlbGxvIFNhZ2ksDQoNClNpbmNlIGJsa19tcV9yZXF1
ZXVlX3JlcXVlc3QoKSBtdXN0IGJlIGNhbGxlZCBhZnRlciBhIHJlcXVlc3QgaGFzIGNvbXBsZXRl
ZA0KdGhlIHRpbWVvdXQgaGFuZGxlciB3aWxsIGlnbm9yZSByZXF1ZXN0cyB0aGF0IGFyZSBiZWlu
ZyByZXF1ZXVlZC4gSGVuY2UgaXQNCmlzIHNhZmUgaW4gdGhpcyBmdW5jdGlvbiB0byBjYWNoZSB0
aGUgcmVxdWVzdCBzdGF0ZSBpbiBhIGxvY2FsIHZhcmlhYmxlLg0KDQo+ID4gK3N0YXRpYyBpbmxp
bmUgYm9vbCBibGtfbXFfY2hhbmdlX3JxX3N0YXRlKHN0cnVjdCByZXF1ZXN0ICpycSwNCj4gPiAr
CQkJCQkgIGVudW0gbXFfcnFfc3RhdGUgb2xkX3N0YXRlLA0KPiA+ICsJCQkJCSAgZW51bSBtcV9y
cV9zdGF0ZSBuZXdfc3RhdGUpDQo+ID4gICB7DQo+ID4gLQl1NjQgb2xkX3ZhbCA9IFJFQURfT05D
RShycS0+Z3N0YXRlKTsNCj4gPiAtCXU2NCBuZXdfdmFsID0gKG9sZF92YWwgJiB+TVFfUlFfU1RB
VEVfTUFTSykgfCBzdGF0ZTsNCj4gPiAtDQo+ID4gLQlpZiAoc3RhdGUgPT0gTVFfUlFfSU5fRkxJ
R0hUKSB7DQo+ID4gLQkJV0FSTl9PTl9PTkNFKChvbGRfdmFsICYgTVFfUlFfU1RBVEVfTUFTSykg
IT0gTVFfUlFfSURMRSk7DQo+ID4gLQkJbmV3X3ZhbCArPSBNUV9SUV9HRU5fSU5DOw0KPiA+IC0J
fQ0KPiA+ICsJdW5zaWduZWQgbG9uZyBvbGRfdmFsID0gKFJFQURfT05DRShycS0+X19kZWFkbGlu
ZSkgJiB+UlFfU1RBVEVfTUFTSykgfA0KPiA+ICsJCQkJb2xkX3N0YXRlOw0KPiA+ICsJdW5zaWdu
ZWQgbG9uZyBuZXdfdmFsID0gKG9sZF92YWwgJiB+UlFfU1RBVEVfTUFTSykgfCBuZXdfc3RhdGU7
DQo+ID4gICANCj4gPiAtCS8qIGF2b2lkIGV4cG9zaW5nIGludGVyaW0gdmFsdWVzICovDQo+ID4g
LQlXUklURV9PTkNFKHJxLT5nc3RhdGUsIG5ld192YWwpOw0KPiA+ICsJcmV0dXJuIGNtcHhjaGco
JnJxLT5fX2RlYWRsaW5lLCBvbGRfdmFsLCBuZXdfdmFsKSA9PSBvbGRfdmFsOw0KPiA+ICAgfQ0K
PiANCj4gQ2FuIHlvdSBleHBsYWluIHdoeSB0aGlzIHRha2VzIHRoZSBvbGRfc3RhdGUgb2YgdGhl
IHJlcXVlc3Q/DQoNCkNhbiB5b3UgY2xhcmlmeSB5b3VyIHF1ZXN0aW9uPyBUaGUgcHVycG9zZSBv
ZiB0aGlzIGZ1bmN0aW9uIGlzIHRvIGNoYW5nZQ0KdGhlIHJlcXVlc3Qgc3RhdGUgb25seSBpbnRv
IEBuZXdfc3RhdGUgaWYgaXQgbWF0Y2hlcyBAb2xkX3N0YXRlLiBJIHRoaW5rDQp0aGF0IGlzIGFs
c28gd2hhdCB0aGUgaW1wbGVtZW50YXRpb24gb2YgdGhpcyBmdW5jdGlvbiBkb2VzLg0KDQpUaGFu
a3MsDQoNCkJhcnQuDQoNCg0KDQoNCg==

WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "sagi@grimberg.me" <sagi@grimberg.me>,
	"axboe@kernel.dk" <axboe@kernel.dk>
Cc: "hch@lst.de" <hch@lst.de>, "tj@kernel.org" <tj@kernel.org>,
	"israelr@mellanox.com" <israelr@mellanox.com>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"maxg@mellanox.com" <maxg@mellanox.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"ming.lei@redhat.com" <ming.lei@redhat.com>
Subject: Re: [PATCH v5] blk-mq: Avoid that a completion can be ignored for BLK_EH_RESET_TIMER
Date: Wed, 11 Apr 2018 13:34:51 +0000	[thread overview]
Message-ID: <6684d4b3049397cbbabbee3008f96d80b48c0a17.camel@wdc.com> (raw)
In-Reply-To: <255b84ff-3fe5-1269-70a8-3ab5cc89c1ef@grimberg.me>

On Wed, 2018-04-11 at 16:19 +0300, Sagi Grimberg wrote:
> >   static void __blk_mq_requeue_request(struct request *rq)
> >   {
> >   	struct request_queue *q = rq->q;
> > +	enum mq_rq_state old_state = blk_mq_rq_state(rq);
> >   
> >   	blk_mq_put_driver_tag(rq);
> >   
> >   	trace_block_rq_requeue(q, rq);
> >   	wbt_requeue(q->rq_wb, &rq->issue_stat);
> >   
> > -	if (blk_mq_rq_state(rq) != MQ_RQ_IDLE) {
> > -		blk_mq_rq_update_state(rq, MQ_RQ_IDLE);
> > +	if (old_state != MQ_RQ_IDLE) {
> > +		if (!blk_mq_change_rq_state(rq, old_state, MQ_RQ_IDLE))
> > +			WARN_ON_ONCE(true);
> >   		if (q->dma_drain_size && blk_rq_bytes(rq))
> >   			rq->nr_phys_segments--;
> >   	}
> 
> Can you explain why was old_state kept as a local variable?

Hello Sagi,

Since blk_mq_requeue_request() must be called after a request has completed
the timeout handler will ignore requests that are being requeued. Hence it
is safe in this function to cache the request state in a local variable.

> > +static inline bool blk_mq_change_rq_state(struct request *rq,
> > +					  enum mq_rq_state old_state,
> > +					  enum mq_rq_state new_state)
> >   {
> > -	u64 old_val = READ_ONCE(rq->gstate);
> > -	u64 new_val = (old_val & ~MQ_RQ_STATE_MASK) | state;
> > -
> > -	if (state == MQ_RQ_IN_FLIGHT) {
> > -		WARN_ON_ONCE((old_val & MQ_RQ_STATE_MASK) != MQ_RQ_IDLE);
> > -		new_val += MQ_RQ_GEN_INC;
> > -	}
> > +	unsigned long old_val = (READ_ONCE(rq->__deadline) & ~RQ_STATE_MASK) |
> > +				old_state;
> > +	unsigned long new_val = (old_val & ~RQ_STATE_MASK) | new_state;
> >   
> > -	/* avoid exposing interim values */
> > -	WRITE_ONCE(rq->gstate, new_val);
> > +	return cmpxchg(&rq->__deadline, old_val, new_val) == old_val;
> >   }
> 
> Can you explain why this takes the old_state of the request?

Can you clarify your question? The purpose of this function is to change
the request state only into @new_state if it matches @old_state. I think
that is also what the implementation of this function does.

Thanks,

Bart.





  reply	other threads:[~2018-04-11 13:34 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-10 21:01 [PATCH v5] blk-mq: Avoid that a completion can be ignored for BLK_EH_RESET_TIMER Bart Van Assche
2018-04-11  2:11 ` Ming Lei
2018-04-11 13:10   ` Sagi Grimberg
2018-04-11 13:36   ` Bart Van Assche
2018-04-11 13:36     ` Bart Van Assche
2018-04-12 13:21   ` Christoph Hellwig
2018-04-11 13:19 ` Sagi Grimberg
2018-04-11 13:34   ` Bart Van Assche [this message]
2018-04-11 13:34     ` Bart Van Assche
2018-04-11 14:32     ` Sagi Grimberg
2018-04-11 14:34       ` Bart Van Assche
2018-04-11 14:34         ` Bart Van Assche
2018-04-12  5:32   ` Christoph Hellwig
2018-04-12 13:23 ` Christoph Hellwig
2018-04-14  0:06 ` Ming Lei

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=6684d4b3049397cbbabbee3008f96d80b48c0a17.camel@wdc.com \
    --to=bart.vanassche@wdc.com \
    --cc=axboe@kernel.dk \
    --cc=hch@lst.de \
    --cc=israelr@mellanox.com \
    --cc=linux-block@vger.kernel.org \
    --cc=maxg@mellanox.com \
    --cc=ming.lei@redhat.com \
    --cc=sagi@grimberg.me \
    --cc=stable@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.