From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f182.google.com (mail-pg1-f182.google.com [209.85.215.182]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 801691BBBE5 for ; Tue, 18 Nov 2025 03:44:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.182 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763437451; cv=none; b=iW3ZuXQKE6f0blpgX5OVuA6EDtD7ko6OItvHzOzlca2r4z3U6y6XuGwfm0N9D2RYSUY0VT4wYr7dWFt7Rfx7wpCOyzDlZnZ0/59Ww5emFVWoWyuS37DKo6p5m8u/uAZ5qrnCzV99rnpSb6xnx9kUKpmdN5ljaPM9MBZB2kKsf64= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763437451; c=relaxed/simple; bh=/US2JXP4kF/4wCz3UaGOCWVf6rXm3OEtoFNFplpN7Pg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=N2d2+D9/oyD/lngwoNNGRvBDbxuJx2Wu0N1u/7g4fdrureOmsGE4/B8B3XY/sN/xnYictKbSNgZZW/KOe3seMIeSBKtjUUie/GSD9yrsV5MF7m61yoE4FHW3e5A+2S/A6JDqwLBA9YQqhurZ/SUQ6jMsNwFyvDmrR/Xi7wWdzmg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=purestorage.com; spf=fail smtp.mailfrom=purestorage.com; dkim=pass (2048-bit key) header.d=purestorage.com header.i=@purestorage.com header.b=GGYxNG6X; arc=none smtp.client-ip=209.85.215.182 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=purestorage.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=purestorage.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=purestorage.com header.i=@purestorage.com header.b="GGYxNG6X" Received: by mail-pg1-f182.google.com with SMTP id 41be03b00d2f7-b98a619f020so3841280a12.2 for ; Mon, 17 Nov 2025 19:44:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=purestorage.com; s=google2022; t=1763437448; x=1764042248; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=YuMDq3EAHV9Hf5tDPdEXp65geuqpRY/9VYhZBt32+nI=; b=GGYxNG6XP6PdxvRpT7UTFBoyfBOaX5NoT6tfKQ4vFoqeMh5z0i6XFaAjpE97y4eWCQ vVBk7PSImFAbVa156SyZUJ4Y1kgFGUDFMgYRsHgoqntugYj6uLCr0ppg7PNGwyPyV6e9 2/hUqzk45jPa14X0Glnsu53+slZnOfDD2pzu9R8Fcrw+oDrS1Vf/WRT4Dp0TK+sWKYuf OuhyHERNgEb5gXIj2TUrI2c9QQ8Z2KPW5WvqBQLj/Vk+CxB3bJF1iqwxFVgz8jWyJGnK YzZeohPPCOeEHfp8yt6U1n9VV6/e0fkaHoYS3flE+CVEFH75cT61W9zRb/sBHK5r5bve GbbA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763437448; x=1764042248; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=YuMDq3EAHV9Hf5tDPdEXp65geuqpRY/9VYhZBt32+nI=; b=mJsOgw/Bsp+3Zo+eFsDOGOA8e9Co8FLBg1R6vbFXf9iuMSpk7lQ9NkcJlGr3A4K8HS unZGOCLcWbC3dxZ6nOLG73bU8ipq8tzNbnVuFQl7qDf0fM2ZPX79A3r3NOE3yNLp9qk5 krn9BgCb0ztrr43BA28Ub3AxN4xwRnnG2hsgIQbb8LSrGq6TNMRD0ncXsHYckRXDz4ls zmK7bT3U7rqK16j8ClIoOT6Uxu5fiLaC2ignCNVea71WUHIIBwTlK3fPH4JCJg03wa7n L1i8SaWW5pdpi9x9Uv479RdItmK+avGMRbvV2MwJwsihjpih/jvyy2gg7+KNXLM/wkME sXsw== X-Forwarded-Encrypted: i=1; AJvYcCX2Tqaym/ChbgQgcn0SDErj+y5KIi6/1JMT8rTCJHXIw6lxtlrsfu7HwD+2oZbFsAjCWfKyzVkCcva+n/o=@vger.kernel.org X-Gm-Message-State: AOJu0Yx4D2U4QutNNwLxlJb5BlhznBagVdC13RNnG/f/7XCDGXF345k6 wzfdl0XJfGV/k7y0YoEselTqJ7x264nbXduqFbCTVUYJgefTpqVODvMjba/DJ0Ho4pXu2pKjWaD 7UUF0 X-Gm-Gg: ASbGncumfHqFmPM1KpR058euUCfOi6NFN8EeMg80gPyWPP0TBW3Jov9x37Ib/omk9P+ NVSWGV8edW9OrfbvY4mFhG1z6C5s2+d6EegY1g8SfHTawy/bHUzBvsjdaYlRrGdULnKqoBMrn52 BwAQa4Y0e9+7xBv1RhQU6yQ0bOVw175UYYEwpsIBUrhSqr+nMKwrzSGB7pZS5NtGxY2wV56aP6V 0neuiTufanJSJgi8P8V8b2gg1Rn0Jln4FQP4imxBvqBJA/AbAkQIWHXbhuhD0E9K1r86xHS9Rt8 dHOB5i5oDLD2MZkjZ8zhQSotnUBUUlb72ktAeNife6bljapUD0oH2Qo/5EEUqPjO18KfLpyiCz2 JZcT4uv0Pcpkt6nbeLkJj/A4noyCrpGKJncQZWdeozQ0PcuwBxG0hncUT/NCrteQSvF9+cBB3xU 9taFGkrs3EnDuiSSAb X-Google-Smtp-Source: AGHT+IFfQz7Ek6tvG3wigeBakizxwVFmYJ3H2HNKhxbtlVrj11hKdPLnYxrjN2OWgodPVTXUuCGF5w== X-Received: by 2002:a05:7300:e607:b0:2a4:3594:72da with SMTP id 5a478bee46e88-2a4aba9f1a8mr6635089eec.9.1763437447193; Mon, 17 Nov 2025 19:44:07 -0800 (PST) Received: from medusa.lab.kspace.sh ([2601:640:8202:6fb0::f013]) by smtp.googlemail.com with UTF8SMTPSA id 5a478bee46e88-2a49d9ead79sm69675621eec.1.2025.11.17.19.44.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Nov 2025 19:44:06 -0800 (PST) Date: Mon, 17 Nov 2025 19:44:05 -0800 From: Mohamed Khalfella To: Ming Lei Cc: Jens Axboe , Keith Busch , Sagi Grimberg , Chaitanya Kulkarni , Casey Chen , Vikas Manocha , Yuanyuan Zhong , Hannes Reinecke , linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/1] nvme: Convert tag_list mutex to rwsemaphore to avoid deadlock Message-ID: <20251118034405.GB2376676-mkhalfella@purestorage.com> References: <20251117202414.4071380-1-mkhalfella@purestorage.com> <20251117202414.4071380-2-mkhalfella@purestorage.com> <20251118021504.GC2197103-mkhalfella@purestorage.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Tue 2025-11-18 10:30:52 +0800, Ming Lei wrote: > On Mon, Nov 17, 2025 at 06:15:04PM -0800, Mohamed Khalfella wrote: > > On Tue 2025-11-18 10:00:19 +0800, Ming Lei wrote: > > > On Mon, Nov 17, 2025 at 12:23:53PM -0800, Mohamed Khalfella wrote: > > > > static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set, > > > > struct request_queue *q) > > > > { > > > > - mutex_lock(&set->tag_list_lock); > > > > + struct request_queue *firstq; > > > > + unsigned int memflags; > > > > > > > > - /* > > > > - * Check to see if we're transitioning to shared (from 1 to 2 queues). > > > > - */ > > > > - if (!list_empty(&set->tag_list) && > > > > - !(set->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) { > > > > - set->flags |= BLK_MQ_F_TAG_QUEUE_SHARED; > > > > - /* update existing queue */ > > > > - blk_mq_update_tag_set_shared(set, true); > > > > - } > > > > - if (set->flags & BLK_MQ_F_TAG_QUEUE_SHARED) > > > > - queue_set_hctx_shared(q, true); > > > > - list_add_tail(&q->tag_set_list, &set->tag_list); > > > > + down_write(&set->tag_list_rwsem); > > > > + if (!list_is_singular(&set->tag_list)) { > > > > + if (set->flags & BLK_MQ_F_TAG_QUEUE_SHARED) > > > > + queue_set_hctx_shared(q, true); > > > > + list_add_tail(&q->tag_set_list, &set->tag_list); > > > > + up_write(&set->tag_list_rwsem); > > > > + return; > > > > + } > > > > > > > > - mutex_unlock(&set->tag_list_lock); > > > > + /* Transitioning firstq and q to shared. */ > > > > + set->flags |= BLK_MQ_F_TAG_QUEUE_SHARED; > > > > + list_add_tail(&q->tag_set_list, &set->tag_list); > > > > + downgrade_write(&set->tag_list_rwsem); > > > > + queue_set_hctx_shared(q, true); > > > > > > queue_set_hctx_shared(q, true) should be moved into write critical area > > > because this queue has been added to the list. > > > > > > > I failed to see why that is the case. What can go wrong by running > > queue_set_hctx_shared(q, true) after downgrade_write()? > > > > After the semaphore is downgraded we promise not to change the list > > set->tag_list because now we have read-only access. Marking the "q" as > > shared should be fine because it is new and we know there will be no > > users of the queue yet (that is why we skipped freezing it). > > I think it is read/write lock's use practice. The protected data shouldn't be > written any more when you downgrade to read lock. > > In this case, it may not make a difference, because it is one new queue and > the other readers don't use the `shared` flag, but still better to do > correct things from beginning and make code less fragile. > set->tag_list_rwsem protects set->tag_list. It does not protect hctx->flags. hctx->flags is protected by the context. In the case of "q" it is new and we are not expecting request allocation. In case of "firstq" the queue is frozen which makes it safe to update hctx->flags. I prefer to keep the code as it is unless there is a reason to change it.