From nobody Tue Apr 4 15:30:25 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 4PrXRb6SRcz43m3f; Tue, 4 Apr 2023 15:55:55 +0000 (UTC) (envelope-from mm@FreeBSD.org) Received: from www541.your-server.de (www541.your-server.de [213.133.107.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4PrXRZ5cS6z3mbq; Tue, 4 Apr 2023 15:55:54 +0000 (UTC) (envelope-from mm@FreeBSD.org) Authentication-Results: mx1.freebsd.org; dkim=none; spf=softfail (mx1.freebsd.org: 213.133.107.7 is neither permitted nor denied by domain of mm@FreeBSD.org) smtp.mailfrom=mm@FreeBSD.org; dmarc=none Received: from sslproxy01.your-server.de ([78.46.139.224]) by www541.your-server.de with esmtpsa (TLS1.3) tls TLS_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1pjic6-0005mK-OK; Tue, 04 Apr 2023 17:30:26 +0200 Received: from [188.167.171.2] (helo=[10.0.9.128]) by sslproxy01.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pjic6-000LMi-7f; Tue, 04 Apr 2023 17:30:26 +0200 Message-ID: <98c71e6f-5b48-79f3-e7b0-95d674949624@FreeBSD.org> Date: Tue, 4 Apr 2023 17:30:25 +0200 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 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: git: 8ee579abe09e - main - zfs: fall back if block_cloning feature is disabled Content-Language: en-US To: Rick Macklem , Mateusz Guzik Cc: Cy Schubert , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org References: <202304041145.334Bjx6l035872@gitrepo.freebsd.org> <20230404141717.B976D31C@slippy.cwsent.com> From: Martin Matuska In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Authenticated-Sender: martin@matuska.de X-Virus-Scanned: Clear (ClamAV 0.103.8/26865/Tue Apr 4 09:24:56 2023) X-Spamd-Result: default: False [-1.65 / 15.00]; SUSPICIOUS_RECIPS(1.50)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.99)[-0.990]; NEURAL_HAM_MEDIUM(-0.96)[-0.963]; RCVD_IN_DNSWL_LOW(-0.10)[78.46.139.224:received]; MIME_GOOD(-0.10)[text/plain]; TO_MATCH_ENVRCPT_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; FREEFALL_USER(0.00)[mm]; DMARC_NA(0.00)[freebsd.org]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; MIME_TRACE(0.00)[0:+]; TAGGED_RCPT(0.00)[]; ARC_NA(0.00)[]; HAS_X_AS(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; RCPT_COUNT_FIVE(0.00)[6]; R_SPF_SOFTFAIL(0.00)[~all]; TO_DN_SOME(0.00)[]; RCVD_TLS_ALL(0.00)[]; RCVD_COUNT_THREE(0.00)[3]; FROM_EQ_ENVFROM(0.00)[]; FREEMAIL_TO(0.00)[gmail.com]; ASN(0.00)[asn:24940, ipnet:213.133.96.0/19, country:DE]; R_DKIM_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[] X-Rspamd-Queue-Id: 4PrXRZ5cS6z3mbq X-Spamd-Bar: - X-ThisMailContainsUnwantedMimeParts: N So I am now a little bit confused - what is the consensus? :-) On 4. 4. 2023 17:26, Rick Macklem wrote: > On Tue, Apr 4, 2023 at 7:38 AM Mateusz Guzik wrote: >> CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to IThelp@uoguelph.ca >> >> >> 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. > Since VOP_COPY_FILE_RANGE() is only called when invp and outvp > are on the same mount point, I don't think this can happen now. > However, there is a TO DO comment that suggests a call with invp and > outvp on different mount points may be in the future. > > As such, leaving Martin's patch in so that it calls vn_generic_copy_file_range() > when zfs_clone_range() returns EXDEV seems like a good idea to me. > >> That aside the code looks rather suspicious for the case where target >> and source vnode are the same. iow more work is needed here. > Definitely needs to be tested. I'll do that later to-day. > > rick > >> 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