block: rework flush sequencing for blk-mq

Witch to using a preallocated flush_rq for blk-mq similar to what's done with the old request path. This allows us to set up the request properly with a tag from the actually allowed range and ->rq_disk as needed by some drivers. To make life easier we also switch to dynamic allocation of ->flush_rq for the old path.

This effectively reverts most of

"blk-mq: fix for flush deadlock"

and

"blk-mq: Don't reserve a tag for flush request"

Reusing the tag for flush request is considered before. The problem is driver need get a request from a tag, reusing tag breaks this. The possible solution is we provide a blk_mq_tag_to_request, and force driver uses it. And in this function, if tag equals to flush_rq tag, we return flush_request.

Quoted 4 lines · Shaohua Li

Reusing the tag for flush request is considered before. The problem is driver need get a request from a tag, reusing tag breaks this. The possible solution is we provide a blk_mq_tag_to_request, and force driver uses it. And in this function, if tag equals to flush_rq tag, we return flush_request.

If we want to support tag to request reverse mapping we defintively need to do this in the core, at which point special casing flush_rq there is easy.

Which driver needs the reverse mapping? Neither virtio_blk nor null_blk seem to and I've not seen any other conversions submitted except for the scsi work.

Yep, none driver needs it. But reverse mapping is the one of the points we use tag, so better we prepare it.

Witch to using a preallocated flush_rq for blk-mq similar to what's done with the old request path. This allows us to set up the request properly with a tag from the actually allowed range and ->rq_disk as needed by some drivers. To make life easier we also switch to dynamic allocation of ->flush_rq for the old path.

This effectively reverts most of

"blk-mq: fix for flush deadlock"

and

"blk-mq: Don't reserve a tag for flush request"

Signed-off-by Christoph Hellwig <[email protected]>

block/blk-flush.c +9 -3

-static void blk_mq_flush_data_insert(struct request *rq)
+static bool blk_flush_queue_rq(struct request *rq)
 {
-	INIT_WORK(&rq->mq_flush_data, mq_flush_data_run);
-	kblockd_schedule_work(rq->q, &rq->mq_flush_data);
+	if (rq->q->mq_ops) {
+		INIT_WORK(&rq->mq_flush_work, mq_flush_run);
+		kblockd_schedule_work(rq->q, &rq->mq_flush_work);
+		return false;
+	} else {
+		list_add_tail(&rq->queuelist, &rq->q->queue_head);
+		return true;
+	}
 }
 
 /**

@@ -187,12 +193,7 @@ static bool blk_flush_complete_seq(struct request *rq, unsigned int seq,
 

Hi, Christoph,

Did you mean to switch from list_add to list_add_tail? That seems like a change that warrants mention.

Quoted 4 lines · Jeff Moyer

Hi, Christoph,

Did you mean to switch from list_add to list_add_tail? That seems like a change that warrants mention.

No, that wasn't intentional and should be fixed. Btw, there was another issue with that commit, in that dm-multipath also needs to allocate ->flush_rq. I saw a patch from Hannes fixing it in the SuSE tree, and would really love to see him submit that for mainline as well.

Unfortunately SuSE seems to have lots of block and dm fixes and even features that they don't submit upstream.

Quoted 10 lines · Christoph Hellwig

Quoted 4 lines · Jeff Moyer

Hi, Christoph,

Did you mean to switch from list_add to list_add_tail? That seems like a change that warrants mention.

No, that wasn't intentional and should be fixed. Btw, there was another issue with that commit, in that dm-multipath also needs to allocate ->flush_rq. I saw a patch from Hannes fixing it in the SuSE tree, and would really love to see him submit that for mainline as well.

Ugh, rq-based DM calls blk_init_allocated_queue.. (ah, looks like best to move q->flush_rq allocation from blk_init_queue_node to blk_alloc_queue_node?). Anyway, this all makes sense given the crashes we've been dealing with.. we couldn't immediately understand how the q->flush_rq would be NULL... grr. I guess I should've checked with you sooner. I reverted commit 1874198 "blk-mq: rework flush sequencing logic" from RHEL7 just yesterday because we were seeing crashes on flush with dm-mpath. But can easily re-apply for RHEL7.1 (since the request_queue's embedded flush_rq takes up so much space we get ample kABI padding).

Not overly proud of the revert, but I deemed easier to revert than hunt down the fix given RHEL7 won't actually be providing any blk-mq enabled drivers. That'll change for RHEL7.1.

Quoted 2 lines

Unfortunately SuSE seems to have lots of block and dm fixes and even features that they don't submit upstream.

Yeah, it is certainly disturbing. No excuse for sitting on fixes like this.

Hannes, _please_ get this dm-mpath flush_rq fix for 3.14 posted ASAP. Jens or I will need to get it to Linus next week.

Quoted 18 lines · Mike Snitzer

Quoted 13 lines · Christoph Hellwig

Quoted 5 lines · Jeff Moyer

Hi, Christoph,

Did you mean to switch from list_add to list_add_tail? That seems like a change that warrants mention.

No, that wasn't intentional and should be fixed. Btw, there was another issue with that commit, in that dm-multipath also needs to allocate ->flush_rq. I saw a patch from Hannes fixing it in the SuSE tree, and would really love to see him submit that for mainline as well.

Quoted 3 lines

Ugh, rq-based DM calls blk_init_allocated_queue.. (ah, looks like best to move q->flush_rq allocation from blk_init_queue_node to blk_alloc_queue_node?).

Quoted 10 lines

Quoted 2 lines

Unfortunately SuSE seems to have lots of block and dm fixes and even features that they don't submit upstream.

Yeah, it is certainly disturbing. No excuse for sitting on fixes like this.

Hannes, _please_ get this dm-mpath flush_rq fix for 3.14 posted ASAP. Jens or I will need to get it to Linus next week.

Hey, calm down.

I've made the fix just two days ago. And was quite surprised that I've been the first hitting that; should've crashed for everybody using dm-multipath.

And given the pushback I've gotten recently from patches I would have thought that it would work for most users; sure the author would've done due diligence on the original patchset ...
Plus I've gotten the reports from S/390, so I put it down to mainframe weirdness.

BTW, it not _my_ decision to sit on tons of SUSE specific patches.

I really try to get things upstream. But I cannot do more than sending patches upstream, answer patiently any questions, and redo the patchset. Which I did. Frequently, But, alas, it's up to the maintainer to apply them. And I can only ask and hope. The usual story...

I'll be sending the patch soon, Monday at latest.

Quoted 19 lines · Hannes Reinecke

Quoted 18 lines · Mike Snitzer

Quoted 13 lines · Christoph Hellwig

Quoted 5 lines · Jeff Moyer

Hi, Christoph,

Did you mean to switch from list_add to list_add_tail? That seems like a change that warrants mention.

No, that wasn't intentional and should be fixed. Btw, there was another issue with that commit, in that dm-multipath also needs to allocate ->flush_rq. I saw a patch from Hannes fixing it in the SuSE tree, and would really love to see him submit that for mainline as well.

Quoted 3 lines

Quoted 3 lines

Ugh, rq-based DM calls blk_init_allocated_queue.. (ah, looks like best to move q->flush_rq allocation from blk_init_queue_node to blk_alloc_queue_node?).

The above suggestion would work. But we'd lose the side-effect benefit to bio-based DM not needing q->flush_rq allocated at all.

But I'm not immediately seeing a clean way to get that benefit (of not allocating for bio-based request_queue) while always allocating it for request-based queues.

Actually, how about moving the flush_rq allocation to blk_init_allocated_queue(), it'd accomplish both.. what do others think of that?

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

Unfortunately SuSE seems to have lots of block and dm fixes and even features that they don't submit upstream.

Yeah, it is certainly disturbing. No excuse for sitting on fixes like this.

Hannes, _please_ get this dm-mpath flush_rq fix for 3.14 posted ASAP. Jens or I will need to get it to Linus next week.

Quoted 1 line

Hey, calm down.

I'm calm.. was just a bit frustrated. But this isn't a big deal. I'll make an effort to reach out to relevant people sooner when similar stuff is reported against recently upstreamed code. Would be cool if you did the same. I can relate to needing to have the distro vendor hat on (first needing to determine/answer "is this issue specific to our hacked distro kernel?", etc).

Quoted 8 lines · Mike Snitzer

I'm calm.. was just a bit frustrated. But this isn't a big deal. I'll make an effort to reach out to relevant people sooner when similar stuff is reported against recently upstreamed code. Would be cool if you did the same. I can relate to needing to have the distro vendor hat on (first needing to determine/answer "is this issue specific to our hacked distro kernel?", etc).

The patch I made wasn't in the context of 'recently upstreamed code', it was due to a backport Jan Kara did for our next distro kernels (3.12-based).

Quoted 9 lines · Hannes Reinecke

Quoted 8 lines · Mike Snitzer

I'm calm.. was just a bit frustrated. But this isn't a big deal. I'll make an effort to reach out to relevant people sooner when similar stuff is reported against recently upstreamed code. Would be cool if you did the same. I can relate to needing to have the distro vendor hat on (first needing to determine/answer "is this issue specific to our hacked distro kernel?", etc).

Quoted 3 lines

The patch I made wasn't in the context of 'recently upstreamed code', it was due to a backport Jan Kara did for our next distro kernels (3.12-based).

"3.12-based" means nothing given all the backporting for SLES, much like "3.10-based" means nothing in the context of RHEL7.

The only way this fix is applicable is in the context of "recently upstreamed code", commit 1874198 ("blk-mq: rework flush sequencing logic") went upstream for v3.14-rc3.

Jens, please feel free to queue this tested fix for 3.14-rc:

Quoted 19 lines · Mike Snitzer

Quoted 9 lines · Hannes Reinecke

Quoted 8 lines · Mike Snitzer

I'm calm.. was just a bit frustrated. But this isn't a big deal. I'll make an effort to reach out to relevant people sooner when similar stuff is reported against recently upstreamed code. Would be cool if you did the same. I can relate to needing to have the distro vendor hat on (first needing to determine/answer "is this issue specific to our hacked distro kernel?", etc).

Quoted 3 lines

The patch I made wasn't in the context of 'recently upstreamed code', it was due to a backport Jan Kara did for our next distro kernels (3.12-based).

"3.12-based" means nothing given all the backporting for SLES, much like "3.10-based" means nothing in the context of RHEL7.

Quoted 5 lines

The only way this fix is applicable is in the context of "recently upstreamed code", commit 1874198 ("blk-mq: rework flush sequencing logic") went upstream for v3.14-rc3.

Jens, please feel free to queue this tested fix for 3.14-rc:

Thanks Mike, queued up.

Also queued up the list addition reversal change.

Quoted 20 lines · Jens Axboe

Quoted 19 lines · Mike Snitzer

Quoted 9 lines · Hannes Reinecke

Quoted 8 lines · Mike Snitzer

I'm calm.. was just a bit frustrated. But this isn't a big deal. I'll make an effort to reach out to relevant people sooner when similar stuff is reported against recently upstreamed code. Would be cool if you did the same. I can relate to needing to have the distro vendor hat on (first needing to determine/answer "is this issue specific to our hacked distro kernel?", etc).

Quoted 3 lines

The patch I made wasn't in the context of 'recently upstreamed code', it was due to a backport Jan Kara did for our next distro kernels (3.12-based).

"3.12-based" means nothing given all the backporting for SLES, much like "3.10-based" means nothing in the context of RHEL7.

Quoted 7 lines

Quoted 5 lines

The only way this fix is applicable is in the context of "recently upstreamed code", commit 1874198 ("blk-mq: rework flush sequencing logic") went upstream for v3.14-rc3.

Jens, please feel free to queue this tested fix for 3.14-rc:

Thanks Mike, queued up.

Thanks.

Quoted 1 line

Also queued up the list addition reversal change.

I had a look at what you queued, thing is commit 1874198 replaced code in blk_kick_flush() that did use list_add_tail(). So getting back to the way the original code was (before 1874198) would need something like the following patch.

But it isn't clear to me why we'd have the duality of front vs tail additions for flushes. Maybe Christoph knows?

Quoted 6 lines · Mike Snitzer

I had a look at what you queued, thing is commit 1874198 replaced code in blk_kick_flush() that did use list_add_tail(). So getting back to the way the original code was (before 1874198) would need something like the following patch.

Quoted 2 lines

But it isn't clear to me why we'd have the duality of front vs tail additions for flushes. Maybe Christoph knows?

Not sure it'd even make a difference with the use case, but always tail would be broken. But the flushing in general is a bit of a nightmare, so I'd be inclined to add your full fix too, at least this late in -rc.

Hey, calm down.
I've made the fix just two days ago. And was quite surprised that I've been the first hitting that; should've crashed for everybody using dm-multipath.
And given the pushback I've gotten recently from patches I would have thought that it would work for most users; sure the author would've done due diligence on the original patchset ...

BTW, it not _my_ decision to sit on tons of SUSE specific patches. I really try to get things upstream. But I cannot do more than sending patches upstream, answer patiently any questions, and redo the patchset.
Which I did. Frequently, But, alas, it's up to the maintainer to apply them. And I can only ask and hope. The usual story...

Quoted 7 lines · Hannes Reinecke

Hey, calm down.
I've made the fix just two days ago. And was quite surprised that I've been the first hitting that; should've crashed for everybody using dm-multipath.
And given the pushback I've gotten recently from patches I would have thought that it would work for most users; sure the author would've done due diligence on the original patchset ...

There's very little testing of dm-multipath for upstream work, as I've seen tons of avoidable breakage. Doesn't help that it uses a special code path that one else uses.

Quoted 6 lines

BTW, it not _my_ decision to sit on tons of SUSE specific patches. I really try to get things upstream. But I cannot do more than sending patches upstream, answer patiently any questions, and redo the patchset.
Which I did. Frequently, But, alas, it's up to the maintainer to apply them. And I can only ask and hope. The usual story...

Let's make this a little less personal. Fact is that the SuSE trees have tons of patches in there that never have even been sent upstream. There's also tons that have been posted once or twice. While I feel your frustration with the SCSI process fully and we'll need to work on that somehow, how about you do another round of dumping the DM patches on the dm-devel list and Mike?

I'll ping some of the other worst offenders as time permits.

From the device-mapper side we have these patches which are not

Quoted 1 line

From the device-mapper side we have these patches which are not

included upstream:

Quoted 2 lines · Hannes Reinecke

Quoted 1 line

From the device-mapper side we have these patches which are not

included upstream:

Another one on the dm side that seems useful is:

dm-emulate-blkrrpart-ioctl:
Partitions on device-mapper devices are managed by kpartx (if at all). So if we were just to send out a 'change' event if someone called BLKRRPART on these devices, kpartx will be triggered via udev and can manage the partitions accordingly.

The SLES12 tree has various patches to implement special O_DIRECT|O_NONBLOCK semantics for block devices:

	https://gitorious.org/opensuse/kernel-source/source/806eab3e4b02e798c1ae942440051f81c822ca35:patches.suse/block-nonblock-causes-failfast

this seems genuinely useful and I'd be really happy if people would do this work upstream for two reasons:

a) implementing different semantics only in a vendor kernel is a nightmare. No proper way to document it in the man pages for example, and silent breakage of applications that expect it to be present, or even more nasty not present.
b) Which brings us to: we had various issues with adding O_NONBLOCK to files that didn't support it before. How well was this whole feature tested?

I also think there should probably be just one FAILFAST flag. Where it was the DEV or the TRANSPORT or the DRIVER that failed could be returned in the error code for any caller that cared. But as I don't know why the one became three I could well be missing something important.

Quoted 7 lines · Hannes Reinecke

Hey, calm down.
I've made the fix just two days ago. And was quite surprised that I've been the first hitting that; should've crashed for everybody using dm-multipath.
And given the pushback I've gotten recently from patches I would have thought that it would work for most users; sure the author would've done due diligence on the original patchset ...

There's very little testing of dm-multipath for upstream work, as I've seen tons of avoidable breakage. Doesn't help that it uses a special code path that one else uses.

Quoted 6 lines

BTW, it not _my_ decision to sit on tons of SUSE specific patches. I really try to get things upstream. But I cannot do more than sending patches upstream, answer patiently any questions, and redo the patchset.
Which I did. Frequently, But, alas, it's up to the maintainer to apply them. And I can only ask and hope. The usual story...

Let's make this a little less personal. Fact is that the SuSE trees have tons of patches in there that never have even been sent upstream. There's also tons that have been posted once or twice. While I feel your frustration with the SCSI process fully and we'll need to work on that somehow, how about you do another round of dumping the DM patches on the dm-devel list and Mike?

I'll ping some of the other worst offenders as time permits.

Pretty ironic that in the same email that you ask someone to "Let's make this a little less personal." you start by asserting upstream dm-multipath sees very little testing -- and use your commit that recently broke dm-multipath as the basis. Anyway, please exapnd on what you feel is broken with upstream dm-multipath.

Quoted 5 lines · Mike Snitzer

Pretty ironic that in the same email that you ask someone to "Let's make this a little less personal." you start by asserting upstream dm-multipath sees very little testing -- and use your commit that recently broke dm-multipath as the basis. Anyway, please exapnd on what you feel is broken with upstream dm-multipath.

Getting a little upset, eh? I didn't say it's broken, I said it gets very little testing. The regression from me was found like so many before only after it was backported o some enterprise kernel.

I think the problem here is two-fold:
a) the hardware you use with dm-multipath isn't widely available.
b) it uses a very special code path in the block layer no one else uses

a) might be fixable by having some RDAC or similar emulation in qemu if someone wants to spend the effort.
b) is a bit harder, but we should think hard about it when rewriting the multipath code to support blk-mq. Talking about which I think trying to use dm-multipath on any blk-mq device will go horribly crash and boom at the moment.

That was actually one of my plans, move dm-multipath over to use blk-mq. But then I'd need to discuss with Jens et al how to best achieve this; the current static hctx allocation doesn't play well with multipaths dynamic path management.

b) is a bit harder, but we should think hard about it when rewriting the multipath code to support blk-mq. Talking about which I think trying to use dm-multipath on any blk-mq device will go horribly crash and boom at the moment.

Quoted 4 lines · Christoph Hellwig

b) is a bit harder, but we should think hard about it when rewriting the multipath code to support blk-mq. Talking about which I think trying to use dm-multipath on any blk-mq device will go horribly crash and boom at the moment.

Talking abnout crashing and burning.. Hannes, did you run this patch past dm-devel and linux-scsi yet? Don't quite like it but the problem seems real..

No, I haven't. This issue is only exhibited if you try to run multipath on a non-SCSI device (in this case it was cciss). But then that project got abandoned, and there never was a machine with a multipathed cciss controller.

Quoted 4 lines · Hannes Reinecke

No, I haven't. This issue is only exhibited if you try to run multipath on a non-SCSI device (in this case it was cciss). But then that project got abandoned, and there never was a machine with a multipathed cciss controller.

We still shouldn't be able to easily crash the kernel because someone configured the wrong device. And with scsi-mq that wrong devices case might become a lot more common..

Quoted 5 lines · Mike Snitzer

Pretty ironic that in the same email that you ask someone to "Let's make this a little less personal." you start by asserting upstream dm-multipath sees very little testing -- and use your commit that recently broke dm-multipath as the basis. Anyway, please exapnd on what you feel is broken with upstream dm-multipath.

Getting a little upset, eh? I didn't say it's broken, I said it gets very little testing. The regression from me was found like so many before only after it was backported o some enterprise kernel.

I think the problem here is two-fold:
a) the hardware you use with dm-multipath isn't widely available.
b) it uses a very special code path in the block layer no one else uses

a) might be fixable by having some RDAC or similar emulation in qemu if someone wants to spend the effort.

b) is a bit harder, but we should think hard about it when rewriting the multipath code to support blk-mq. Talking about which I think trying to use dm-multipath on any blk-mq device will go horribly crash and boom at the moment.

Quoted 3 lines

Getting a little upset, eh? I didn't say it's broken, I said it gets very little testing. The regression from me was found like so many before only after it was backported o some enterprise kernel.

Even _really_ basic dm-multipath testing would've uncovered this bug.

Quoted 6 lines

I think the problem here is two-fold:
a) the hardware you use with dm-multipath isn't widely available.
b) it uses a very special code path in the block layer no one else uses

a) might be fixable by having some RDAC or similar emulation in qemu if someone wants to spend the effort.

The regression from the commit in question was easily reproduced/tested using scsi_debug. Just start the multipathd service and any scsi_debug device in the system will get multipath'd.

Quoted 4 lines · Mike Snitzer

Quoted 3 lines

Getting a little upset, eh? I didn't say it's broken, I said it gets very little testing. The regression from me was found like so many before only after it was backported o some enterprise kernel.

Quoted 1 line

Even _really_ basic dm-multipath testing would've uncovered this bug.

But major testing using dm, md and various low-level drivers didn't. Do you really expect everyone to remember to specificly test dm-multipath for a core block change? Without a nicely documented way to do it?

That being said I should have remembered that it's special, but even I didn't and I'm sorry about that.

Quoted 7 lines

Quoted 6 lines

I think the problem here is two-fold:
a) the hardware you use with dm-multipath isn't widely available.
b) it uses a very special code path in the block layer no one else uses

a) might be fixable by having some RDAC or similar emulation in qemu if someone wants to spend the effort.

Quoted 3 lines

The regression from the commit in question was easily reproduced/tested using scsi_debug. Just start the multipathd service and any scsi_debug device in the system will get multipath'd.

Really? That sounds a like a bug in the INQUIRY information returned by scsi_debug. Either way please write up these things in Documentation so you can point people at it easily.

I was more reacting to the assertion you made like multipath regresses all the time. I'm not faulting you at all for not having tested multipath. Hell, I even forget to test multipath more than I should. /me says with shame

Yeah, not sure why single path scsi_debug "just works", maybe it is a "feature" of the older multipathd I have kicking around?, but for basic data path testing scsi_debug is a quick means to an end. I can look closer at _why_ it gets multipathd in a bit. But maybe Ben or Hannes will have quicker insight?

For me, if multipathd is running, if I issue the following a multipath device gets layered ontop of the associated scsi_debug created sd
device: modprobe scsi_debug dev_size_mb=1024

I think it useful to have scsi_debug work with multipath. I know people in Red Hat's QE organization have even simulated multiple paths with it.. but I don't recall if they had to hack scsi_debug to do that. I'll try to find out.

Yeah, not sure why single path scsi_debug "just works", maybe it is a "feature" of the older multipathd I have kicking around?, but for basic data path testing scsi_debug is a quick means to an end. I can look closer at _why_ it gets multipathd in a bit. But maybe Ben or Hannes will have quicker insight?