Changeset View
Standalone View
sys/geom/geom_io.c
Show First 20 Lines • Show All 44 Lines • ▼ Show 20 Lines | |||||
#include <sys/param.h> | #include <sys/param.h> | ||||
#include <sys/systm.h> | #include <sys/systm.h> | ||||
#include <sys/kernel.h> | #include <sys/kernel.h> | ||||
#include <sys/malloc.h> | #include <sys/malloc.h> | ||||
#include <sys/bio.h> | #include <sys/bio.h> | ||||
#include <sys/ktr.h> | #include <sys/ktr.h> | ||||
#include <sys/proc.h> | #include <sys/proc.h> | ||||
#include <sys/sbuf.h> | #include <sys/sbuf.h> | ||||
#include <sys/sf_buf.h> | |||||
#include <sys/stack.h> | #include <sys/stack.h> | ||||
#include <sys/sysctl.h> | #include <sys/sysctl.h> | ||||
#include <sys/vmem.h> | #include <sys/vmem.h> | ||||
#include <machine/stdarg.h> | #include <machine/stdarg.h> | ||||
#include <sys/errno.h> | #include <sys/errno.h> | ||||
#include <geom/geom.h> | #include <geom/geom.h> | ||||
#include <geom/geom_int.h> | #include <geom/geom_int.h> | ||||
▲ Show 20 Lines • Show All 675 Lines • ▼ Show 20 Lines | |||||
int transient_map_soft_failures; | int transient_map_soft_failures; | ||||
SYSCTL_INT(_kern_geom, OID_AUTO, transient_map_soft_failures, CTLFLAG_RD, | SYSCTL_INT(_kern_geom, OID_AUTO, transient_map_soft_failures, CTLFLAG_RD, | ||||
&transient_map_soft_failures, 0, | &transient_map_soft_failures, 0, | ||||
"Count of retried failures to establish the transient mapping"); | "Count of retried failures to establish the transient mapping"); | ||||
int inflight_transient_maps; | int inflight_transient_maps; | ||||
SYSCTL_INT(_kern_geom, OID_AUTO, inflight_transient_maps, CTLFLAG_RD, | SYSCTL_INT(_kern_geom, OID_AUTO, inflight_transient_maps, CTLFLAG_RD, | ||||
&inflight_transient_maps, 0, | &inflight_transient_maps, 0, | ||||
"Current count of the active transient maps"); | "Current count of the active transient maps"); | ||||
static void | |||||
g_io_bio_copy(struct bio *bp, void *kaddr, bool pinned, bool in) | |||||
kib: Pass uio_rw directly, instead of bool. It would be also much easier to read. | |||||
{ | |||||
size_t dofs = 0; | |||||
size_t dlen = bp->bio_length; | |||||
int i, sf_flags; | |||||
if (pinned) | |||||
sf_flags = SFB_CPUPRIVATE; | |||||
else | |||||
sf_flags = 0; | |||||
for ( i = 0; i < bp->bio_ma_n; i++ ) { | |||||
struct sf_buf *sfb; | |||||
vm_page_t m; | |||||
char *vm; | |||||
size_t l; | |||||
int flags; | |||||
flags = VM_ALLOC_WAITOK | VM_ALLOC_WIRED; | |||||
m = vm_page_grab_unlocked( | |||||
bp->bio_ma[i]->object, | |||||
bp->bio_ma[i]->pindex, | |||||
flags); | |||||
markjUnsubmitted Not Done Inline ActionsDoesn't this just return bp->bio_ma[i]? markj: Doesn't this just return `bp->bio_ma[i]`? | |||||
asomersAuthorUnsubmitted Done Inline ActionsYes, locked and wired as kib and I discussed below. asomers: Yes, locked and wired as kib and I discussed below. | |||||
MPASS(m != NULL); | |||||
sfb = sf_buf_alloc(m, sf_flags); | |||||
MPASS(sfb != NULL); | |||||
Not Done Inline Actionscopyin/copyout seem like confusing names since those words typically mean something else. I would suggest keeping the function private to GELI for now. To support authenticated mode you will need a more general variant of this function. markj: copyin/copyout seem like confusing names since those words typically mean something else.
I… | |||||
vm = (char*)sf_buf_kva(sfb); | |||||
l = MIN(dlen, PAGE_SIZE); | |||||
if (i == 0) { | |||||
vm += bp->bio_ma_offset; | |||||
l -= bp->bio_ma_offset; | |||||
} | |||||
if (in) | |||||
bcopy(vm, (char*)kaddr + dofs, l); | |||||
else | |||||
bcopy((char*)kaddr + dofs, vm, l); | |||||
/* XXX is vm_page_xunbusy really correct ? */ | |||||
asomersAuthorUnsubmitted Done Inline ActionsSomebody please double-check me on this. I'm really not sure of the correct way to undo the effects of the preceding vm_page_grab_unlocked() asomers: Somebody please double-check me on this. I'm really not sure of the correct way to undo the… | |||||
kibUnsubmitted Not Done Inline ActionsYou are doing quite strange thing there. Why not use e.g. uiomove_fromphys() ? kib: You are doing quite strange thing there.
Why not use e.g. uiomove_fromphys() ? | |||||
asomersAuthorUnsubmitted Done Inline ActionsBecause in the email thread markj suggested using sf_buf. I can try uiomove_fromphys, though. asomers: Because in the email thread markj suggested using sf_buf. I can try uiomove_fromphys, though. | |||||
kibUnsubmitted Not Done Inline ActionsUse of sf_bufs would make sense if you do inplace operation. Since you only copy data between buffers, it is much cheaper to uiomove(). Also what you did to get vm_page for an address is not correct. kib: Use of sf_bufs would make sense if you do inplace operation. Since you only copy data between… | |||||
asomersAuthorUnsubmitted Done Inline ActionsThen pray tell, what is the correct way to get vm_page for an address? asomers: Then pray tell, what is the correct way to get vm_page for an address? | |||||
kibUnsubmitted Not Done Inline ActionsWhy do you need to get it for address ? bio_ma[] is already an array of vm_pages. kib: Why do you need to get it for address ? bio_ma[] is already an array of vm_pages. | |||||
asomersAuthorUnsubmitted Done Inline ActionsI needed to wire the pages in order to use sf_buf_alloc. And before I could do that I needed to lock them. vm_page_grab_unlocked does both. asomers: I needed to wire the pages in order to use sf_buf_alloc. And before I could do that I needed… | |||||
kibUnsubmitted Not Done Inline Actionssf_buf_alloc() does not require any of that, it takes just vm_page. Geom (well not geom but callers with unmapped bios) guarantee that bio_ma[] elements are stable and not reused during the whole bio lifetime. kib: sf_buf_alloc() does not require any of that, it takes just vm_page. Geom (well not geom but… | |||||
asomersAuthorUnsubmitted Done Inline ActionsYeah it does. From the man page: sf_buf_alloc() is not responsible for arranging for the page to be present in physical memory; the caller should already have arranged for the page to be wired, i.e., by calling vm_page_wire(9). asomers: Yeah it does. From the man page:
```
sf_buf_alloc() is not responsible for arranging… | |||||
kibUnsubmitted Not Done Inline ActionsThe man page might be not the cleanest, but what it tries to say is that reuse of the page (e.g. by pagedaemon if the page is managed) is not blocked by allocation of sf_buf. Wiring the page might be a solution, but wiring would have to occur not at the time of sf_buf allocation but at the moment the code takes handle of vm_page. This is how bio_ma[] have to be arranged, otherwise io requests would corrupt or leak random memory. That said, this detail is absolutely irrelevant for sf_buf(9), becase vm_page is never recycled, it is only reused for different context. Lets ignore dynamically allocated fictitious vm_pages, they cannot appear nearby geom. kib: The man page might be not the cleanest, but what it tries to say is that reuse of the page (e.g. | |||||
asomersAuthorUnsubmitted Done Inline ActionsI got it working with uiomove_fromphys instead of sf_buf. However, that erased all of the performance gains. My benchmarks are back to where they were with mapped buffers. Can you suggest why that might be? If not, I'll stick with the sf_buf approach (but remove vm_page_grab_unlocked as you suggested). asomers: I got it working with uiomove_fromphys instead of sf_buf. However, that erased all of the… | |||||
markjUnsubmitted Not Done Inline ActionsPlease show the diff. markj: Please show the diff. | |||||
vm_page_xunbusy(m); | |||||
sf_buf_free(sfb); | |||||
dlen -= l; | |||||
dofs += l; | |||||
} | |||||
} | |||||
/* | |||||
* Copy data from a (potentially unmapped) bio to a kernelspace buffer. | |||||
* | |||||
* The buffer must have at least as much room as bp->bio_length. | |||||
* If the thread is pinned to a single CPU, set pinned=true | |||||
*/ | |||||
void | |||||
g_io_bio_copyin(struct bio *bp, void *kaddr, bool pinned) | |||||
{ | |||||
g_io_bio_copy(bp, kaddr, pinned, true); | |||||
} | |||||
/* | |||||
* Copy data from a kernelspace buffer to a (potentially unmapped) bio | |||||
* | |||||
* The buffer must have at least as much room as bp->bio_length. | |||||
* If the thread is pinned to a single CPU, set pinned=true | |||||
*/ | |||||
void | |||||
g_io_bio_copyout(void *kaddr, struct bio *bp, bool pinned) | |||||
{ | |||||
g_io_bio_copy(bp, kaddr, pinned, false); | |||||
} | |||||
static int | static int | ||||
g_io_transient_map_bio(struct bio *bp) | g_io_transient_map_bio(struct bio *bp) | ||||
{ | { | ||||
vm_offset_t addr; | vm_offset_t addr; | ||||
long size; | long size; | ||||
u_int retried; | u_int retried; | ||||
▲ Show 20 Lines • Show All 329 Lines • Show Last 20 Lines |
Pass uio_rw directly, instead of bool. It would be also much easier to read.