Re: split scsi passthrough fields out of struct request V2

2017-02-02 Thread Bart Van Assche 
On Thu, 2017-02-02 at 16:04 -0500, Mike Snitzer wrote:
> Any progress on getting this to work without requiring infiniband HW?

Hello Mike,

Intructions for running these tests over SoftRoCE have been added to
the README.md file in https://github.com/bvanassche/srp-test. However,
I'm not sure the SoftRoCE driver is already stable enough to run these
tests on top of that driver.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-02-02 Thread Mike Snitzer 
On Thu, Feb 02 2017 at  4:04pm -0500,
Mike Snitzer  wrote:

> On Thu, Feb 02 2017 at  2:46pm -0500,
> Bart Van Assche  wrote:
> 
> > On Thu, 2017-02-02 at 14:13 -0500, Mike Snitzer wrote:
> > > On Thu, Feb 02 2017 at  1:43pm -0500, Bart Van Assche 
> > >  wrote:
> > > > On Thu, 2017-02-02 at 13:33 -0500, Mike Snitzer wrote:
> > > > > I'll go back over hch's changes to see if I can spot anything.  But is
> > > > > this testing using dm_mod.use_bk_mq=Y or are you testing old 
> > > > > .request_fn
> > > > > dm-multipath?
> > > > 
> > > > The srp-test software tests multiple configurations: dm-mq on scsi-mq, 
> > > > dm-sq
> > > > on scsi-mq and dm-sq on scsi-sq. I have not yet checked which of these
> > > > three configurations triggers the kernel crash.
> > > 
> > > OK, such info is important to provide for crashes like this.  Please let
> > > me know once you do.
> > 
> > Hello Mike,
> > 
> > Apparently it's the large I/O test (using dm-mq on scsi-mq) that triggers 
> > the
> > crash:
> 
> I've gone over Christoph's "dm: always defer request allocation to the
> owner of the request_queue" commit yet again.  Most of that commit's
> changes are just mechanical.  I didn't see any problems.
> 
> In general, dm_start_request() calls dm_get(md) to take a reference on
> the mapped_device.  And rq_completed() calls dm_put(md) to drop the
> reference.  The DM device's request_queue (md->queue) should _not_ ever
> be torn down before all references on the md have been dropped. But I'll
> have to look closer on how/if that is enforced anywhere by coordinating
> with block core.
> 
> In any case, the crash you reported was that the mapped_device was being
> dereferenced after it was freed (at line 187's md->queue).  Which seems
> to imply a dm_get/dm_put reference count regression.  But I'm not seeing
> where at this point.

Maybe it isn't a regression but something about Christoph's changes
causes a race to present itself?

Care to try moving the dm_get(md) at the end of dm_start_request() to
the beginning of dm_start_request() and report back on whether it helps
at all?

Thanks,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-02-02 Thread Mike Snitzer 
On Thu, Feb 02 2017 at  2:46pm -0500,
Bart Van Assche  wrote:

> On Thu, 2017-02-02 at 14:13 -0500, Mike Snitzer wrote:
> > On Thu, Feb 02 2017 at  1:43pm -0500, Bart Van Assche 
> >  wrote:
> > > On Thu, 2017-02-02 at 13:33 -0500, Mike Snitzer wrote:
> > > > I'll go back over hch's changes to see if I can spot anything.  But is
> > > > this testing using dm_mod.use_bk_mq=Y or are you testing old .request_fn
> > > > dm-multipath?
> > > 
> > > The srp-test software tests multiple configurations: dm-mq on scsi-mq, 
> > > dm-sq
> > > on scsi-mq and dm-sq on scsi-sq. I have not yet checked which of these
> > > three configurations triggers the kernel crash.
> > 
> > OK, such info is important to provide for crashes like this.  Please let
> > me know once you do.
> 
> Hello Mike,
> 
> Apparently it's the large I/O test (using dm-mq on scsi-mq) that triggers the
> crash:

I've gone over Christoph's "dm: always defer request allocation to the
owner of the request_queue" commit yet again.  Most of that commit's
changes are just mechanical.  I didn't see any problems.

In general, dm_start_request() calls dm_get(md) to take a reference on
the mapped_device.  And rq_completed() calls dm_put(md) to drop the
reference.  The DM device's request_queue (md->queue) should _not_ ever
be torn down before all references on the md have been dropped. But I'll
have to look closer on how/if that is enforced anywhere by coordinating
with block core.

In any case, the crash you reported was that the mapped_device was being
dereferenced after it was freed (at line 187's md->queue).  Which seems
to imply a dm_get/dm_put reference count regression.  But I'm not seeing
where at this point.

> # ~bart/software/infiniband/srp-test/run_tests -r 10
> [ ... ]
> Test /home/bart/software/infiniband/srp-test/tests/02-sq-on-mq succeeded
> Running test /home/bart/software/infiniband/srp-test/tests/03 ...
> Test large transfer sizes with cmd_sg_entries=255
> removing /dev/mapper/mpatht: [ CRASH ]
> 
> The source code of the test I ran is available at
> https://github.com/bvanassche/srp-test.

Any progress on getting this to work without requiring infiniband HW?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-02-02 Thread Bart Van Assche 
On Thu, 2017-02-02 at 14:13 -0500, Mike Snitzer wrote:
> On Thu, Feb 02 2017 at  1:43pm -0500, Bart Van Assche 
>  wrote:
> > On Thu, 2017-02-02 at 13:33 -0500, Mike Snitzer wrote:
> > > I'll go back over hch's changes to see if I can spot anything.  But is
> > > this testing using dm_mod.use_bk_mq=Y or are you testing old .request_fn
> > > dm-multipath?
> > 
> > The srp-test software tests multiple configurations: dm-mq on scsi-mq, dm-sq
> > on scsi-mq and dm-sq on scsi-sq. I have not yet checked which of these
> > three configurations triggers the kernel crash.
> 
> OK, such info is important to provide for crashes like this.  Please let
> me know once you do.

Hello Mike,

Apparently it's the large I/O test (using dm-mq on scsi-mq) that triggers the
crash:

# ~bart/software/infiniband/srp-test/run_tests -r 10
[ ... ]
Test /home/bart/software/infiniband/srp-test/tests/02-sq-on-mq succeeded
Running test /home/bart/software/infiniband/srp-test/tests/03 ...
Test large transfer sizes with cmd_sg_entries=255
removing /dev/mapper/mpatht: [ CRASH ]

The source code of the test I ran is available at
https://github.com/bvanassche/srp-test.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-02-02 Thread Mike Snitzer 
On Thu, Feb 02 2017 at  1:43pm -0500,
Bart Van Assche  wrote:

> On Thu, 2017-02-02 at 13:33 -0500, Mike Snitzer wrote:
> > I'll go back over hch's changes to see if I can spot anything.  But is
> > this testing using dm_mod.use_bk_mq=Y or are you testing old .request_fn
> > dm-multipath?
> 
> Hello Mike,
> 
> The srp-test software tests multiple configurations: dm-mq on scsi-mq, dm-sq
> on scsi-mq and dm-sq on scsi-sq. I have not yet checked which of these
> three configurations triggers the kernel crash.

OK, such info is important to provide for crashes like this.  Please let
me know once you do.

Thanks,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-02-02 Thread Bart Van Assche 
On Thu, 2017-02-02 at 13:33 -0500, Mike Snitzer wrote:
> I'll go back over hch's changes to see if I can spot anything.  But is
> this testing using dm_mod.use_bk_mq=Y or are you testing old .request_fn
> dm-multipath?

Hello Mike,

The srp-test software tests multiple configurations: dm-mq on scsi-mq, dm-sq
on scsi-mq and dm-sq on scsi-sq. I have not yet checked which of these
three configurations triggers the kernel crash.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-02-02 Thread Mike Snitzer 
On Thu, Feb 02 2017 at 12:27pm -0500,
Bart Van Assche  wrote:

> On Wed, 2017-02-01 at 22:01 +, Bart Van Assche wrote:
> > However, a new issue shows up sporadically, an issue that I had not yet seen
> > during any test with a kernel tree from Linus:
> >
> > [  227.613440] general protection fault:  [#1] SMP
> > [  227.613495] Modules linked in: dm_service_time ib_srp scsi_transport_srp 
> > target_core_user uio target_core_pscsi target_core_file ib_srpt 
> > target_core_iblock target_core_mod brd netconsole xt_CHECKSUM 
> > iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat 
> > nf_nat_ipv4 nf_nat libcrc32c nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack 
> > nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp tun bridge stp llc 
> > ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter ip_tables 
> > x_tables af_packet ib_ipoib msr rdma_ucm ib_ucm ib_uverbs ib_umad rdma_cm 
> > configfs ib_cm iw_cm mlx4_ib ib_core sb_edac edac_core x86_pkg_temp_thermal 
> > intel_powerclamp coretemp kvm_intel ipmi_ssif kvm irqbypass 
> > crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel pcbc tg3 
> > aesni_intel iTCO_wdt mlx4_core ptp iTCO_vendor_support dcdbas aes_x86_64 
> > crypto_simd glue_helper pps_core cryptd pcspkr devlink ipmi_si libphy 
> > ipmi_devintf fjes ipmi_msghandler tpm_tis tpm_tis_core lpc_ich mei_me 
> > mfd_core mei shpchp wmi tpm button hid_generic usbhid mgag200 i2c_algo_bit 
> > drm_kms_helper syscopyarea sysfillrect sr_mod sysimgblt fb_sys_fops cdrom 
> > ttm drm ehci_pci ehci_hcd usbcore usb_common sg dm_multipath dm_mod 
> > scsi_dh_rdac scsi_dh_emc scsi_dh_alua autofs4
> > [  227.613774] CPU: 3 PID: 28 Comm: ksoftirqd/3 Not tainted 4.10.0-rc5-dbg+ 
> > #1
> > [  227.613840] Hardware name: Dell Inc. PowerEdge R430/03XKDV, BIOS 1.0.2 
> > 11/17/2014
> > [  227.613893] task: 880172a249c0 task.stack: c90001aa8000
> > [  227.613932] RIP: 0010:rq_completed+0x12/0x90 [dm_mod]
> > [  227.613965] RSP: 0018:c90001aabda8 EFLAGS: 00010246
> > [  227.614006] RAX:  RBX: 6b6b6b6b6b6b6b6b RCX: 
> > 
> > [  227.614043] RDX:  RSI:  RDI: 
> > 6b6b6b6b6b6b6b6b
> > [  227.614074] RBP: c90001aabdc0 R08: 8803825f4c38 R09: 
> > 
> > [  227.614105] R10:  R11:  R12: 
> > 
> > [  227.614137] R13:  R14: 81c05120 R15: 
> > 0004
> > [  227.614170] FS:  () GS:88046f2c() 
> > knlGS:
> > [  227.614209] CS:  0010 DS:  ES:  CR0: 80050033
> > [  227.614239] CR2: 557e28bc20d0 CR3: 00038594e000 CR4: 
> > 001406e0
> > [  227.614268] Call Trace:
> > [  227.614301]  dm_softirq_done+0xe6/0x1e0 [dm_mod]
> > [  227.614337]  blk_done_softirq+0x88/0xa0
> > [  227.614369]  __do_softirq+0xba/0x4c0
> > [  227.614470]  run_ksoftirqd+0x1a/0x50
> > [  227.614499]  smpboot_thread_fn+0x123/0x1e0
> > [  227.614529]  kthread+0x107/0x140
> > [  227.614624]  ret_from_fork+0x2e/0x40
> > [  227.614648] Code: ff ff 31 f6 48 89 c7 e8 cd 0e 2f e1 5d c3 90 66 2e 0f 
> > 1f 84 00 00 00 00 00 55 48 63 f6 48 89 e5 41 55 41 89 d5 41 54 53 48 89 fb 
> > <4c> 8b a7 88 02 00 00 f0 ff 8c b7 50 03 00 00 e8 ba 43 ff ff 85 
> > [  227.614738] RIP: rq_completed+0x12/0x90 [dm_mod] RSP: c90001aabda8
> > 
> > (gdb) list *(rq_completed+0x12)
> > 0xdd12 is in rq_completed (drivers/md/dm-rq.c:187).
> > 182  * the md may be freed in dm_put() at the end of this function.
> > 183  * Or do dm_get() before calling this function and dm_put() later.
> > 184  */
> > 185 static void rq_completed(struct mapped_device *md, int rw, bool 
> > run_queue)
> > 186 {
> > 187 struct request_queue *q = md->queue;
> > 188 unsigned long flags;
> > 189
> > 190 atomic_dec(>pending[rw]);
> > 191
> > 
> > (gdb) disas rq_completed
> > Dump of assembler code for function rq_completed:
> >0xdd00 <+0>: push   %rbp
> >0xdd01 <+1>: movslq %esi,%rsi
> >0xdd04 <+4>: mov%rsp,%rbp
> >0xdd07 <+7>: push   %r13
> >0xdd09 <+9>: mov%edx,%r13d
> >0xdd0c <+12>:push   %r12
> >0xdd0e <+14>:push   %rbx
> >0xdd0f <+15>:mov%rdi,%rbx
> >0xdd12 <+18>:mov0x288(%rdi),%r12
> >0xdd19 <+25>:lock decl 0x350(%rdi,%rsi,4)
> > 
> > So this was caused by an attempt to dereference %rdi = 0x6b6b6b6b6b6b6b6b.
> > Hence this is probably a use-after-free of struct mapped_device.
> 
> Hello Christoph and Mike,
> 
> The above crash occurs with Jens' for-next branch but not with Jens'
> for-4.11/block branch. Sorry but I think this means that the SCSI
> passthrough refactoring code is not yet ready for prime time.

I somehow missed your

Re: split scsi passthrough fields out of struct request V2

2017-01-28 Thread h...@lst.de 
On Fri, Jan 27, 2017 at 09:27:53PM +, Bart Van Assche wrote:
> Have you considered to convert all block drivers to the new
> approach and to get rid of request.special? If so, do you already
> have plans to start working on this? I'm namely wondering wheter I
> should start working on this myself.

Hi Bart,

I'd love to have all drivers move of using .special (and thus reducing
request size further).  I think the general way to do that is to convert
them to blk-mq and not using the legacy cmd_size field.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Bart Van Assche 
On Wed, 2017-01-25 at 18:25 +0100, Christoph Hellwig wrote:
> this series splits the support for SCSI passthrough commands from the
> main struct request used all over the block layer into a separate
> scsi_request structure that drivers that want to support SCSI passthough
> need to embedded as the first thing into their request-private data,
> similar to how we handle NVMe passthrough commands.
> 
> To support this I've added support for that the private data after
> request structure to the legacy request path instead, so that it can
> be treated the same way as the blk-mq path.  Compare to the current
> scsi_cmnd allocator that actually is a major simplification.
> 
> Changes since V1:
>  - fix handling of a NULL sense pointer in __scsi_execute
>  - clean up handling of the flush flags in the block layer and MD
>  - additional small cleanup in dm-rq

Hello Christoph,

A general comment: patch "block: allow specifying size for extra
command data" is a very welcome improvement but unfortunately also
introduces an inconsistency among block drivers. This patch series
namely creates two kinds of block drivers:
- Block drivers that use the block layer core to allocate
  request-private data. These block drivers set request.cmd_size
  to a non-zero value and do not need request.special.
- Block drivers that allocate request-private data themselves.
  These block drivers set request.cmd_size to zero and use
  request.special to translate a request pointer into the private
  data pointer.

Have you considered to convert all block drivers to the new
approach and to get rid of request.special? If so, do you already
have plans to start working on this? I'm namely wondering wheter I
should start working on this myself.

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Jens Axboe 
On 01/27/2017 09:34 AM, Christoph Hellwig wrote:
> On Fri, Jan 27, 2017 at 09:27:02AM -0700, Jens Axboe wrote:
>> Feel free to repost it, I have no problem rebasing that branch as it's
>> standalone for now.
> 
> Ok, I'll repost what I have right now, which is on top of a merge
> of your block/for-4.11/next and your for-next from this morning
> my time.

Perfect.

> Btw, I disagred with your patch to use op_is_flush in
> generic_make_request_checks - given that we clear these flags just
> below I think using the helper obsfucates what's really going on.

Why? It's the exact same check. The ugly part is the fact that
we strip the flags later on, imho.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Jens Axboe 
On 01/27/2017 09:17 AM, Christoph Hellwig wrote:
> On Fri, Jan 27, 2017 at 09:11:14AM -0700, Jens Axboe wrote:
>> I've queued this up for 4.11. Since some of the patches had dependencies
>> on changes in master since for-4.11/block was forked, they are sitting
>> in a separate branch that has both for-4.11/block and v4.10-rc5 pulled
>> in first. for-next has everything, as usual.
> 
> Eww.  I just had a couple non-trivial updates that I now do again.
> In case you haven't pushed it out yet can you let me repost first?

Why the eww?! You can't fix this with a repost.

It's fine, I'll just ship off for-4.11/block first (as usual), then
for-4.11/rq-refactor.

The two issues is in virtio_blk and raid1. For some reason, raid1
included a refactor of a function later in the cycle (hrmpf). So there's
really no good way to solve this, unless I pull in v4.10-rc5 into
for-4.11/block.  And I don't want to do that. Hence the topic branch for
this work.

I have pushed it out, but it's not merged into for-next yet, it's just
standalone. When I've done some sanity testing, I'll push it out.


-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Christoph Hellwig 
On Fri, Jan 27, 2017 at 09:38:40AM -0700, Jens Axboe wrote:
> > Ok, I'll repost what I have right now, which is on top of a merge
> > of your block/for-4.11/next and your for-next from this morning
> > my time.
> 
> Perfect.

At least I tried, looks like the mail server is overloaded and crapped
out three mails into it.  For now there is a git tree here:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/block-pc-refactor

> 
> > Btw, I disagred with your patch to use op_is_flush in
> > generic_make_request_checks - given that we clear these flags just
> > below I think using the helper obsfucates what's really going on.
> 
> Why? It's the exact same check. The ugly part is the fact that
> we strip the flags later on, imho.

But before it was pretty obvious that it clears exactly the flags checked
two lines earlier.  Now it's not as obvious.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Christoph Hellwig 
On Fri, Jan 27, 2017 at 09:21:46AM -0700, Jens Axboe wrote:
> On 01/27/2017 09:17 AM, Christoph Hellwig wrote:
> > On Fri, Jan 27, 2017 at 09:11:14AM -0700, Jens Axboe wrote:
> >> I've queued this up for 4.11. Since some of the patches had dependencies
> >> on changes in master since for-4.11/block was forked, they are sitting
> >> in a separate branch that has both for-4.11/block and v4.10-rc5 pulled
> >> in first. for-next has everything, as usual.
> > 
> > Eww.  I just had a couple non-trivial updates that I now do again.
> > In case you haven't pushed it out yet can you let me repost first?
> 
> Why the eww?! You can't fix this with a repost.

Not because of the merge, mostly because I just spent two same
time adding all the ACKs, fixing typos and adding the removal of
the ->cmd tracing to the series and was getting ready for a repost.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Jens Axboe 
On 01/27/2017 09:23 AM, Christoph Hellwig wrote:
> On Fri, Jan 27, 2017 at 09:21:46AM -0700, Jens Axboe wrote:
>> On 01/27/2017 09:17 AM, Christoph Hellwig wrote:
>>> On Fri, Jan 27, 2017 at 09:11:14AM -0700, Jens Axboe wrote:
 I've queued this up for 4.11. Since some of the patches had dependencies
 on changes in master since for-4.11/block was forked, they are sitting
 in a separate branch that has both for-4.11/block and v4.10-rc5 pulled
 in first. for-next has everything, as usual.
>>>
>>> Eww.  I just had a couple non-trivial updates that I now do again.
>>> In case you haven't pushed it out yet can you let me repost first?
>>
>> Why the eww?! You can't fix this with a repost.
> 
> Not because of the merge, mostly because I just spent two same
> time adding all the ACKs, fixing typos and adding the removal of
> the ->cmd tracing to the series and was getting ready for a repost.

Feel free to repost it, I have no problem rebasing that branch as it's
standalone for now.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Christoph Hellwig 
On Fri, Jan 27, 2017 at 09:11:14AM -0700, Jens Axboe wrote:
> I've queued this up for 4.11. Since some of the patches had dependencies
> on changes in master since for-4.11/block was forked, they are sitting
> in a separate branch that has both for-4.11/block and v4.10-rc5 pulled
> in first. for-next has everything, as usual.

Eww.  I just had a couple non-trivial updates that I now do again.
In case you haven't pushed it out yet can you let me repost first?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-27 Thread Jens Axboe 
On Wed, Jan 25 2017, Christoph Hellwig wrote:
> Hi all,
> 
> this series splits the support for SCSI passthrough commands from the
> main struct request used all over the block layer into a separate
> scsi_request structure that drivers that want to support SCSI passthough
> need to embedded as the first thing into their request-private data,
> similar to how we handle NVMe passthrough commands.
> 
> To support this I've added support for that the private data after
> request structure to the legacy request path instead, so that it can
> be treated the same way as the blk-mq path.  Compare to the current
> scsi_cmnd allocator that actually is a major simplification.
> 
> Changes since V1:
>  - fix handling of a NULL sense pointer in __scsi_execute
>  - clean up handling of the flush flags in the block layer and MD
>  - additional small cleanup in dm-rq

I've queued this up for 4.11. Since some of the patches had dependencies
on changes in master since for-4.11/block was forked, they are sitting
in a separate branch that has both for-4.11/block and v4.10-rc5 pulled
in first. for-next has everything, as usual.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-26 Thread Jens Axboe 
On 01/26/2017 11:59 AM, h...@lst.de wrote:
> On Thu, Jan 26, 2017 at 11:57:36AM -0700, Jens Axboe wrote:
>> It's against my for-4.11/block, which you were running under Christoph's
>> patches. Maybe he's using an older version? In any case, should be
>> pretty trivial for you to hand apply. Just ensure that .flags is set to
>> 0 for the common cases, and inherit 'flags' when it is passed in.
> 
> No, the flush op cleanups you asked for last round create a conflict
> with your patch.  They should be trivial to fix, though.

Ah, makes sense. And yes, as I said, should be trivial to hand apply the
hunk that does fail.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-26 Thread Jens Axboe 
On 01/26/2017 11:29 AM, Bart Van Assche wrote:
> On Wed, 2017-01-25 at 18:25 +0100, Christoph Hellwig wrote:
>> Hi all,
>>
>> this series splits the support for SCSI passthrough commands from the
>> main struct request used all over the block layer into a separate
>> scsi_request structure that drivers that want to support SCSI passthough
>> need to embedded as the first thing into their request-private data,
>> similar to how we handle NVMe passthrough commands.
>>
>> To support this I've added support for that the private data after
>> request structure to the legacy request path instead, so that it can
>> be treated the same way as the blk-mq path.  Compare to the current
>> scsi_cmnd allocator that actually is a major simplification.
>>
>> Changes since V1:
>>  - fix handling of a NULL sense pointer in __scsi_execute
>>  - clean up handling of the flush flags in the block layer and MD
>>  - additional small cleanup in dm-rq
> 
> Hello Christoph,
> 
> Thanks for having fixed the NULL pointer issue I had reported for v1.
> However, if I try to run my srp-test testsuite on top of your
> hch-block/block-pc-refactor branch (commit ID a07dc3521034) merged
> with v4.10-rc5 the following appears on the console:

I think this may be my bug - does the below help?


diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index d05061f27bb1..56b92db944ae 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -117,7 +117,7 @@ struct request *blk_mq_sched_get_request(struct 
request_queue *q,
ctx = blk_mq_get_ctx(q);
hctx = blk_mq_map_queue(q, ctx->cpu);
 
-   blk_mq_set_alloc_data(data, q, 0, ctx, hctx);
+   blk_mq_set_alloc_data(data, q, data->flags, ctx, hctx);
 
if (e) {
data->flags |= BLK_MQ_REQ_INTERNAL;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index dcb567642db7..9e4ed04f398c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -253,7 +253,7 @@ EXPORT_SYMBOL_GPL(__blk_mq_alloc_request);
 struct request *blk_mq_alloc_request(struct request_queue *q, int rw,
unsigned int flags)
 {
-   struct blk_mq_alloc_data alloc_data;
+   struct blk_mq_alloc_data alloc_data = { .flags = flags };
struct request *rq;
int ret;
 
@@ -1382,7 +1382,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue 
*q, struct bio *bio)
 {
const int is_sync = op_is_sync(bio->bi_opf);
const int is_flush_fua = bio->bi_opf & (REQ_PREFLUSH | REQ_FUA);
-   struct blk_mq_alloc_data data;
+   struct blk_mq_alloc_data data = { 0, };
struct request *rq;
unsigned int request_count = 0, srcu_idx;
struct blk_plug *plug;
@@ -1504,7 +1504,7 @@ static blk_qc_t blk_sq_make_request(struct request_queue 
*q, struct bio *bio)
const int is_flush_fua = bio->bi_opf & (REQ_PREFLUSH | REQ_FUA);
struct blk_plug *plug;
unsigned int request_count = 0;
-   struct blk_mq_alloc_data data;
+   struct blk_mq_alloc_data data = { 0, };
struct request *rq;
blk_qc_t cookie;
unsigned int wb_acct;

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-26 Thread h...@lst.de 
On Thu, Jan 26, 2017 at 11:57:36AM -0700, Jens Axboe wrote:
> It's against my for-4.11/block, which you were running under Christoph's
> patches. Maybe he's using an older version? In any case, should be
> pretty trivial for you to hand apply. Just ensure that .flags is set to
> 0 for the common cases, and inherit 'flags' when it is passed in.

No, the flush op cleanups you asked for last round create a conflict
with your patch.  They should be trivial to fix, though.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-26 Thread Jens Axboe 
On 01/26/2017 11:52 AM, Bart Van Assche wrote:
> On Thu, 2017-01-26 at 11:44 -0700, Jens Axboe wrote:
>> I think this may be my bug - does the below help?
> 
> Hello Jens,
> 
> What tree has that patch been generated against? It does not apply
> cleanly on top of Christoph's tree:
> 
> $ git checkout hch-block-pc-refactor
> $ patch -p1 --dry-run -f -s < 
> ~/Re\:_split_scsi_passthrough_fields_out_of_struct_request_V2.mbox
> 1 out of 3 hunks FAILED

It's against my for-4.11/block, which you were running under Christoph's
patches. Maybe he's using an older version? In any case, should be
pretty trivial for you to hand apply. Just ensure that .flags is set to
0 for the common cases, and inherit 'flags' when it is passed in.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-26 Thread Bart Van Assche 
On Thu, 2017-01-26 at 11:44 -0700, Jens Axboe wrote:
> I think this may be my bug - does the below help?

Hello Jens,

What tree has that patch been generated against? It does not apply
cleanly on top of Christoph's tree:

$ git checkout hch-block-pc-refactor
$ patch -p1 --dry-run -f -s < 
~/Re\:_split_scsi_passthrough_fields_out_of_struct_request_V2.mbox
1 out of 3 hunks FAILED

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: split scsi passthrough fields out of struct request V2

2017-01-26 Thread Bart Van Assche 
On Wed, 2017-01-25 at 18:25 +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series splits the support for SCSI passthrough commands from the
> main struct request used all over the block layer into a separate
> scsi_request structure that drivers that want to support SCSI passthough
> need to embedded as the first thing into their request-private data,
> similar to how we handle NVMe passthrough commands.
> 
> To support this I've added support for that the private data after
> request structure to the legacy request path instead, so that it can
> be treated the same way as the blk-mq path.  Compare to the current
> scsi_cmnd allocator that actually is a major simplification.
> 
> Changes since V1:
>  - fix handling of a NULL sense pointer in __scsi_execute
>  - clean up handling of the flush flags in the block layer and MD
>  - additional small cleanup in dm-rq

Hello Christoph,

Thanks for having fixed the NULL pointer issue I had reported for v1.
However, if I try to run my srp-test testsuite on top of your
hch-block/block-pc-refactor branch (commit ID a07dc3521034) merged
with v4.10-rc5 the following appears on the console:

[  707.317403] BUG: scheduling while atomic: fio/9073/0x0003
[  707.317404] 1 lock held by fio/9073:
[  707.317404]  #0:  (rcu_read_lock){..}, at: [] 
__blk_mq_run_hw_queue+0xde/0x1c0
[  707.317409] Modules linked in: dm_service_time ib_srp scsi_transport_srp 
target_core_user uio target_core_pscsi target_core_file ib_srpt 
target_core_iblock target_core_mod brd netconsole xt_CHECKSUM iptable_mangle 
ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat libcrc32c 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT 
nf_reject_ipv4 xt_tcpudp tun bridge stp llc ebtable_filter ebtables 
ip6table_filter ip6_tables iptable_filter ip_tables x_tables af_packet ib_ipoib 
rdma_ucm ib_ucm ib_uverbs ib_umad rdma_cm configfs ib_cm iw_cm msr mlx4_ib 
ib_core sb_edac edac_core x86_pkg_temp_thermal intel_powerclamp coretemp 
ipmi_ssif kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul mlx4_core 
crc32c_intel ghash_clmulni_intel hid_generic pcbc usbhid iTCO_wdt tg3 
aesni_intel
[  707.317445]  ptp iTCO_vendor_support aes_x86_64 crypto_simd pps_core 
glue_helper dcdbas ipmi_si ipmi_devintf libphy devlink lpc_ich cryptd pcspkr 
ipmi_msghandler mfd_core fjes mei_me tpm_tis button tpm_tis_core shpchp mei tpm 
wmi mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt 
fb_sys_fops ttm sr_mod cdrom drm ehci_pci ehci_hcd usbcore usb_common sg 
dm_multipath dm_mod scsi_dh_rdac scsi_dh_emc scsi_dh_alua autofs4
[  707.317469] CPU: 6 PID: 9073 Comm: fio Tainted: GW   
4.10.0-rc5-dbg+ #1
[  707.317470] Hardware name: Dell Inc. PowerEdge R430/03XKDV, BIOS 1.0.2 
11/17/2014
[  707.317470] Call Trace:
[  707.317473]  dump_stack+0x68/0x93
[  707.317475]  __schedule_bug+0x5b/0x80
[  707.317477]  __schedule+0x762/0xb00
[  707.317479]  schedule+0x38/0x90
[  707.317481]  schedule_timeout+0x2fe/0x640
[  707.317491]  io_schedule_timeout+0x9f/0x110
[  707.317493]  blk_mq_get_tag+0x158/0x260
[  707.317496]  __blk_mq_alloc_request+0x16/0xe0
[  707.317498]  blk_mq_sched_get_request+0x30d/0x360
[  707.317502]  blk_mq_alloc_request+0x3b/0x90
[  707.317505]  blk_get_request+0x2f/0x110
[  707.317507]  multipath_clone_and_map+0xcd/0x140 [dm_multipath]
[  707.317512]  map_request+0x3c/0x290 [dm_mod]
[  707.317517]  dm_mq_queue_rq+0x77/0x100 [dm_mod]
[  707.317519]  blk_mq_dispatch_rq_list+0x1ff/0x320
[  707.317521]  blk_mq_sched_dispatch_requests+0xa9/0xe0
[  707.317523]  __blk_mq_run_hw_queue+0x122/0x1c0
[  707.317528]  blk_mq_run_hw_queue+0x84/0x90
[  707.317530]  blk_mq_flush_plug_list+0x39f/0x480
[  707.317531]  blk_flush_plug_list+0xee/0x270
[  707.317533]  blk_finish_plug+0x27/0x40
[  707.317534]  do_io_submit+0x475/0x900
[  707.317537]  SyS_io_submit+0xb/0x10
[  707.317539]  entry_SYSCALL_64_fastpath+0x18/0xad

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html