Changeset View
Standalone View
sys/kern/vfs_bio.c
Show First 20 Lines • Show All 2,444 Lines • ▼ Show 20 Lines | |||||
* | * | ||||
* Release a busy buffer and, if requested, free its resources. The | * Release a busy buffer and, if requested, free its resources. The | ||||
* buffer will be stashed in the appropriate bufqueue[] allowing it | * buffer will be stashed in the appropriate bufqueue[] allowing it | ||||
* to be accessed later as a cache entity or reused for other purposes. | * to be accessed later as a cache entity or reused for other purposes. | ||||
*/ | */ | ||||
void | void | ||||
brelse(struct buf *bp) | brelse(struct buf *bp) | ||||
{ | { | ||||
struct mount *v_mnt; | |||||
int qindex; | int qindex; | ||||
/* | /* | ||||
* Many functions erroneously call brelse with a NULL bp under rare | * Many functions erroneously call brelse with a NULL bp under rare | ||||
* error conditions. Simply return when called with a NULL bp. | * error conditions. Simply return when called with a NULL bp. | ||||
*/ | */ | ||||
if (bp == NULL) | if (bp == NULL) | ||||
return; | return; | ||||
CTR3(KTR_BUF, "brelse(%p) vp %p flags %X", | CTR3(KTR_BUF, "brelse(%p) vp %p flags %X", | ||||
bp, bp->b_vp, bp->b_flags); | bp, bp->b_vp, bp->b_flags); | ||||
KASSERT(!(bp->b_flags & (B_CLUSTER|B_PAGING)), | KASSERT(!(bp->b_flags & (B_CLUSTER|B_PAGING)), | ||||
("brelse: inappropriate B_PAGING or B_CLUSTER bp %p", bp)); | ("brelse: inappropriate B_PAGING or B_CLUSTER bp %p", bp)); | ||||
KASSERT((bp->b_flags & B_VMIO) != 0 || (bp->b_flags & B_NOREUSE) == 0, | KASSERT((bp->b_flags & B_VMIO) != 0 || (bp->b_flags & B_NOREUSE) == 0, | ||||
("brelse: non-VMIO buffer marked NOREUSE")); | ("brelse: non-VMIO buffer marked NOREUSE")); | ||||
v_mnt = bp->b_vp != NULL ? bp->b_vp->v_mount : NULL; | |||||
kib: There is no need to calculate v_mount that early. In fact it might be best to do (v_mnt = (bp… | |||||
darrick.freebsd_gmail.comAuthorUnsubmitted Not Done Inline ActionsThat if-statement is already pretty complex, do we want to introduce an assignment side-effect to it? I will shift the !() as you suggest. darrick.freebsd_gmail.com: That if-statement is already pretty complex, do we want to introduce an assignment side-effect… | |||||
kibUnsubmitted Not Done Inline ActionsDo as you prefer, my main point was that the calculation of the v_mnt is too early, there are returns where the value is simply unused. kib: Do as you prefer, my main point was that the calculation of the v_mnt is too early, there are… | |||||
if (BUF_LOCKRECURSED(bp)) { | if (BUF_LOCKRECURSED(bp)) { | ||||
/* | /* | ||||
* Do not process, in particular, do not handle the | * Do not process, in particular, do not handle the | ||||
* B_INVAL/B_RELBUF and do not release to free list. | * B_INVAL/B_RELBUF and do not release to free list. | ||||
*/ | */ | ||||
BUF_UNLOCK(bp); | BUF_UNLOCK(bp); | ||||
return; | return; | ||||
} | } | ||||
▲ Show 20 Lines • Show All 80 Lines • ▼ Show 20 Lines | brelse(struct buf *bp) | ||||
* Normally we can do this whether a buffer is B_DELWRI or not. If | * Normally we can do this whether a buffer is B_DELWRI or not. If | ||||
* the buffer is an NFS buffer, it is tracking piecemeal writes or | * the buffer is an NFS buffer, it is tracking piecemeal writes or | ||||
* the commit state and we cannot afford to lose the buffer. If the | * the commit state and we cannot afford to lose the buffer. If the | ||||
* buffer has a background write in progress, we need to keep it | * buffer has a background write in progress, we need to keep it | ||||
* around to prevent it from being reconstituted and starting a second | * around to prevent it from being reconstituted and starting a second | ||||
* background write. | * background write. | ||||
*/ | */ | ||||
if ((bp->b_flags & B_VMIO) && (bp->b_flags & B_NOCACHE || | if ((bp->b_flags & B_VMIO) && (bp->b_flags & B_NOCACHE || | ||||
(bp->b_ioflags & BIO_ERROR && bp->b_iocmd == BIO_READ)) && | (bp->b_ioflags & BIO_ERROR && bp->b_iocmd == BIO_READ)) && | ||||
!(bp->b_vp->v_mount != NULL && | !(v_mnt != NULL && (v_mnt->mnt_vfc->vfc_flags & VFCF_NETWORK) != 0 && | ||||
Not Done Inline ActionsIs it actually valid for b_vp to be NULL here? I suspect Coverity may not understand the buffer lifecycle fully. If it is valid, can we come up with a testcase that exercises it? Assuming it's valid, I think it might make sense to pull this (sub)conditional out into a subroutine at this point. This if-statement is huge and unwieldy already. bool vn_is_vfcfc_network(const struct vnode *vp) { return (vp != NULL && vp->v_mount != NULL && vp->v_mount->mnt_vfc->vfc_flags & VFCF_NETWORK != 0); } cem: Is it actually valid for b_vp to be NULL here? I suspect Coverity may not understand the… | |||||
Not Done Inline ActionsI'm not 100% sure whether it's possible. I'd assume it doesn't happen often. If it happened with any degree of frequency then the kernel would panic. darrick.freebsd_gmail.com: I'm not 100% sure whether it's possible. I'd assume it doesn't happen often. If it happened… | |||||
Not Done Inline ActionsThe path that explicitly releases bp->b_vp and sets it to NULL (call to be brelvp) is only called when B_VMIO is not set. The check that could potentially derefer that NULL bp->b_vp pointer is only reached when B_VMIO is set. It may be possible for bp->b_vp to be NULL when brelse() is called, but I'm not sure what those circumstances would be. darrick.freebsd_gmail.com: The path that explicitly releases bp->b_vp and sets it to NULL (call to be brelvp) is only… | |||||
Not Done Inline ActionsRight, VMIO buffers cannot exist with b_vp not assigned, they must always belong to the correct vnode. I prefer the buggy code manifests itself with the NULL deref and panic. Nonetheless, there is one thing to improve in the condition. Note that the bp->b_vp->v_mount is dereferenced twice. First it is done to check for non-NULL, and second time to dereference more and test the flag. Theoretically, locked buffer must prevent the vnode reclamation and v_mount going from non-NULL to NULL, but practically I think that this might be violated sometimes, esp. for devfs. I suggest to create a local struct mount * variable where to fetch the v_mount value once and then use it for check. kib: Right, VMIO buffers cannot exist with b_vp not assigned, they must always belong to the correct… | |||||
Not Done Inline ActionsI can create the local struct mount *. Should we assert b_vp != NULL or not B_VMIO? darrick.freebsd_gmail.com: I can create the local struct mount *.
Should we assert b_vp != NULL or not B_VMIO? | |||||
Not Done Inline Actionsb_vp != NULL is checked by hardware, in robust manner. kib: b_vp != NULL is checked by hardware, in robust manner.
I do not see a need to add asserts. | |||||
(bp->b_vp->v_mount->mnt_vfc->vfc_flags & VFCF_NETWORK) != 0 && | |||||
!vn_isdisk(bp->b_vp, NULL) && (bp->b_flags & B_DELWRI))) { | !vn_isdisk(bp->b_vp, NULL) && (bp->b_flags & B_DELWRI))) { | ||||
vfs_vmio_invalidate(bp); | vfs_vmio_invalidate(bp); | ||||
allocbuf(bp, 0); | allocbuf(bp, 0); | ||||
} | } | ||||
if ((bp->b_flags & (B_INVAL | B_RELBUF)) != 0 || | if ((bp->b_flags & (B_INVAL | B_RELBUF)) != 0 || | ||||
(bp->b_flags & (B_DELWRI | B_NOREUSE)) == B_NOREUSE) { | (bp->b_flags & (B_DELWRI | B_NOREUSE)) == B_NOREUSE) { | ||||
allocbuf(bp, 0); | allocbuf(bp, 0); | ||||
▲ Show 20 Lines • Show All 2,397 Lines • ▼ Show 20 Lines | vfs_bio_getpages(struct vnode *vp, vm_page_t *ma, int count, | ||||
daddr_t lbn, lbnp; | daddr_t lbn, lbnp; | ||||
vm_ooffset_t la, lb, poff, poffe; | vm_ooffset_t la, lb, poff, poffe; | ||||
long bsize; | long bsize; | ||||
int bo_bs, br_flags, error, i, pgsin, pgsin_a, pgsin_b; | int bo_bs, br_flags, error, i, pgsin, pgsin_a, pgsin_b; | ||||
bool redo, lpart; | bool redo, lpart; | ||||
object = vp->v_object; | object = vp->v_object; | ||||
mp = vp->v_mount; | mp = vp->v_mount; | ||||
error = 0; | |||||
Not Done Inline ActionsVOP_GETPAGES must be called with at least one page invalid, and since we downgrade busy state of the page from exclusive to shared without race, bread_gb() must be called at least once. This would initialize error. But I do not object against this formal change. kib: VOP_GETPAGES must be called with at least one page invalid, and since we downgrade busy state… | |||||
la = IDX_TO_OFF(ma[count - 1]->pindex); | la = IDX_TO_OFF(ma[count - 1]->pindex); | ||||
if (la >= object->un_pager.vnp.vnp_size) | if (la >= object->un_pager.vnp.vnp_size) | ||||
return (VM_PAGER_BAD); | return (VM_PAGER_BAD); | ||||
/* | /* | ||||
* Change the meaning of la from where the last requested page starts | * Change the meaning of la from where the last requested page starts | ||||
* to where it ends, because that's the end of the requested region | * to where it ends, because that's the end of the requested region | ||||
* and the start of the potential read-ahead region. | * and the start of the potential read-ahead region. | ||||
▲ Show 20 Lines • Show All 284 Lines • Show Last 20 Lines |
There is no need to calculate v_mount that early. In fact it might be best to do (v_mnt = (bp->b_vp->v_mount) != NULL inside the if ().
Also, I would suggest to take the opportunity and revert the conditions together with the binary operations inside the statement. I.e. remove the '!' before the () and pass it down, replacing && by ||.