Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Feb 2022 13:59:37 +0100
From:   Markus =?utf-8?Q?Bl=C3=B6chl?= <Markus.Bloechl@ipetronik.com>
To:     Christoph Hellwig <hch@lst.de>, Hannes Reinecke <hare@suse.de>
Cc:     Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@kernel.dk>,
        Sagi Grimberg <sagi@grimberg.me>,
        linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, Stefan Roese <sr@denx.de>
Subject: Re: [RFC PATCH] nvme: prevent hang on surprise removal of NVMe disk
Message-ID: <20220216125937.d6brzu7labgywxcg@ipetronik.com>
References: <20220214095107.3t5en5a3tosaeoo6@ipetronik.com>
 <20220215201738.GA26945@lst.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220215201738.GA26945@lst.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?FoJClzlxgdOPLIydC78Z9LSt9OzL/ulsnHU3kuk3Xn4ZVR9qNNFzx2VQQrgz?=
 =?us-ascii?Q?g7QUjRa15OvbwJ9niPcwBoUhc6mjMraw6tvEv7OlOz9bl8TSaLuHJaZaFGEO?=
 =?us-ascii?Q?HJ8wurpRb0NY8kJrpSeBWQLLK6Ibr8YzBlZjhnggk6EwE+OkzpHjTBwweCLg?=
 =?us-ascii?Q?GtluGkE3VFo6EjUxltOH6OdRov1QHqB0xwUth8ks+KiFu5phjMQcIzzQVDWJ?=
 =?us-ascii?Q?KhydwRYFdamf+J2/xwWi8xV31h2mWhS3+8dJtMu90FvO9p1FeaiNkwzjzrTQ?=
 =?us-ascii?Q?CaQCJ7/WTYD1yWhH9YA03xuTNCTQTXB0sWQl6mYrd92L5UsOrcc6fgZVUyhY?=
 =?us-ascii?Q?REStwIbX8i4xkXxKPjqUt2/rJNVgbFULbYKgl2YrUQe/46bIEa+6XbVNS8ae?=
 =?us-ascii?Q?Hfjd5+01IRHI5mR/+8yyra0Yq+GlR6qEt4SFbhcbflhK72Gsi3UD4D49onmj?=
 =?us-ascii?Q?TxVIijwkbiPtvTB50HjOArzUjICKmD7VVh422IZl7vRd9n4Wi/YfzAEENCBQ?=
 =?us-ascii?Q?Hoe/eucTqrERJNHF8S+JAHec3Yph+Iu9q9AuA2nF6ki7PTUXAmt9r5EA8dqA?=
 =?us-ascii?Q?9jdd7q5Ugg09wzRVvpDy0iNuLjEIIZ47HCyyYJQtleSo6d4flaNWJyAoAWaz?=
 =?us-ascii?Q?1EuM2MbvhCrFxnAyuuCfWwd2eOslxdQCAizj7uPaaF4+YJ5FbQBtr23PC8Ff?=
 =?us-ascii?Q?lCfw5ZLNChHNzgn3eynqpC2KujhMx2xdmVjoh3ShxTo1rIrKRXpVYDwVHO45?=
 =?us-ascii?Q?jvERNA/9112h3Lla0Hj81Wqa0hs6HfkTnNA54WWlrBfa2OC+s3l2r5CJhHzQ?=
 =?us-ascii?Q?1AJ8iO8+RrxVlytDjzGpRHutLi7L7k0++9DQXUhpXpsRNUj2omPkcvhnxEjg?=
 =?us-ascii?Q?oP7ooRDOCqNjpWseV4ZXj29nESvWddD5YXrVOHZkejWfITDDplHuxEXDhkfD?=
 =?us-ascii?Q?t6AiNWr2xbuinecqTcLHohTQRF5L2UFH2Si6+kXdheUFoXKve6CImvR90DoS?=
 =?us-ascii?Q?pl1T/9jL4OZZ7CPD0im+PpNxIYgY3qzq38HBgHA6ZwKNNCxAiDHqboGIAg4f?=
 =?us-ascii?Q?zdOiQc2v07pHjOvKzaSWwAO3xYihIYUabWNOn/1mMQnjAtrLqMZUgpe1EBPu?=
 =?us-ascii?Q?Wi3PD/P8CoI8uLKUzfp3TCHI9AxZur2yu1yraphcw6BNJoN4LPyM5lpGn7NZ?=
 =?us-ascii?Q?OIm3yVOqyb8BqVahFZ4Dr+PTCVFjdGmSFfSKI6NcnHGqGn8+hCZyAgX7J9Tm?=
 =?us-ascii?Q?SW09ob8oHB/KXQSn59E88NacL0YuAt5PUMwI6iN3eNmZ4Tcnlba8dXoNugy9?=
 =?us-ascii?Q?v32twdPgAGiYE9v2hN00Dhc/hy8z0lsbDUXUhPeVhz7yIYEgMjm84lhcE9zS?=
 =?us-ascii?Q?5NXZ6fwx2FpShBtqfNrDZO4aA5KPW1N3AvShSxHELqDZfBdVJQU4IVC6Z/0O?=
 =?us-ascii?Q?cDSa9Ya5tXYflsstwIn1Sz/QsQgZLSqmLLYKKX6WuJSoJkCBtaExX9opMV7N?=
 =?us-ascii?Q?X7vR8mu1KXpAAtlNkPJlKexFdCmN8V1e2y/EKvgewMOgrabb+egcksMAQdD3?=
 =?us-ascii?Q?Rb98ErG8JqCL/XlRerqdjvSUXZLhTrrhqT+7qiDmSN0x6ZKvqRxbcQzPyayE?=
 =?us-ascii?Q?q3S8xMdVTtmrMp1LrzccwbViEZoBQHSdB+ZkHdvqF41GFkF0Uba4imaQyDK/?=
 =?us-ascii?Q?DMnugNZ9A7D7kw2KuTl9suC0dgWzk0gt2mMw6izKltxzIyiGcjidC9VauKlw?=
 =?us-ascii?Q?W8UeMH7cVWNmTOBf1dSXkdobLyol1sM=3D?=
X-OriginatorOrg: ipetronik.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 09ba03df-79fd-4efa-69a1-08d9f14c3026
X-MS-Exchange-CrossTenant-AuthSource: AM9P193MB1572.EURP193.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2022 12:59:37.9312
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 982fe058-7d80-4936-bdfa-9bed4f9ae127
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Udy324qWAzUEndWE+jQJJWVN68xQZ/VCZWSj6OnhNDVuacpYaD2RXarp68csTl1Kh0ihuIPJzVTsi+f6VqUXeDh9LNG2N5sAs2QLcNeSHmY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM8P193MB0865
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Feb 15, 2022 at 09:17:38PM +0100, Christoph Hellwig wrote:
> So, I think in th short run setting GD_DEAD is the right thing as a
> non-invasive fix, but I'd rather do it in blk_set_queue_dying to also
> cover the other drivers with a similar pattern.  blk_set_queue_dying
> is never used for cases where q->disk isn't valid so that should be
> fine.

blk_set_queue_dying() is called from blk_cleanup_queue(), which in turn
is called for all kinds of queues without associated disk, even ones
with failed allocations.
But according to disk_release() a disk which was once associated should
always outlive its queue, thus q->disk should indeed always be safe.


It's a little unfortunate that the existing doc on request queues was
so outdated that it had to be removed.

So I was trying to figure out if blk_queue_dying(q) should always imply
(!q->disk || test_bit(GD_DEAD, &q->disk->state)).
Assuming that there is a 1:1 relationship between q and q->disk and that
blk_queue_dying(q) was previously used where GD_DEAD is now, I do hope
so, since GD_DEAD should just be a weaker form of QUEUE_FLAG_DYING to
allow killing the disk way before the queue.


> 
> In the long run I think we just need to remove the fsync_bdev in
> del_gendisk or at least make it conditional.

But wouldn't that require you to pass a flag like SURPRISE_REMOVAL into
del_gendisk and passing it along through delete_partition()?
You would still need GD_DEAD since someone might remove the disk while
you are syncing, so this only sounds like an error-prone optimization to
me. After all, syncing on graceful removal seems to be a sensible thing
to do.


Fiddling with qemu will probably take me too long unless Hannes already
has something up his sleeves.


So I'll submit V2 after I regained access to the test setup.

It will probably just look something like this:

```
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -286,6 +286,8 @@ void blk_queue_start_drain(struct request_queue *q)

 void blk_set_queue_dying(struct request_queue *q)
 {
+       if (q->disk)
+               set_bit(GD_DEAD, &q->disk->state);
        blk_queue_flag_set(QUEUE_FLAG_DYING, q);
        blk_queue_start_drain(q);
 }
```


Thanks for all your input,

Markus