From nobody Tue Apr 4 14:37:56 2023 X-Original-To: dev-commits-src-main@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 4PrVjg5Mjsz43g3B; Tue, 4 Apr 2023 14:37:59 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-ot1-x329.google.com (mail-ot1-x329.google.com [IPv6:2607:f8b0:4864:20::329]) (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 4PrVjg3NWsz3MNt; Tue, 4 Apr 2023 14:37:59 +0000 (UTC) (envelope-from mjguzik@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ot1-x329.google.com with SMTP id f19-20020a9d5f13000000b00693ce5a2f3eso17426291oti.8; Tue, 04 Apr 2023 07:37:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1680619077; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=vFWTgV2WJGf0DWw3CgdxZh/NdVjpZtHOZy/x6TWHbCk=; b=N4lQhhJbT/QwsiP7975UPoLMNipRiWos90WZm+Bwf9beCu1HN9sfYLXnMHpsCHjIUJ TQwUveWlQ23ky7iSvSBbWMr3Rd0B2aRyKEbBzgrlqO1ci0kdu/X3YCSIlDSxj+LkGKy9 6vetZTv7bqfvSwPITOisHQl+ZAeFz3WH0a9Jwt6S9z0n+1CFu9h34+tsoqfRNNsBQcmO 1DhuJGIrxRWo2qNLph1CplrvsPQre7/rGjCR8zv5GslCGs3UypuXIINDOTfOSLXpUd/K oK2hzotWrkEqDzXG8989FjuhZG7tmKFjluw0sH+c/wOPgbLPSdWn9Gmg5NP5vHELsx44 AM0Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680619077; 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=vFWTgV2WJGf0DWw3CgdxZh/NdVjpZtHOZy/x6TWHbCk=; b=Bi7mFW43FGzIYT0/N2ffFCxPN9ah5QUOrvHI5/E+K1grV4fZShiy4HGhtY86liOlX7 NEzymhFsQikkfqSCZ862/mpmpxyRVKCRwRFrCLZf5FRSE6ukNfBHCc6gajZULLgOfzRc dHHdByrBW7GnvjzRu4BLvDYbkhaRlgztqbW8o7HmOEgd5iQiq37LlBWe0M0mgLpALZzy yKn8rchGW+c9bDRxi/ZhRvlKjbMSH2iAWKoe6t1DLOCnwvaxgSIiVIimFt7m5ThuOQrw ejY8y6ovsGY9nJZn90a2B+5iQFJ+U61DV/OS3pviyxkO06fIi5Np4tj6TvMCo4iT6JRt wjjQ== X-Gm-Message-State: AAQBX9fUQZZ/EHZEze7L263HypcyjnW8hIsDIyrvc4T58sTnUvVAEapG ZWtq50E5CSjK+Z+e1+LJQ+2w4CK2+RygaoU8Tqk2HTFV X-Google-Smtp-Source: AKy350aU5o3EETFFohPK0RSytlXD9kgjPIF8ki55XmwpN49w57fDuPJk34jl6ZTkeRXXVmFSutbjYoXWPoxd0cuEMK4= X-Received: by 2002:a9d:6ac9:0:b0:698:f988:7c30 with SMTP id m9-20020a9d6ac9000000b00698f9887c30mr886357otq.2.1680619077385; Tue, 04 Apr 2023 07:37:57 -0700 (PDT) List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Received: by 2002:ac9:570a:0:b0:49c:b071:b1e3 with HTTP; Tue, 4 Apr 2023 07:37:56 -0700 (PDT) In-Reply-To: <20230404141717.B976D31C@slippy.cwsent.com> References: <202304041145.334Bjx6l035872@gitrepo.freebsd.org> <20230404141717.B976D31C@slippy.cwsent.com> From: Mateusz Guzik Date: Tue, 4 Apr 2023 16:37:56 +0200 Message-ID: Subject: Re: git: 8ee579abe09e - main - zfs: fall back if block_cloning feature is disabled To: Cy Schubert Cc: Martin Matuska , 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: 4PrVjg3NWsz3MNt X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N On 4/4/23, Cy Schubert wrote: > In message <202304041145.334Bjx6l035872@gitrepo.freebsd.org>, Martin > Matuska wr > ites: >> The branch main has been updated by mm: >> >> URL: >> https://cgit.FreeBSD.org/src/commit/?id=8ee579abe09ec1fe15c588fc9a08370b >> 83b81cd6 >> >> commit 8ee579abe09ec1fe15c588fc9a08370b83b81cd6 >> Author: Martin Matuska >> AuthorDate: 2023-04-04 11:40:41 +0000 >> Commit: Martin Matuska >> CommitDate: 2023-04-04 11:43:34 +0000 >> >> zfs: fall back if block_cloning feature is disabled >> >> If block_cloning is disabled, or other errors from zfs_clone_range() >> return an EXDEV we should fall back to vn_generic_copy_file_range(). >> >> This fixes issues when copying files on the same dataset with >> block_cloning disabled. >> >> Upstreamed as pull request to OpenZFS. >> >> Reviewed by: Mateusz Guzik >> OpenZFS pull request: 14713 >> --- >> .../openzfs/module/os/freebsd/zfs/zfs_vnops_os.c | 17 >> ++++++++++----- >> -- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c >> b/sys/c >> ontrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c >> index 97429b360a36..2cd1d27e37bc 100644 >> --- a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c >> +++ b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c >> @@ -6243,13 +6243,6 @@ zfs_freebsd_copy_file_range(struct >> vop_copy_file_range >> _args *ap) >> int error; >> uint64_t len = *ap->a_lenp; >> >> - /* >> - * TODO: If offset/length is not aligned to recordsize, use >> - * vn_generic_copy_file_range() on this fragment. >> - * It would be better to do this after we lock the vnodes, but then we >> - * need something else than vn_generic_copy_file_range(). >> - */ >> - >> /* Lock both vnodes, avoiding risk of deadlock. */ >> do { >> mp = NULL; >> @@ -6300,6 +6293,16 @@ unlock: >> if (mp != NULL) >> vn_finished_write(mp); >> >> + /* >> + * Fall back if block_cloning feature is disabled >> + * or other EXDEV failures from zfs_vnops.c >> + */ >> + if (error == EXDEV) { >> + error = vn_generic_copy_file_range(ap->a_invp, ap->a_inoffp, >> + ap->a_outvp, ap->a_outoffp, ap->a_lenp, ap->a_flags >> , >> + ap->a_incred, ap->a_outcred, ap->a_fsizetd); >> + } >> + >> return (error); >> } >> >> > > This is too late to fall back. On Rick's suggestion the following makes the > > determination at > zfs_freebsd_copy_file_range() entry much earlier. > It's not too late, but I agree it is faster to bail out early. The proposed patch adds a condition which *differs* from the one in zfs_clone_range: if (dmu_objset_spa(inos) != dmu_objset_spa(outos)) { zfs_exit_two(inzfsvfs, outzfsvfs, FTAG); return (SET_ERROR(EXDEV)); } ... meaning with the proposed patch the routine can still fail with EXDEV, making zfs_freebsd_copy_file_range also do it, which must not happen. That aside the code looks rather suspicious for the case where target and source vnode are the same. iow more work is needed here. As the vnode is unlocked, you *can't* safely access zfsvfs_t *outzfsvfs = ZTOZSB(outzp); in that spot in this manner -- a forced unmount at the same time can free it. iow this patch does *NOT* work. With the committed variant the situation is damage controlled enough that there is time to sort it out correctly. > diff --git a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c > b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c > index d41821ff67f1..e18dcca58192 100644 > --- a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c > +++ b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c > @@ -6243,6 +6243,18 @@ zfs_freebsd_copy_file_range(struct > vop_copy_file_range_args *ap) > int error; > uint64_t len = *ap->a_lenp; > > + znode_t *outzp = VTOZ(ap->a_outvp); > + zfsvfs_t *outzfsvfs = ZTOZSB(outzp); > + objset_t *outos = outzfsvfs->z_os; > + > + if (!spa_feature_is_enabled(dmu_objset_spa(outos), > + SPA_FEATURE_BLOCK_CLONING)) { > + error = vn_generic_copy_file_range(ap->a_invp, ap->a_inoffp, > + ap->a_outvp, ap->a_outoffp, ap->a_lenp, ap->a_flags, > + ap->a_incred, ap->a_outcred, ap->a_fsizetd); > + return (error); > + } > + > /* > * TODO: If offset/length is not aligned to recordsize, use > * vn_generic_copy_file_range() on this fragment. > > > Can you revert your commit and commit this, please. > > > -- > Cheers, > Cy Schubert > FreeBSD UNIX: Web: https://FreeBSD.org > NTP: Web: https://nwtime.org > > e^(i*pi)+1=0 > > > > -- Mateusz Guzik