Re: New Patch series for the UBD Driver

[Jens Axboe <axboe@kernel.dk>]

On 11/12/18 11:23 AM, Anton Ivanov wrote:
> 
> On 11/12/18 6:14 PM, Jens Axboe wrote:
>> On 11/12/18 11:12 AM, Anton Ivanov wrote:
>>> On 11/12/18 6:04 PM, Jens Axboe wrote:
>>>> On 11/12/18 11:00 AM, Anton Ivanov wrote:
>>>>> On 11/12/18 5:50 PM, Jens Axboe wrote:
>>>>>> On 11/12/18 10:41 AM, anton.ivanov@cambridgegreys.com wrote:
>>>>>>> This is the revised patchset for the ubd driver. Known issues:
>>>>>>>
>>>>>>> 1. If an error is returned to an operation different from READ/WRITE/SYNC
>>>>>>> the block subsystem goes south finishing with a crash. I have tried everything
>>>>>>> I can think of and whatever I could lift out of the loop and nbd driver. I do
>>>>>>> not think that the problem is in the ubd driver. I am lost here, Jens can you
>>>>>>> please give me a hand in handling the case when ubd_handler() gets an error in
>>>>>>> reply to a discard.
>>>>>> It would help if you showed a crash from doing that, and an example of
>>>>>> how you are doing that.
>>>>> Test case - 1G ext4 image on MS DOS filesystem.
>>>>>
>>>>> With the present patch, the logic around line 844 should be commented
>>>>> out so that DISCARD is always enabled. I can send a patch where this is
>>>>> always enabled for testing purposes.
>>>>>
>>>>> Mount filesystem - mount /dev/ubdb /mnt
>>>>>
>>>>> Try fstrim /mnt
>>>>>
>>>>> Crash is immediate,  but different in each case.
>>>>>
>>>>> Some cases contain the WARN from line 439 in blk.h . Some contain
>>>>> spinlock recursion BUG(). Some do not contain anything - it just
>>>>> crashes. Or hangs.
>>>>>
>>>>> I tried various combinations (and code theft from loop and nbd) of
>>>>> blk_update_request, __blk_mq_end_request, blk_mq_end_request, etc - the
>>>>> flavor of the crash changes a bit, but not the essence.
>>>> Sorry, all of this isn't very clear to me. It definitely sounds like
>>>> you are doing something wrong. Where are you calling falloc_punch()
>>>> from?
>>>   From the IO thread. It sets the error code and writes to the IPC pipe
>>> which triggers an interrupt. Same as with any other disk IO in UML/UBD.
>>>
>>>> It's not even safe from ->queue_rq(), you'd need to add
>>>> BLK_MQ_F_BLOCKING to your mq_ops flags to ensure that blk-mq
>>>> always calls you from a context that can block. If you're doing
>>>> it from the irq handler, well then...
>>> The only thing the IRQ handler does is to process the completion request.
>>>
>>> That works OK for requests which have no error.
>> Note the warning above on blocking from ->queue()rq...
> 
> This is already of help. The closest to UML UBD is NBD. The difference 
> is that there you talk to a remote server, while in UML you talk to an 
> IO thread which does all the IO.
> 
> NBD already has MQ_F_BLOCKING enabled, I will have a look at it in more 
> detail.
> 
>>
>>> It also works fine (no crashes) for requests which return an error to
>>> the FSYNC call. I did not do proper exhaustive testing with error
>>> injection, but based on cursory testing read/write error handling also
>>> seems to work (do not take my word for it, I will write a proper "error
>>> injection" harness tomorrow to retest that).
>>>
>>> The issue shows immediately when processing the completion of a DISCARD
>>> request which has an error set. DISCARD requests with no error set are fine.
>> Please share some of the crashes, I can't help you with this without
>> having something concrete.
> 
> root@stretch-opx:~# fstrim /mnt
> [  135.160000] do_io - discard failed err = 95 fd = 23
> [  135.160000] do_io - discard failed err = 95 fd = 23
> [  135.160000]  [<600850f0>] ? prepare_to_wait+0x0/0x80
> [  135.170000] ------------[ cut here ]------------
> [  135.170000] WARNING: CPU: 0 PID: 1151 at block/blk.h:439 
> generic_make_request_checks+0x3b5/0x680
> [  135.170000] Modules linked in:
> [  135.170000] CPU: 0 PID: 1151 Comm: fstrim Tainted: G W         
> 4.20.0-rc1-00005-g046844eea47e-dirty #121
> [  135.170000] Stack:
> [  135.170000]  d3821740 60744258 d3821788 00000000
> [  135.170000]  00000009 00000000 d3821750 607442ac
> [  135.170000]  d38217c0 6005034e d3821800 600e5902
> [  135.170000] Call Trace:
> [  135.170000]  [<603ffec5>] ? generic_make_request_checks+0x3b5/0x680
> [  135.170000]  [<6009271a>] ? printk+0x0/0x94
> [  135.170000]  [<6002a849>] show_stack+0x129/0x190
> [  135.170000]  [<60744258>] ? dump_stack_print_info+0xe8/0x100
> [  135.170000]  [<607442ac>] dump_stack+0x2a/0x2e
> [  135.170000]  [<6005034e>] __warn+0x13e/0x170
> [  135.170000]  [<600e5902>] ? mempool_alloc+0x92/0x1e0
> [  135.170000]  [<600504f6>] warn_slowpath_null+0x46/0x4b
> [  135.170000]  [<603ffec5>] generic_make_request_checks+0x3b5/0x680
> [  135.170000]  [<603f9783>] ? bio_alloc_bioset+0x193/0x320
> [  135.170000]  [<60044ef0>] ? set_signals+0x0/0x50
> [  135.170000]  [<60044ef0>] ? set_signals+0x0/0x50
> [  135.170000]  [<6040118f>] generic_make_request+0xdf/0x480
> [  135.170000]  [<603f92d6>] ? __bio_clone_fast+0xd6/0xe0
> [  135.170000]  [<603f9944>] ? bio_clone_fast+0x34/0x40
> [  135.170000]  [<6040961e>] blk_queue_split+0x59e/0x620
> [  135.170000]  [<60044ef0>] ? set_signals+0x0/0x50
> [  135.170000]  [<6040f84a>] blk_mq_make_request+0x7a/0x400
> [  135.170000]  [<600e5902>] ? mempool_alloc+0x92/0x1e0
> [  135.170000]  [<603fae90>] ? bio_endio+0x0/0x130
> [  135.170000]  [<604014cd>] generic_make_request+0x41d/0x480
> [  135.170000]  [<6040170f>] submit_bio+0x1df/0x1f0
> [  135.170000]  [<603f8348>] submit_bio_wait+0x68/0x90
> [  135.170000]  [<6040bd22>] blkdev_issue_discard+0x72/0xb0
> [  135.170000]  [<602174f0>] ext4_trim_all_free+0x350/0x680
> [  135.170000]  [<607ccc90>] ? _raw_spin_unlock+0x0/0x20
> [  135.170000]  [<6002ce80>] ? copy_chunk_from_user+0x0/0x30
> [  135.170000]  [<6021b5af>] ext4_trim_fs+0x1cf/0x270
> [  135.170000]  [<602109cf>] ext4_ioctl+0x1cbf/0x1f40
> [  135.170000]  [<60152e08>] ? vfs_getattr_nosec+0x68/0x90
> [  135.170000]  [<60153163>] ? cp_new_stat+0x163/0x180
> [  135.170000]  [<60165b18>] do_vfs_ioctl+0x798/0xa40
> [  135.170000]  [<60165e23>] ksys_ioctl+0x63/0xa0
> [  135.170000]  [<6078def0>] ? ptrace+0x0/0xa0
> [  135.170000]  [<60165e70>] sys_ioctl+0x10/0x20
> [  135.170000]  [<6002ce58>] handle_syscall+0x98/0xc0
> [  135.170000]  [<6075be10>] ? __waitpid+0x0/0xa0
> [  135.170000]  [<600482d6>] userspace+0x4c6/0x5b0
> [  135.170000]  [<600438af>] ? save_registers+0x1f/0x50
> [  135.170000]  [<6004b747>] ? arch_prctl+0x117/0x1c0
> [  135.170000]  [<6005c587>] ? calculate_sigpending+0x77/0x80
> [  135.170000]  [<60029204>] fork_handler+0x94/0xa0
> [  135.170000]
> [  135.170000] ---[ end trace 24ca562b91e6dbdf ]---
> 
> And hang. Let me try a few more times to see if I can get the spinlock 
> recursion and/or abort instead of hang case (they are more difficult to 
> reproduce).

This shows exactly what I was talking about, you are trying to
issue IO from irq/non-blocking context. If you look up the warning,
that's exactly what it is telling you.

-- 
Jens Axboe


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um