Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Github version: https://github.com/mmatuska/freebsd-src/commits/openzfs_gc1c31a835
@mjg please review the modification of zfs_freebsd_readlink() in sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
Huh. Looks like upstream added the following everywhere:
static int zfs_freebsd_read(struct vop_read_args *ap) { zfs_uio_t uio; zfs_uio_init(&uio, ap->a_uio); return (zfs_read(VTOZ(ap->a_vp), &uio, ioflags(ap->a_ioflag), ap->a_cred)); }
Not only this adds a likely avoidable data copy, this can no longer be tail called as return has to clean up the stack. That's a major bummer and should get reimplemented. I don't have time to get into it now.
That said, the readlink bit looks fine.
I don't think it's feasible to really review the entire patchset tough.
@mjg I don't think much has changed under the hood because:
typedef struct zfs_uio { struct uio *uio; } zfs_uio_t; #define GET_UIO_STRUCT(u) (u)->uio #define zfs_uio_segflg(u) GET_UIO_STRUCT(u)->uio_segflg #define zfs_uio_offset(u) GET_UIO_STRUCT(u)->uio_offset #define zfs_uio_resid(u) GET_UIO_STRUCT(u)->uio_resid #define zfs_uio_iovcnt(u) GET_UIO_STRUCT(u)->uio_iovcnt #define zfs_uio_iovlen(u, idx) GET_UIO_STRUCT(u)->uio_iov[(idx)].iov_len #define zfs_uio_iovbase(u, idx) GET_UIO_STRUCT(u)->uio_iov[(idx)].iov_base #define zfs_uio_td(u) GET_UIO_STRUCT(u)->uio_td #define zfs_uio_rw(u) GET_UIO_STRUCT(u)->uio_rw #define zfs_uio_fault_disable(u, set) #define zfs_uio_prefaultpages(size, u) (0) static __inline void zfs_uio_init(zfs_uio_t *uio, struct uio *uio_s) { GET_UIO_STRUCT(uio) = uio_s; }
But if it is like you say, then we do symlink_len -= zfs_uio_resid(&uio); after line error = zfs_readlink(ap->a_vp, &uio, ap->a_cred, NULL); and that would be a no-go in zfs_freebsd_readlink().
The zfs test suite was not very happy with this in my bhyve, I guess we postpone this or someone else could continue with the merge. I have cherry-picked two important commits for 13.0-RELEASE.
Is the test suite happy with zfs as is? I know it used to pass few months back, but to my understanding some of the assertions were not being compiled in.
I ran the few zfs tests I have on openzfs_gc1c31a835-n244815-1f4c076e9e38 without observing any problems.
To be true guys, what is much more interesting than this diff is the diff between vendor/openzfs and sys/contrib/openzfs - I guess we want to keep it as small as possible (long-term). And currently there is already some difference.
So if you have no objections I might throw this in but I am not really into having this in 13.0-RELEASE, 13-stable after a reasonable amount of time (at least 2 weeks) would be o.k. for me.
The most notable improvement for me is the speedup in zpool import. I have a 102-drive pool with 60TB of L2 cache and the import time is reduced by a factor of 10.
The intent is to have next to no local patches, but upstreaming got stalled at some point with CI failures and I did not pick it up later. It is a little iffy right now but should be sorted out soon(tm).
Sorry I didn't get a chance to review this in time. Thank you for updating to 436ab35a5. I share the concern about our diff against upstream.