Date: Tue, 4 Apr 2023 07:59:07 -0700 From: Rick Macklem <rick.macklem@gmail.com> To: Cy Schubert <Cy.Schubert@cschubert.com> Cc: Martin Matuska <mm@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, Pawel Dawidek <pjd@freebsd.org>, mjg@freebsd.org Subject: Re: git: 8ee579abe09e - main - zfs: fall back if block_cloning feature is disabled Message-ID: <CAM5tNy7ZP0pYtBqeqEwbiirDg3NwFKHb7oi0WsJ85ep%2B5TZbZQ@mail.gmail.com> In-Reply-To: <20230404141717.B976D31C@slippy.cwsent.com> References: <202304041145.334Bjx6l035872@gitrepo.freebsd.org> <20230404141717.B976D31C@slippy.cwsent.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--00000000000066693605f883eb13 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Apr 4, 2023 at 7:17=E2=80=AFAM Cy Schubert <Cy.Schubert@cschubert.c= om> 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 > > > 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=3D8ee579abe09ec1fe15c588fc= 9a08370b > > 83b81cd6 > > > > commit 8ee579abe09ec1fe15c588fc9a08370b83b81cd6 > > Author: Martin Matuska <mm@FreeBSD.org> > > AuthorDate: 2023-04-04 11:40:41 +0000 > > Commit: Martin Matuska <mm@FreeBSD.org> > > 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 <mjguzik@gmail.com> > > 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 =3D *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 th= en we > > - * need something else than vn_generic_copy_file_range(). > > - */ > > - > > /* Lock both vnodes, avoiding risk of deadlock. */ > > do { > > mp =3D NULL; > > @@ -6300,6 +6293,16 @@ unlock: > > if (mp !=3D NULL) > > vn_finished_write(mp); > > > > + /* > > + * Fall back if block_cloning feature is disabled > > + * or other EXDEV failures from zfs_vnops.c > > + */ > > + if (error =3D=3D EXDEV) { > > + error =3D vn_generic_copy_file_range(ap->a_invp, ap->a_in= offp, > > + 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 t= he > determination at > zfs_freebsd_copy_file_range() entry much earlier. > > 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 =3D *ap->a_lenp; > > + znode_t *outzp =3D VTOZ(ap->a_outvp); > + zfsvfs_t *outzfsvfs =3D ZTOZSB(outzp); > + objset_t *outos =3D outzfsvfs->z_os; > + > + if (!spa_feature_is_enabled(dmu_objset_spa(outos), > + SPA_FEATURE_BLOCK_CLONING)) { > + error =3D vn_generic_copy_file_range(ap->a_invp, ap->a_in= offp, > + ap->a_outvp, ap->a_outoffp, ap->a_lenp, ap->a_fla= gs, > + ap->a_incred, ap->a_outcred, ap->a_fsizetd); > + return (error); > + } > + Can this test be done safely when the vnode is not locked? (I don't know if ZFS ever does "forced dismount", but an unlocked vnode could be doomed if it does?) If so, adding this patch would be nice. I'd leave Martin's patch in the code so that other EXDEV failures get handled. If you cannot do the above check with an unlocked vnode, the attached patch at least avoids some of the locking.(Not yet even compile tested.) rick > /* > * 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 <Cy.Schubert@cschubert.com> > FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org > NTP: <cy@nwtime.org> Web: https://nwtime.org > > e^(i*pi)+1=3D0 > > > > --00000000000066693605f883eb13 Content-Type: application/octet-stream; name="zfscopynew.patch" Content-Disposition: attachment; filename="zfscopynew.patch" Content-Transfer-Encoding: base64 Content-ID: <f_lg2dydw30> X-Attachment-Id: f_lg2dydw30 LS0tIHN5cy9jb250cmliL29wZW56ZnMvbW9kdWxlL29zL2ZyZWVic2QvemZzL3pmc192bm9wc19v cy5jCTIwMjMtMDQtMDQgMDc6MDk6MTYuNDA0NDk5MDAwIC0wNzAwCisrKyBzeXMvY29udHJpYi9v cGVuemZzL21vZHVsZS9vcy9mcmVlYnNkL3pmcy96ZnNfdm5vcHNfb3MuYy5uZXcJMjAyMy0wNC0w NCAwNzo1NjowNS4yMjgyODYwMDAgLTA3MDAKQEAgLTYyNDIsNyArNjI0MiwzMSBAQCB6ZnNfZnJl ZWJzZF9jb3B5X2ZpbGVfcmFuZ2Uoc3RydWN0IHZvcF9jb3B5X2ZpbGVfcmFuZ2UKIAlzdHJ1Y3Qg dWlvIGlvOwogCWludCBlcnJvcjsKIAl1aW50NjRfdCBsZW4gPSAqYXAtPmFfbGVucDsKKwl6ZnN2 ZnNfdCAqb3V0emZzdmZzOworCW9qc2V0X3QgKm91dG9zOworCWJvb2wgZG9uZV9vdXR2cDsKIAor CW1wID0gTlVMTDsKKwllcnJvciA9IHZuX3N0YXJ0X3dyaXRlKG91dHZwLCAmbXAsIFZfV0FJVCk7 CisJaWYgKGVycm9yID09IDApCisJCWVycm9yID0gdm5fbG9jayhvdXR2cCwgTEtfRVhDTFVTSVZF KTsKKwlkb25lX291dHZwID0gdHJ1ZTsKKwlpZiAoZXJyb3IgPT0gMCkgeworCQlvdXR6ZnN2ZnMg PSBaVE9aU0IoVlRPWihvdXR2cCkpOworCQlvdXRvcyA9IG91dHpmc3Zmcy0+el9vczsKKwkJaWYg KCFzcGFfZmVhdHVyZV9pc19lbmFibGVkKGRtdV9vanNldF9zcGEob3V0b3MpLAorCQkgICAgU1BB X0ZFQVRVUkVfQkxPQ0tfQ0xPTklORykpIHsKKwkJCVZPUF9VTkxPQ0sob3V0dnApOworCQkJaWYg KG1wICE9IE5VTEwpCisJCQkJdm5fZmluaXNoZWRfd3JpdGUobXApOworCQkJZXJyb3IgPSB2bl9n ZW5lcmljX2NvcHlfZmlsZV9yYW5nZShhcC0+YV9pbnZwLAorCQkJICAgIGFwLT5hX2lub2ZmcCwg YXAtPmFfb3V0dnAsIGFwLT5hX291dG9mZnAsCisJCQkgICAgYXAtPmFfbGVucCwgYXAtPmFfZmxh Z3MsIGFwLT5hX2luY3JlZCwKKwkJCSAgICBhcC0+YV9vdXRjcmVkLCBhcC0+YV9mc2l6ZXRkKTsK KwkJCXJldHVybiAoZXJyb3IpOworCQl9CisJfQorCiAJLyoKIAkgKiBUT0RPOiBJZiBvZmZzZXQv bGVuZ3RoIGlzIG5vdCBhbGlnbmVkIHRvIHJlY29yZHNpemUsIHVzZQogCSAqIHZuX2dlbmVyaWNf Y29weV9maWxlX3JhbmdlKCkgb24gdGhpcyBmcmFnbWVudC4KQEAgLTYyNTIsMjcgKzYyNzYsMjkg QEAgemZzX2ZyZWVic2RfY29weV9maWxlX3JhbmdlKHN0cnVjdCB2b3BfY29weV9maWxlX3Jhbmdl CiAKIAkvKiBMb2NrIGJvdGggdm5vZGVzLCBhdm9pZGluZyByaXNrIG9mIGRlYWRsb2NrLiAqLwog CWRvIHsKLQkJbXAgPSBOVUxMOwotCQllcnJvciA9IHZuX3N0YXJ0X3dyaXRlKG91dHZwLCAmbXAs IFZfV0FJVCk7CisJCWlmICghZG9uZV9vdXR2cCkgeworCQkJbXAgPSBOVUxMOworCQkJZXJyb3Ig PSB2bl9zdGFydF93cml0ZShvdXR2cCwgJm1wLCBWX1dBSVQpOworCQkJaWYgKGVycm9yID09IDAp CisJCQkJZXJyb3IgPSB2bl9sb2NrKG91dHZwLCBMS19FWENMVVNJVkUpOworCQl9CiAJCWlmIChl cnJvciA9PSAwKSB7Ci0JCQllcnJvciA9IHZuX2xvY2sob3V0dnAsIExLX0VYQ0xVU0lWRSk7Ci0J CQlpZiAoZXJyb3IgPT0gMCkgewotCQkJCWlmIChpbnZwID09IG91dHZwKQotCQkJCQlicmVhazsK LQkJCQllcnJvciA9IHZuX2xvY2soaW52cCwgTEtfU0hBUkVEIHwgTEtfTk9XQUlUKTsKLQkJCQlp ZiAoZXJyb3IgPT0gMCkKLQkJCQkJYnJlYWs7Ci0JCQkJVk9QX1VOTE9DSyhvdXR2cCk7Ci0JCQkJ aWYgKG1wICE9IE5VTEwpCi0JCQkJCXZuX2ZpbmlzaGVkX3dyaXRlKG1wKTsKLQkJCQltcCA9IE5V TEw7Ci0JCQkJZXJyb3IgPSB2bl9sb2NrKGludnAsIExLX1NIQVJFRCk7Ci0JCQkJaWYgKGVycm9y ID09IDApCi0JCQkJCVZPUF9VTkxPQ0soaW52cCk7Ci0JCQl9CisJCQlpZiAoaW52cCA9PSBvdXR2 cCkKKwkJCQlicmVhazsKKwkJCWVycm9yID0gdm5fbG9jayhpbnZwLCBMS19TSEFSRUQgfCBMS19O T1dBSVQpOworCQkJaWYgKGVycm9yID09IDApCisJCQkJYnJlYWs7CisJCQlWT1BfVU5MT0NLKG91 dHZwKTsKKwkJCWlmIChtcCAhPSBOVUxMKQorCQkJCXZuX2ZpbmlzaGVkX3dyaXRlKG1wKTsKKwkJ CW1wID0gTlVMTDsKKwkJCWVycm9yID0gdm5fbG9jayhpbnZwLCBMS19TSEFSRUQpOworCQkJaWYg KGVycm9yID09IDApCisJCQkJVk9QX1VOTE9DSyhpbnZwKTsKIAkJfQogCQlpZiAobXAgIT0gTlVM TCkKIAkJCXZuX2ZpbmlzaGVkX3dyaXRlKG1wKTsKKwkJZG9uZV9vdXR2cCA9IGZhbHNlOwogCX0g d2hpbGUgKGVycm9yID09IDApOwogCWlmIChlcnJvciAhPSAwKQogCQlyZXR1cm4gKGVycm9yKTsK --00000000000066693605f883eb13--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAM5tNy7ZP0pYtBqeqEwbiirDg3NwFKHb7oi0WsJ85ep%2B5TZbZQ>