Changeset View
Standalone View
sys/kern/vfs_vnops.c
Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines | |||||
#include <sys/kdb.h> | #include <sys/kdb.h> | ||||
#include <sys/ktr.h> | #include <sys/ktr.h> | ||||
#include <sys/stat.h> | #include <sys/stat.h> | ||||
#include <sys/priv.h> | #include <sys/priv.h> | ||||
#include <sys/proc.h> | #include <sys/proc.h> | ||||
#include <sys/limits.h> | #include <sys/limits.h> | ||||
#include <sys/lock.h> | #include <sys/lock.h> | ||||
#include <sys/mman.h> | #include <sys/mman.h> | ||||
#include <sys/malloc.h> | |||||
#include <sys/mount.h> | #include <sys/mount.h> | ||||
#include <sys/mutex.h> | #include <sys/mutex.h> | ||||
#include <sys/namei.h> | #include <sys/namei.h> | ||||
#include <sys/vnode.h> | #include <sys/vnode.h> | ||||
#include <sys/bio.h> | #include <sys/bio.h> | ||||
#include <sys/buf.h> | #include <sys/buf.h> | ||||
#include <sys/filio.h> | #include <sys/filio.h> | ||||
#include <sys/resourcevar.h> | #include <sys/resourcevar.h> | ||||
▲ Show 20 Lines • Show All 2,415 Lines • ▼ Show 20 Lines | |||||
vn_fsid(struct vnode *vp, struct vattr *va) | vn_fsid(struct vnode *vp, struct vattr *va) | ||||
{ | { | ||||
fsid_t *f; | fsid_t *f; | ||||
f = &vp->v_mount->mnt_stat.f_fsid; | f = &vp->v_mount->mnt_stat.f_fsid; | ||||
va->va_fsid = (uint32_t)f->val[1]; | va->va_fsid = (uint32_t)f->val[1]; | ||||
va->va_fsid <<= sizeof(f->val[1]) * NBBY; | va->va_fsid <<= sizeof(f->val[1]) * NBBY; | ||||
va->va_fsid += (uint32_t)f->val[0]; | va->va_fsid += (uint32_t)f->val[0]; | ||||
} | |||||
/* Malloc a zero'd block to compare with the data block read in. */ | |||||
static char *copyfilerange_zerodat = NULL; | |||||
asomers: You can avoid the extra malloc by using the globally defined `zero_region` (see vm/vm_kern.c). | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsI did a simple C version using a u_int *. Maybe it would be more efficient to use rmacklem: I did a simple C version using a u_int *. Maybe it would be more efficient to use
a uint64_t *? | |||||
static u_long copyfilerange_zerosize = 0; | |||||
int | |||||
vn_copy_file_range(struct vnode *invp, off_t *inoffp, struct vnode *outvp, | |||||
off_t *outoffp, size_t *lenp, u_int flags) | |||||
{ | |||||
struct statfs *sfp; | |||||
struct vattr va; | |||||
struct mount *mp; | |||||
u_long blksize; | |||||
int error, xfer; | |||||
ssize_t aresid; | |||||
size_t len; | |||||
char *dat; | |||||
struct thread *td = curthread; | |||||
len = *lenp; | |||||
error = 0; | |||||
mp = NULL; | |||||
/* Do some sanity checks on the arguments. */ | |||||
if (invp->v_type == VDIR || outvp->v_type == VDIR) | |||||
error = EISDIR; | |||||
else if (*inoffp < 0 || (*inoffp + len) < *inoffp || *outoffp < 0 || | |||||
kibUnsubmitted Done Inline Actionsoff_t is signed type, so the overflow is undefined, and you cannot check for it by testing the wrap. We do specify -fwrapv to compiler, but there is a desire to not add new code which depends on undefined behavior, since the fight with compiler will be lost anyway. kib: off_t is signed type, so the overflow is undefined, and you cannot check for it by testing the… | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsIn the code below, I used two uint64_t variables to do the overflow checks. I suppose I could define the variables as uoff_t, but I don't know if that will rmacklem: In the code below, I used two uint64_t variables to do the overflow checks.
I think I got the… | |||||
(*outoffp + len) < *outoffp || invp->v_type != VREG || | |||||
outvp->v_type != VREG) | |||||
asomersUnsubmitted Not Done Inline ActionsYou may want to allow using VCHR. asomers: You may want to allow using `VCHR`. | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsOnly defined for VREG on Linux and I'm not sure why you'd want to do this for VCHR? rmacklem: Only defined for VREG on Linux and I'm not sure why you'd want to do this for VCHR? | |||||
error = EINVAL; | |||||
/* Check that the offset + len does not go past EOF of invp. */ | |||||
if (error == 0) | |||||
error = VOP_GETATTR(invp, &va, curthread->td_ucred); | |||||
if (error == 0 && va.va_size < (*inoffp + len)) | |||||
error = EINVAL; | |||||
Done Inline ActionsThe invp == outvp error case should return EBADF, not EINVAL. asomers: The `invp == outvp` error case should return `EBADF`, not `EINVAL`. | |||||
Done Inline ActionsThe syscall actually got this right and I don't even recall what the correct NFSv4.2 rmacklem: The syscall actually got this right and I don't even recall what the correct NFSv4.2
error is… | |||||
if (error != 0) { | |||||
*lenp = 0; | |||||
return (error); | |||||
} | |||||
/* | |||||
* If the two vnodes are for the same file system, try the | |||||
* VOP_COPY_FILE_RANGE() call first and do it here if the VOP | |||||
* call fails. | |||||
*/ | |||||
if (invp->v_mount == outvp->v_mount) { | |||||
error = VOP_COPY_FILE_RANGE(invp, inoffp, outvp, outoffp, | |||||
lenp, flags); | |||||
if (error == 0) | |||||
return (error); | |||||
} | |||||
/* | |||||
* Copy blocks of the size preferred by the input file, with a | |||||
* minimum of 16Kbytes and a maximum of 1Mbytes. | |||||
*/ | |||||
sfp = malloc(sizeof(*sfp), M_STATFS, M_WAITOK); | |||||
error = VFS_STATFS(invp->v_mount, sfp); | |||||
asomersUnsubmitted Done Inline ActionsYou can avoid the malloc and VFS_STATFS by just using the cached value: invp->v_mount->mnt_stat.f_iosize. asomers: You can avoid the malloc and `VFS_STATFS` by just using the cached value: `invp->v_mount… | |||||
if (error != 0) { | |||||
free(sfp, M_STATFS); | |||||
*lenp = 0; | |||||
return (error); | |||||
} | |||||
if (sfp->f_iosize < 16384) | |||||
blksize = 16384; | |||||
else if (sfp->f_iosize > 1048576) | |||||
blksize = 1048576; | |||||
else | |||||
blksize = sfp->f_iosize; | |||||
kibUnsubmitted Done Inline ActionsI really think you should use the least common multiple of in and out block sizes. kib: I really think you should use the least common multiple of in and out block sizes. | |||||
asomersUnsubmitted Done Inline ActionsMake that the LCM of in, out, and 16384. asomers: Make that the LCM of in, out, and 16384. | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsBasically done. It now uses holesize if it can get that and falls back on f_iosize. rmacklem: Basically done. It now uses holesize if it can get that and falls back on f_iosize.
Assuming… | |||||
free(sfp, M_STATFS); | |||||
/* Start write for outvp. */ | |||||
error = vn_start_write(outvp, &mp, V_WAIT | PCATCH); | |||||
kibUnsubmitted Done Inline ActionsThe wait in vn_start_write() is after the vnode lock in global order. You are introducing the deadlock there. kib: The wait in vn_start_write() is after the vnode lock in global order. You are introducing the… | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsOops, I've made this mistake before. rmacklem: Oops, I've made this mistake before.
| |||||
if (error != 0) { | |||||
*lenp = 0; | |||||
return (error); | |||||
} | |||||
dat = malloc(blksize, M_TEMP, M_WAITOK); | |||||
kibUnsubmitted Done Inline ActionsDo not use malloc(M_WAITOK) while holding vnode lock. Pagedaemon might need to page out a page belonging to the vnode locked by current thread.. kib: Do not use malloc(M_WAITOK) while holding vnode lock. Pagedaemon might need to page out a page… | |||||
/* | |||||
* It would be nice to use VOP_IOCTL() to find holes, but that | |||||
* requires that invp be unlocked/relocked for each block read. | |||||
* I am not sure we want to do that here, since it would open | |||||
* up a window where another thread could write to the file while | |||||
* the copy is in progress. | |||||
* In the meantime, just scan for a read block of all 0s. | |||||
asomersUnsubmitted Done Inline Actionsglibc handles this simply: the fallback is implemented in userspace instead of kernelspace. asomers: glibc handles this simply: the fallback is implemented in userspace instead of kernelspace. | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsYep. Although the Linux man page claims this is done in the kernel to reduce the I think doing it across file systems in the kernel makes sense. I wanted the NFS server to do it across multiple exported file systems, so that it rmacklem: Yep. Although the Linux man page claims this is done in the kernel to reduce the
number of… | |||||
*/ | |||||
if (copyfilerange_zerosize < blksize) { | |||||
free(copyfilerange_zerodat, M_TEMP); | |||||
copyfilerange_zerosize = blksize; | |||||
copyfilerange_zerodat = malloc(copyfilerange_zerosize, M_TEMP, | |||||
M_WAITOK | M_ZERO); | |||||
} | |||||
while (error == 0 && len > 0) { | |||||
if (len > blksize) | |||||
xfer = blksize; | |||||
else | |||||
xfer = len; | |||||
error = vn_rdwr(UIO_READ, invp, dat, xfer, *inoffp, | |||||
kibUnsubmitted Done Inline ActionsEither this or next vn_rdwr() calls are done in the way which skips range locking. The result is that torn writes can be seen. FreeBSD makes the guarantee that each single write(2) syscall effects are either seen as a whole, or not seen at all, by each single read(2) syscall. kib: Either this or next vn_rdwr() calls are done in the way which skips range locking. The result… | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsI'll see what others say about this. rmacklem: I'll see what others say about this.
If they want atomicity, then we need to create non… | |||||
UIO_SYSSPACE, IO_NODELOCKED, td->td_ucred, NULL, &aresid, | |||||
td); | |||||
/* Linux considers a range that exceeds EOF to be an error. */ | |||||
if (error == 0 && aresid > 0) | |||||
error = EINVAL; | |||||
if (error == 0) { | |||||
/* Skip the write for holes. */ | |||||
if (memcmp(dat, copyfilerange_zerodat, xfer) != 0) | |||||
error = vn_rdwr(UIO_WRITE, outvp, dat, xfer, | |||||
*outoffp, UIO_SYSSPACE, IO_NODELOCKED, | |||||
td->td_ucred, NULL, NULL, td); | |||||
else if (xfer == len) { | |||||
/* Hole at EOF. */ | |||||
asomersUnsubmitted Done Inline ActionsThis code looks like it ignores holes altogether. If the destination file contains a non-hole and the source file contains a hole, then you need to either hole-punch the destination or at least fill it with zeros. asomers: This code looks like it ignores holes altogether. If the destination file contains a non-hole… | |||||
rmacklemAuthorUnsubmitted Done Inline ActionsThanks, good catch. I was thinking of/testing with an empty output file. The code now truncates the output file if the range covers the output file rmacklem: Thanks, good catch. I was thinking of/testing with an empty output file.
The code now… | |||||
VATTR_NULL(&va); | |||||
va.va_size = *outoffp + len; | |||||
error = VOP_SETATTR(outvp, &va, td->td_ucred); | |||||
} | |||||
if (error == 0) { | |||||
*inoffp += xfer; | |||||
*outoffp += xfer; | |||||
len -= xfer; | |||||
} | |||||
} | |||||
} | |||||
*lenp -= len; | |||||
if (mp != NULL) | |||||
vn_finished_write(mp); | |||||
free(dat, M_TEMP); | |||||
return (error); | |||||
} | } | ||||
int | int | ||||
vn_fsync_buf(struct vnode *vp, int waitfor) | vn_fsync_buf(struct vnode *vp, int waitfor) | ||||
{ | { | ||||
struct buf *bp, *nbp; | struct buf *bp, *nbp; | ||||
struct bufobj *bo; | struct bufobj *bo; | ||||
struct mount *mp; | struct mount *mp; | ||||
int error, maxretry; | int error, maxretry; | ||||
error = 0; | error = 0; | ||||
maxretry = 10000; /* large, arbitrarily chosen */ | maxretry = 10000; /* large, arbitrarily chosen */ | ||||
mp = NULL; | mp = NULL; | ||||
if (vp->v_type == VCHR) { | if (vp->v_type == VCHR) { | ||||
Done Inline ActionsThis ignores errors that it shouldn't, like EIO. It should return for any error except EOPNOTSUPP. Better yet, the fallback logic should be moved into vop_stdcopy_file_range and that should be the default method for this vop. asomers: This ignores errors that it shouldn't, like EIO. It should return for any error except… | |||||
Done Inline ActionsWell, I am not sure which errors are going to be recoverable by doing vn_rdwr() in a loop. I have no idea what other errors a non-FreeBSD NFSv4.2 server might return which is If you think fuse (or some future local fs that chooses to implement the VOP) will I don't see putting the "fallback" in the VOP, since it is designed to work across I think working across multiple file systems could be a useful feature. rmacklem: Well, I am not sure which errors are going to be recoverable by doing vn_rdwr() in a loop.
One… | |||||
Done Inline ActionsActually any error VOP_COPY_FILE_RANGE() returns gets returned by the syscall. rmacklem: Actually any error VOP_COPY_FILE_RANGE() returns gets returned by the syscall.
The current code… | |||||
Done Inline ActionsThis ignores errors that it shouldn't, like EIO. It should return for any error except EOPNOTSUPP. Better yet, the fallback logic should be moved into vop_stdcopy_file_range and that should be the default method for this vop. asomers: This ignores errors that it shouldn't, like `EIO`. It should return for any error except… | |||||
VI_LOCK(vp); | VI_LOCK(vp); | ||||
mp = vp->v_rdev->si_mountpt; | mp = vp->v_rdev->si_mountpt; | ||||
VI_UNLOCK(vp); | VI_UNLOCK(vp); | ||||
} | } | ||||
bo = &vp->v_bufobj; | bo = &vp->v_bufobj; | ||||
BO_LOCK(bo); | BO_LOCK(bo); | ||||
loop1: | loop1: | ||||
/* | /* | ||||
▲ Show 20 Lines • Show All 74 Lines • Show Last 20 Lines |
You can avoid the extra malloc by using the globally defined zero_region (see vm/vm_kern.c). But comparing against that would still needlessly consume CPU cache. Better to write a mem_iszero helper that compares a single buffer against the literal 0.