From nobody Thu Aug 10 14:23:50 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4RM8LJ6Q1Fz4mLKY; Thu, 10 Aug 2023 14:23:52 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-oo1-xc2a.google.com (mail-oo1-xc2a.google.com [IPv6:2607:f8b0:4864:20::c2a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4RM8LJ4hdPz3XBb; Thu, 10 Aug 2023 14:23:52 +0000 (UTC) (envelope-from mjguzik@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-oo1-xc2a.google.com with SMTP id 006d021491bc7-56c85b723cfso778138eaf.3; Thu, 10 Aug 2023 07:23:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1691677431; x=1692282231; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=pXgdnZkAuyGoW4BeojKBcvs04Kgw/BKvL3EVOBybvJM=; b=S/bRIWzVnwag2Mapm2uCTcj9V1seQw/NetUDuyDFnFddX2y2qExZTv533Xe+5dm85d NR+5OqzAm/jIFQZPK1+KdpcVN1sBYvFeju1glU8BV6St6v0xXQct8keOksQTs7EGvr5G N8Y37RY5+RMuBhdKj5iuCdRm2gRDPCh5h/J+EeXAY/GT4Z35ayo3zMo65PE7wXOJgKVF ShVqsE9XDzGBDM+NwxQRy++r9e6137y4zZ3uQiFsPqlIwcNRP3fc/cLYbsztwStpV857 FDfQxDv+gPV/saZYIhG1uV6BkNOfUirWE+Tn4fou3asHJPHUDYnBDf6BMnSHes2UL+vB I+Xw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691677431; x=1692282231; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=pXgdnZkAuyGoW4BeojKBcvs04Kgw/BKvL3EVOBybvJM=; b=Eu/LUBiW2fpK2mcNtlI5BsWQ0unAMm+AlZjseGuQywiJt9T6XQm6xuBKFaR02ubZ3Y kW2t4Lwrm60bBJHD96gAcFHG7KusXVz6J97hWeQ6h56WL0TisOGOYNfKz/knwuJRiFVL i2Sb9IL4LDI6Xw5vxi+HIOEeWKXQYkt1rOdzi6sMhIr8+bYNjgR0fuqpa5rNXd81qrzc vTevy4uLwcC1Sb/Wiw8RmIVRGtF3phowFrpM9rqzC5LJp3Kx1JRQWo51cV6hGrotAaFe 2qQxgN2wf3hQy+nyrp3j1gLGybalS+hb3GuTKz2o08JAof00HOAKTZM8cTR0zav1TE1q dYog== X-Gm-Message-State: AOJu0YzbHNVPz1T/7iGFTlVMb29LFfYoUTPMeID2D1yehgltoasIXz3t f01RTD3jxqcp0bjRFQcyqMScElYcZJ7aujvhNDDguB/8bVo= X-Google-Smtp-Source: AGHT+IHcjwnU5s1jGjGH7QLgE68Fo5hIouq2fREKcSeoe55jIDL+2RUq9vnJlshFJBuqIi73RNHt8aGrl1nO8nzA4uQ= X-Received: by 2002:a4a:2a4c:0:b0:56c:7120:8363 with SMTP id x12-20020a4a2a4c000000b0056c71208363mr2114462oox.5.1691677431045; Thu, 10 Aug 2023 07:23:51 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Received: by 2002:a8a:129a:0:b0:4f0:1250:dd51 with HTTP; Thu, 10 Aug 2023 07:23:50 -0700 (PDT) In-Reply-To: <202308100835.37A8ZFgc042543@gitrepo.freebsd.org> References: <202308100835.37A8ZFgc042543@gitrepo.freebsd.org> From: Mateusz Guzik Date: Thu, 10 Aug 2023 16:23:50 +0200 Message-ID: Subject: Re: git: cd25b0f740f8 - main - zfs: cherry-pick fix from openzfs To: Martin Matuska Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4RM8LJ4hdPz3XBb X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] On 8/10/23, Martin Matuska wrote: > The branch main has been updated by mm: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=cd25b0f740f8c846562fd66e7380437eb898c875 > > commit cd25b0f740f8c846562fd66e7380437eb898c875 > Author: Martin Matuska > AuthorDate: 2023-08-10 07:55:42 +0000 > Commit: Martin Matuska > CommitDate: 2023-08-10 07:56:53 +0000 > > zfs: cherry-pick fix from openzfs > > Vendor PR: > #15080 ZIL: Fix config lock deadlock > > Obtained from: OpenZFS > OpenZFS commit: 2cb992a99ccadb78d97049b40bd442eb4fdc549d > > Note: full vendor imports will continue when stable/14 has been > branched First a stylistic issue: Can you please use upstream one-liner, maybe prefixed with zfs:. For this commit it would be: zfs: ZIL: Fix config lock deadlock. then copy their commit message For example see https://cgit.freebsd.org/src/commit/?id=d09a955a605d03471c5ab7bd17b8a6186fdc148c A not stylistic issue is the delay between upstream fixes and them getting merged into FreeBSD. For example the commit at hand is over 2 weeks old and I presume this merge got only prompted by des@ running a zfs-related deadlock. We really should be getting timely updates without local people running into them. > --- > sys/contrib/openzfs/module/zfs/zil.c | 34 > +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/sys/contrib/openzfs/module/zfs/zil.c > b/sys/contrib/openzfs/module/zfs/zil.c > index 00d66a2481d7..af7137faaccf 100644 > --- a/sys/contrib/openzfs/module/zfs/zil.c > +++ b/sys/contrib/openzfs/module/zfs/zil.c > @@ -151,6 +151,7 @@ static kmem_cache_t *zil_lwb_cache; > static kmem_cache_t *zil_zcw_cache; > > static void zil_lwb_commit(zilog_t *zilog, lwb_t *lwb, itx_t *itx); > +static void zil_lwb_write_issue(zilog_t *zilog, lwb_t *lwb); > static itx_t *zil_itx_clone(itx_t *oitx); > > static int > @@ -1768,7 +1769,7 @@ static uint_t zil_maxblocksize = > SPA_OLD_MAXBLOCKSIZE; > * Has to be called under zl_issuer_lock to chain more lwbs. > */ > static lwb_t * > -zil_lwb_write_close(zilog_t *zilog, lwb_t *lwb) > +zil_lwb_write_close(zilog_t *zilog, lwb_t *lwb, list_t *ilwbs) > { > lwb_t *nlwb = NULL; > zil_chain_t *zilc; > @@ -1870,6 +1871,27 @@ zil_lwb_write_close(zilog_t *zilog, lwb_t *lwb) > > dmu_tx_commit(tx); > > + /* > + * We need to acquire the config lock for the lwb to issue it later. > + * However, if we already have a queue of closed parent lwbs already > + * holding the config lock (but not yet issued), we can't block here > + * waiting on the lock or we will deadlock. In that case we must > + * first issue to parent IOs before waiting on the lock. > + */ > + if (ilwbs && !list_is_empty(ilwbs)) { > + if (!spa_config_tryenter(spa, SCL_STATE, lwb, RW_READER)) { > + lwb_t *tlwb; > + while ((tlwb = list_remove_head(ilwbs)) != NULL) > + zil_lwb_write_issue(zilog, tlwb); > + spa_config_enter(spa, SCL_STATE, lwb, RW_READER); > + } > + } else { > + spa_config_enter(spa, SCL_STATE, lwb, RW_READER); > + } > + > + if (ilwbs) > + list_insert_tail(ilwbs, lwb); > + > /* > * If there was an allocation failure then nlwb will be null which > * forces a txg_wait_synced(). > @@ -1933,7 +1955,7 @@ zil_lwb_write_issue(zilog_t *zilog, lwb_t *lwb) > ZIL_STAT_INCR(zilog, zil_itx_metaslab_normal_alloc, > BP_GET_LSIZE(&lwb->lwb_blk)); > } > - spa_config_enter(zilog->zl_spa, SCL_STATE, lwb, RW_READER); > + ASSERT(spa_config_held(zilog->zl_spa, SCL_STATE, RW_READER)); > zil_lwb_add_block(lwb, &lwb->lwb_blk); > lwb->lwb_issued_timestamp = gethrtime(); > zio_nowait(lwb->lwb_root_zio); > @@ -2037,8 +2059,7 @@ cont: > lwb_sp < zil_max_waste_space(zilog) && > (dlen % max_log_data == 0 || > lwb_sp < reclen + dlen % max_log_data))) { > - list_insert_tail(ilwbs, lwb); > - lwb = zil_lwb_write_close(zilog, lwb); > + lwb = zil_lwb_write_close(zilog, lwb, ilwbs); > if (lwb == NULL) > return (NULL); > zil_lwb_write_open(zilog, lwb); > @@ -2937,8 +2958,7 @@ zil_process_commit_list(zilog_t *zilog, > zil_commit_waiter_t *zcw, list_t *ilwbs) > zfs_commit_timeout_pct / 100; > if (sleep < zil_min_commit_timeout || > lwb->lwb_sz - lwb->lwb_nused < lwb->lwb_sz / 8) { > - list_insert_tail(ilwbs, lwb); > - lwb = zil_lwb_write_close(zilog, lwb); > + lwb = zil_lwb_write_close(zilog, lwb, ilwbs); > zilog->zl_cur_used = 0; > if (lwb == NULL) { > while ((lwb = list_remove_head(ilwbs)) > @@ -3096,7 +3116,7 @@ zil_commit_waiter_timeout(zilog_t *zilog, > zil_commit_waiter_t *zcw) > * since we've reached the commit waiter's timeout and it still > * hasn't been issued. > */ > - lwb_t *nlwb = zil_lwb_write_close(zilog, lwb); > + lwb_t *nlwb = zil_lwb_write_close(zilog, lwb, NULL); > > ASSERT3S(lwb->lwb_state, !=, LWB_STATE_OPENED); > > -- Mateusz Guzik