Date: Wed, 11 Jul 2001 00:39:26 +0200 From: Thomas Moestl <tmoestl@gmx.net> To: hackers@FreeBSD.org Cc: Matt Dillon <dillon@FreeBSD.org>, Boris Popov <bp@FreeBSD.org> Subject: Please review: bugfix for vinvalbuf() Message-ID: <20010711003926.B8799@crow.dom2ip.de>
next in thread | raw e-mail | index | archive | help
--QKdGvSO+nmPlgiQ/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Hi,
I've just tripped over an obviously long-standing (since about
Jan. 1998) bug in vinvalbuf while looking into PR kern/26224. The
problematic code looks like (on -CURRENT):
/*
* Destroy the copy in the VM cache, too.
*/
mtx_lock(&vp->v_interlock);
if (VOP_GETVOBJECT(vp, &object) == 0) {
vm_object_page_remove(object, 0, 0,
(flags & V_SAVE) ? TRUE : FALSE);
}
mtx_unlock(&vp->v_interlock);
The locks seem to be needed for file systems that don't perform real
locking (on -STABLE, they are simplelocks).
This, however, is incorrect because vm_object_page_remove may sleep.
I've attached a patch that passes the interlock to
vm_object_page_remove, which in turn passes it to a modified version
of vm_page_sleep, which unlocks it around the sleep.
I think that this is correct, because the object should be in a valid
state when we sleep (and all checks are reexecuted in that case).
Since I'm not very experienced with vfs and vm stuff, I'd be glad if
this patch could get some review. In particular, is the lock/unlock
pair really still needed, and are there notable differeces in -STABLE
(because the fix would need the be MFC'ed)?
Thanks,
- thomas
--QKdGvSO+nmPlgiQ/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="vinvalbuf.diff"
Index: kern/vfs_subr.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/vfs_subr.c,v
retrieving revision 1.315
diff -u -r1.315 vfs_subr.c
--- kern/vfs_subr.c 2001/07/04 16:20:13 1.315
+++ kern/vfs_subr.c 2001/07/10 19:45:41
@@ -800,7 +800,7 @@
mtx_lock(&vp->v_interlock);
if (VOP_GETVOBJECT(vp, &object) == 0) {
vm_object_page_remove(object, 0, 0,
- (flags & V_SAVE) ? TRUE : FALSE);
+ (flags & V_SAVE) ? TRUE : FALSE, &vp->v_interlock);
}
mtx_unlock(&vp->v_interlock);
Index: vm/vm_map.c
===================================================================
RCS file: /home/ncvs/src/sys/vm/vm_map.c,v
retrieving revision 1.206
diff -u -r1.206 vm_map.c
--- vm/vm_map.c 2001/07/04 20:15:16 1.206
+++ vm/vm_map.c 2001/07/10 20:02:41
@@ -1875,7 +1875,7 @@
vm_object_page_remove(object,
OFF_TO_IDX(offset),
OFF_TO_IDX(offset + size + PAGE_MASK),
- FALSE);
+ FALSE, NULL);
}
VOP_UNLOCK(object->handle, 0, curproc);
vm_object_deallocate(object);
@@ -1991,7 +1991,7 @@
offidxend = offidxstart + count;
if ((object == kernel_object) || (object == kmem_object)) {
- vm_object_page_remove(object, offidxstart, offidxend, FALSE);
+ vm_object_page_remove(object, offidxstart, offidxend, FALSE, NULL);
} else {
pmap_remove(map->pmap, s, e);
if (object != NULL &&
@@ -1999,7 +1999,7 @@
(object->flags & (OBJ_NOSPLIT|OBJ_ONEMAPPING)) == OBJ_ONEMAPPING &&
(object->type == OBJT_DEFAULT || object->type == OBJT_SWAP)) {
vm_object_collapse(object);
- vm_object_page_remove(object, offidxstart, offidxend, FALSE);
+ vm_object_page_remove(object, offidxstart, offidxend, FALSE, NULL);
if (object->type == OBJT_SWAP) {
swap_pager_freespace(object, offidxstart, count);
}
@@ -2994,7 +2994,7 @@
/*
* Remove unneeded old pages
*/
- vm_object_page_remove(first_object, 0, 0, 0);
+ vm_object_page_remove(first_object, 0, 0, 0, NULL);
/*
* Invalidate swap space
Index: vm/vm_object.c
===================================================================
RCS file: /home/ncvs/src/sys/vm/vm_object.c,v
retrieving revision 1.196
diff -u -r1.196 vm_object.c
--- vm/vm_object.c 2001/07/04 20:15:16 1.196
+++ vm/vm_object.c 2001/07/10 20:03:23
@@ -1438,7 +1438,7 @@
* The object must be locked.
*/
void
-vm_object_page_remove(vm_object_t object, vm_pindex_t start, vm_pindex_t end, boolean_t clean_only)
+vm_object_page_remove(vm_object_t object, vm_pindex_t start, vm_pindex_t end, boolean_t clean_only, struct mtx *interlk)
{
vm_page_t p, next;
unsigned int size;
@@ -1478,7 +1478,7 @@
* interrupt -- minimize the spl transitions
*/
- if (vm_page_sleep_busy(p, TRUE, "vmopar"))
+ if (vm_page_sleep_busy_lk(p, TRUE, "vmopar", interlk))
goto again;
if (clean_only && p->valid) {
@@ -1509,7 +1509,7 @@
* The busy flags are only cleared at
* interrupt -- minimize the spl transitions
*/
- if (vm_page_sleep_busy(p, TRUE, "vmopar"))
+ if (vm_page_sleep_busy_lk(p, TRUE, "vmopar", interlk))
goto again;
if (clean_only && p->valid) {
@@ -1601,7 +1601,7 @@
if (next_pindex < prev_object->size) {
vm_object_page_remove(prev_object,
next_pindex,
- next_pindex + next_size, FALSE);
+ next_pindex + next_size, FALSE, NULL);
if (prev_object->type == OBJT_SWAP)
swap_pager_freespace(prev_object,
next_pindex, next_size);
Index: vm/vm_object.h
===================================================================
RCS file: /home/ncvs/src/sys/vm/vm_object.h,v
retrieving revision 1.73
diff -u -r1.73 vm_object.h
--- vm/vm_object.h 2001/07/04 20:15:16 1.73
+++ vm/vm_object.h 2001/07/10 19:52:18
@@ -187,7 +187,7 @@
void vm_object_vndeallocate (vm_object_t);
void vm_object_init (void);
void vm_object_page_clean (vm_object_t, vm_pindex_t, vm_pindex_t, boolean_t);
-void vm_object_page_remove (vm_object_t, vm_pindex_t, vm_pindex_t, boolean_t);
+void vm_object_page_remove (vm_object_t, vm_pindex_t, vm_pindex_t, boolean_t, struct mtx *);
void vm_object_pmap_copy (vm_object_t, vm_pindex_t, vm_pindex_t);
void vm_object_pmap_copy_1 (vm_object_t, vm_pindex_t, vm_pindex_t);
void vm_object_pmap_remove (vm_object_t, vm_pindex_t, vm_pindex_t);
Index: vm/vm_page.c
===================================================================
RCS file: /home/ncvs/src/sys/vm/vm_page.c,v
retrieving revision 1.168
diff -u -r1.168 vm_page.c
--- vm/vm_page.c 2001/07/04 23:27:08 1.168
+++ vm/vm_page.c 2001/07/10 20:03:55
@@ -508,7 +508,7 @@
*/
int
-vm_page_sleep_busy(vm_page_t m, int also_m_busy, const char *msg)
+vm_page_sleep_busy_lk(vm_page_t m, int also_m_busy, const char *msg, struct mtx *interlk)
{
GIANT_REQUIRED;
if ((m->flags & PG_BUSY) || (also_m_busy && m->busy)) {
@@ -518,7 +518,11 @@
* Page is busy. Wait and retry.
*/
vm_page_flag_set(m, PG_WANTED | PG_REFERENCED);
+ if (interlk != NULL)
+ mtx_unlock(interlk);
tsleep(m, PVM, msg, 0);
+ if (interlk != NULL)
+ mtx_lock(interlk);
}
splx(s);
return(TRUE);
Index: vm/vm_page.h
===================================================================
RCS file: /home/ncvs/src/sys/vm/vm_page.h,v
retrieving revision 1.89
diff -u -r1.89 vm_page.h
--- vm/vm_page.h 2001/07/04 23:27:09 1.89
+++ vm/vm_page.h 2001/07/10 19:59:42
@@ -340,7 +340,9 @@
void vm_page_copy(vm_page_t src_m, vm_page_t dest_m);
void vm_page_free(vm_page_t m);
void vm_page_free_zero(vm_page_t m);
-int vm_page_sleep_busy(vm_page_t m, int also_m_busy, const char *msg);
+#define vm_page_sleep_busy(m, also_m_busy, msg) \
+ vm_page_sleep_busy_lk(m, also_m_busy, msg, NULL)
+int vm_page_sleep_busy_lk(vm_page_t m, int also_m_busy, const char *msg, struct mtx *);
void vm_page_dirty(vm_page_t m);
void vm_page_undirty(vm_page_t m);
vm_page_t vm_page_list_find(int basequeue, int index, boolean_t prefer_zero);
Index: vm/vnode_pager.c
===================================================================
RCS file: /home/ncvs/src/sys/vm/vnode_pager.c,v
retrieving revision 1.134
diff -u -r1.134 vnode_pager.c
--- vm/vnode_pager.c 2001/07/04 19:00:13 1.134
+++ vm/vnode_pager.c 2001/07/10 19:57:35
@@ -294,7 +294,7 @@
vm_freeze_copyopts(object, OFF_TO_IDX(nsize), object->size);
if (nobjsize < object->size) {
vm_object_page_remove(object, nobjsize, object->size,
- FALSE);
+ FALSE, NULL);
}
/*
* this gets rid of garbage at the end of a page that is now
--QKdGvSO+nmPlgiQ/--
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010711003926.B8799>