From nobody Tue Apr 4 20:18:25 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 4PrfmR458Xz448df; Tue, 4 Apr 2023 20:40: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 4PrfmR2TXZz4QnF; Tue, 4 Apr 2023 20:40:55 +0000 (UTC) (envelope-from mm@FreeBSD.org) Authentication-Results: mx1.freebsd.org; none Received: from sslproxy02.your-server.de ([78.47.166.47]) by www541.your-server.de with esmtpsa (TLS1.3) tls TLS_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1pjn6p-000DJN-1R; Tue, 04 Apr 2023 22:18:27 +0200 Received: from [188.167.171.2] (helo=[10.0.9.128]) by sslproxy02.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pjn6o-000Sd6-8L; Tue, 04 Apr 2023 22:18:26 +0200 Message-ID: <88b2e6c2-94d7-81f8-f85d-9d7a6b7b8d11@FreeBSD.org> Date: Tue, 4 Apr 2023 22:18:25 +0200 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 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: Cy Schubert , Mateusz Guzik Cc: Rick Macklem , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, Pawel Dawidek References: <202304041145.334Bjx6l035872@gitrepo.freebsd.org> <20230404141717.B976D31C@slippy.cwsent.com> <98c71e6f-5b48-79f3-e7b0-95d674949624@FreeBSD.org> <20230404091844.639cb1c1@slippy> <20230404093418.3041ff23@slippy> <20230404181823.0EA79C4@slippy.cwsent.com> From: Martin Matuska In-Reply-To: <20230404181823.0EA79C4@slippy.cwsent.com> 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-Rspamd-Queue-Id: 4PrfmR2TXZz4QnF X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; TAGGED_RCPT(0.00)[]; ASN(0.00)[asn:24940, ipnet:213.133.96.0/19, country:DE] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N I agree that these are three individual fixes. 1.) pass ap->a_outcred instead of ap->a_fsizetd->td_ucred to zfs_clone_range() I am ok with this, the way the argument is subsequently used it should be ap->a_outcred which is intended for the write. 2.) do a vn_generic_copy_file_range() in case of EXDEV The comment vn_generic_copy_file_range() says: /*  * Copy a byte range of one file to another.  This function can handle the  * case where invp and outvp are on different file systems.  * It can also be called by a VOP_COPY_FILE_RANGE() to do the work, if there  * is no better file system specific way to do it.  */ That is actually our case. zfs_clone_range() exits with EXDEV if: - source and destination are not on the same pool - the block_cloning feature is not enabled - input and output files have a different block size - offset and len are not at block boundaries - length is not a multiple of block size, with except for the end of file - we are trying to clone a block created in the current transaction group - we are cloning encrypted data not in the same dataset IMO we can fallback to vn_generic_copy_file_range() in all of these cases. As of the locks, we need to run vn_generic_copy_file_range() on unlocked vnodes, just look into the function. In both fuse_vnops.c and nfs_clvnops.c it does not run on locked vnodes. Even the comment from pjd in zfs_vnops_os.c says:         /*          * 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().          */ So IMO it should be at the end after unlock. 3.) By doing the feature check early, we save locking the input vnode and calling mac_vnode_check_write() and vn_rlimit_fsize() at the cost of checking for the disabled feature twice. Maybe documented skipping of the check in zfs_clone_range()? The code of the early check looks ok to me. On 4. 4. 2023 20:18, Cy Schubert wrote: > In message om> > , Mateusz Guzik writes: >> can you please post a review > I could but I didn't write any of it. Rick Macklem and Martin Matuska wrote > it. My patch was for discussion only. > > Martin and Rick, do you mind if I post this as a review. It should probably > be two, maybe three separate commits, fixing two different problems. > >