Re: EXT4 Oops (Re: [PATCH V15 06/22] mmc: block: Add blk-mq support)

From: Dmitry Osipenko <digetx@gmail.com>
To: Andreas Dilger <adilger@dilger.ca>, "Theodore Ts'o" <tytso@mit.edu>
Cc: Adrian Hunter <adrian.hunter@intel.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	linux-mmc <linux-mmc@vger.kernel.org>,
	linux-block <linux-block@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Bough Chen <haibo.chen@nxp.com>,
	Alex Lemberg <alex.lemberg@sandisk.com>,
	Mateusz Nowak <mateusz.nowak@intel.com>,
	Yuliy Izrailov <Yuliy.Izrailov@sandisk.com>,
	Jaehoon Chung <jh80.chung@samsung.com>,
	Dong Aisheng <dongas86@gmail.com>,
	Das Asutosh <asutoshd@codeaurora.org>,
	Zhangfei Gao <zhangfei.gao@gmail.com>,
	Sahitya Tummala <stummala@codeaurora.org>,
	Harjani Ritesh <riteshh@codeaurora.org>,
	Venu Byravarasu <vbyravarasu@nvidia.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Christoph Hellwig <hch@lst.de>,
	Thierry Reding <treding@nvidia.com>,
	Krishna Reddy <vdumpa@nvidia.com>,
	linux-ext4 <linux-ext4@vger.kernel.org>
Subject: Re: EXT4 Oops (Re: [PATCH V15 06/22] mmc: block: Add blk-mq support)
Date: Fri, 2 Mar 2018 19:39:25 +0300	[thread overview]
Message-ID: <24166154-8b08-bab6-ac8e-618eeba30674@gmail.com> (raw)
In-Reply-To: <AF3403B9-9791-4A77-A6FB-3281B8EA7A31@dilger.ca>

On 01.03.2018 23:20, Andreas Dilger wrote:
> 
> On Mar 1, 2018, at 9:04 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> This doesn't seem to make sense; the PC is where we are currently
>> executing, and LR is the "Link Register" where the flow of control
>> will be returning after the current function returns, right?  Well,
>> dx_probe should *not* be returning to __wait_on_bit().  So this just
>> seems.... weird.
>>
>> Ignoring the LR register, this stack trace looks sane...  I can't see
>> which pointer could be NULL and getting dereferenced, though.  How
>> easily can you reproduce the problem?  Can you either (a) translate
>> the PC into a line number, or better yet, if you can reproduce, add a
>> series of BUG_ON's so we can see what's going on?

Ted, thank you for the suggestion. I don't have a bug-reproducer, it happens
only under some IO load and quite randomly. I've applied the BUG_ON()'s, but it
may take some time to catch the bug again.

>> +	BUG_ON(frame);
> 
> I think you mean:
> 	BUG_ON(frame == NULL);
> or
> 	BUG_ON(!frame);
> 
> 
>> 	memset(frame_in, 0, EXT4_HTREE_LEVEL * sizeof(frame_in[0]));
>> 	frame->bh = ext4_read_dirblock(dir, 0, INDEX);
>> 	if (IS_ERR(frame->bh))
>> 		return (struct dx_frame *) frame->bh;
>>
>> +	BUG_ON(frame->bh);
>> +	BUG_ON(frame->bh->b_data);
> 
> Same here.
> 
> 	BUG_ON(frame->bh == NULL);
> 	BUG_ON(frame->bh->b_data == NULL);
> 
> This is why I don't like implicit "is NULL" or "is non-zero" usage.  Lustre
> used to require "== NULL" or "!= NULL" to avoid bugs like this, but had to
> abandon that because of upstream code style.

Well spotted, thanks Andreas.

>> 	root = (struct dx_root *) frame->bh->b_data;
>> 	if (root->info.hash_version != DX_HASH_TEA &&
>> 	    root->info.hash_version != DX_HASH_HALF_MD4 &&
>> 	    root->info.hash_version != DX_HASH_LEGACY) {
>>
>> These are "could never" happen scenarios from looking at the code, but
>> that will help explain what is going on.
>>
>> If this is reliably only happening with mq, the only way I could see
>> that if is something is returning an error when it previously wasn't.
>> This isn't a problem we're seeing with any of our testing, though.