Re: [PATCH] block: don't read block device if it's invalid

From: Christoph Hellwig <hch@infradead.org>
To: Xianting Tian <xianting_tian@126.com>
Cc: axboe@kernel.dk, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org
Subject: Re: [PATCH] block: don't read block device if it's invalid
Date: Mon, 17 Aug 2020 07:42:36 +0100	[thread overview]
Message-ID: <20200817064236.GA22917@infradead.org> (raw)
In-Reply-To: <1597153386-87954-1-git-send-email-xianting_tian@126.com>

On Tue, Aug 11, 2020 at 09:43:06AM -0400, Xianting Tian wrote:
> We found several processes in 'D' state after nvme device hot-removed,
> The call trace as below, we can see process 848 got lock 'bdev->bd_mutex'
> in blkdev_reread_part(), but scheduled out due to wait for IO done. But
> the IO won't be completed as the device is hot-removed. Then it caused
> the lock 'bdev->bd_mutex' can't be unlocked. As a result, it caused
> other processes, which need to get the same lock 'bdev->bd_mutex',
> blocked on this lock.
> 
> When nvme device hot-removed, kernel will start a thread to handle the
> task of nvme device removing, as the call trace of process 1111504 shows
> below. I listed the call trace of nvme_kill_queues() in detail as below,
> we can see 'NVME_NS_DEAD' is set, then when executing
> nvme_revalidate_disk(), it found 'NVME_NS_DEAD' is set and
> 'set_capacity(disk, 0)' will be called to set disk capacity to 0.
>     nvme_kill_queues()
>         if (test_and_set_bit(NVME_NS_DEAD, &ns->flags)) return;
>             revalidate_disk(disk)
>                 disk->fops->revalidate_disk(disk) <=for nvme device, revalidate_disk=nvme_revalidate_disk()
>                      mutex_lock(&bdev->bd_mutex)
> 
> This patch is to reduce the probability of such problem. Before getting
> the lock of 'bdev->bd_mutex' in blkdev_reread_part(), add the code to
> check if the capacity of the disk is 0, just return. Then we can avoid
> the happen of the issue:
> nvme device is hot-removed, and its capacity is alreday set to 0; then
> if there is process like 848 want to read the device, it will return
> directly in blkdev_reread_part(), then it will not get the lock
> "bdev->bd_mutex", which can't be unlocked by the process itself as IO
> can't be completed.

We need to fix this for real, as you stated at best this reduces the
window that the race can happen.

I think our main problem is that due to bd_mutex we can't update the
block device size from arbitrary context.  If we instead add an irqsave
spinlock just for the size we'd get rid of the limitation and can stop
papering over the problem.  Give m a little time to try to do that.