From nobody Tue Nov 14 22:06:19 2023 X-Original-To: current@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 4SVL3l4r9gz50yxs for ; Tue, 14 Nov 2023 22:06:27 +0000 (UTC) (envelope-from kib@freebsd.org) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (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 4SVL3k6mw6z3TVC; Tue, 14 Nov 2023 22:06:26 +0000 (UTC) (envelope-from kib@freebsd.org) Authentication-Results: mx1.freebsd.org; dkim=none; spf=softfail (mx1.freebsd.org: 2001:470:d5e7:1::1 is neither permitted nor denied by domain of kib@freebsd.org) smtp.mailfrom=kib@freebsd.org; dmarc=none Received: from tom.home (kib@localhost [127.0.0.1] (may be forged)) by kib.kiev.ua (8.17.1/8.17.1) with ESMTP id 3AEM6JJE043497; Wed, 15 Nov 2023 00:06:22 +0200 (EET) (envelope-from kib@freebsd.org) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 3AEM6JJE043497 Received: (from kostik@localhost) by tom.home (8.17.1/8.17.1/Submit) id 3AEM6JXB043496; Wed, 15 Nov 2023 00:06:19 +0200 (EET) (envelope-from kib@freebsd.org) X-Authentication-Warning: tom.home: kostik set sender to kib@freebsd.org using -f Date: Wed, 15 Nov 2023 00:06:19 +0200 From: Konstantin Belousov To: Rick Macklem Cc: Mateusz Guzik , Alexander Motin , Ronald Klop , current@freebsd.org Subject: Re: crash zfs_clone_range() Message-ID: References: <349700057.3452.1699611152405@localhost> <1900239445.5968.1699966796547@localhost> List-Id: Discussions about the use of FreeBSD-current List-Archive: https://lists.freebsd.org/archives/freebsd-current List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-current@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=4.0.0 X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-14) on tom.home X-Spamd-Result: default: False [-3.10 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-1.000]; MIME_GOOD(-0.10)[text/plain]; FREEMAIL_TO(0.00)[gmail.com]; MLMMJ_DEST(0.00)[current@freebsd.org]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_TLS_LAST(0.00)[]; R_DKIM_NA(0.00)[]; FROM_EQ_ENVFROM(0.00)[]; ASN(0.00)[asn:6939, ipnet:2001:470::/32, country:US]; MIME_TRACE(0.00)[0:+]; R_SPF_SOFTFAIL(0.00)[~all]; RCPT_COUNT_FIVE(0.00)[5]; DMARC_NA(0.00)[freebsd.org]; FREEFALL_USER(0.00)[kib]; ARC_NA(0.00)[]; FREEMAIL_CC(0.00)[gmail.com,freebsd.org,klop.ws]; FROM_HAS_DN(0.00)[]; TAGGED_RCPT(0.00)[]; TO_DN_SOME(0.00)[]; HAS_XAW(0.00)[]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Queue-Id: 4SVL3k6mw6z3TVC X-Spamd-Bar: --- On Tue, Nov 14, 2023 at 01:30:25PM -0800, Rick Macklem wrote: > On Tue, Nov 14, 2023 at 1:20 PM Konstantin Belousov wrote: > > > > On Tue, Nov 14, 2023 at 06:47:46PM +0100, Mateusz Guzik wrote: > > > On 11/14/23, Alexander Motin wrote: > > > > On 14.11.2023 12:39, Mateusz Guzik wrote: > > > >> One of the vnodes is probably not zfs, I suspect this will do it > > > >> (untested): > > > >> > > > >> 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 107cd69c756c..e799a7091b8e 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 > > > >> @@ -6270,6 +6270,11 @@ zfs_freebsd_copy_file_range(struct > > > >> vop_copy_file_range_args *ap) > > > >> goto bad_write_fallback; > > > >> } > > > >> } > > > >> + > > > >> + if (invp->v_mount->mnt_vfc != outvp->v_mount->mnt_vfc) { > > > >> + goto bad_write_fallback; > > > >> + } > > > >> + > > > >> if (invp == outvp) { > > > >> if (vn_lock(outvp, LK_EXCLUSIVE) != 0) { > > > >> goto bad_write_fallback; > > > >> > > > > > > > > vn_copy_file_range() verifies for that: > > > > > > > > /* > > > > * If the two vnodes are for the same file system type, call > > > > * VOP_COPY_FILE_RANGE(), otherwise call > > > > vn_generic_copy_file_range() > > > > * which can handle copies across multiple file system types. > > > > */ > > > > *lenp = len; > > > > if (inmp == outmp || strcmp(inmp->mnt_vfc->vfc_name, > > > > outmp->mnt_vfc->vfc_name) == 0) > > > > error = VOP_COPY_FILE_RANGE(invp, inoffp, outvp, outoffp, > > > > lenp, flags, incred, outcred, fsize_td); > > > > else > > > > error = vn_generic_copy_file_range(invp, inoffp, outvp, > > > > outoffp, lenp, flags, incred, outcred, fsize_td); > > > > > > > > > > > > > > The crash at hand comes from nullfs. If "outward" vnodes are both > > > nullfs, but only one underlying vnode is zfs, you get the above. > > > > If this is the reason, the check must be done by nullfs bypass for > > vop_copy_file_range(). > I suppose this is a reasonable alternative, although it means that > all stacked file systems will need the check. > It just seems easier to do it in the actual VOPs, but it is up to others. In theory, eventually we can have much more implementations for VOP than VOP' callers. I.e. fixing all stacked fs means adding unionfs to my patch. Forcing this requirements on all future VOP_COPY_FILE_RANGE implementations is IMO not good. BTW, we already have zfs, nfs, and fuse implementing the VOP. > > Btw, the stuff above the VOP_COPY_FILE_RANGE() that busies the > mounts and checks mnt_vfc being the same could be dropped, if the > VOP_COPY_FILE_RANGE() calls like NFS were careful to lock the > vnodes before doing a "same fs type or same mount" check. > (I suppose that would be a subtle change in VOP semantics that > is arguably not allowed for a minor version.) > > Anyhow, I am happy with whatever others decide. > > rick > > >