Re: CVS commit: src/sys/kern
On 11.11.2016 18:10, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Fri Nov 11 17:10:04 UTC 2016 > > Modified Files: > src/sys/kern: sys_ptrace_common.c > > Log Message: > kern/51621: When attaching to a child send it a SIGTRAP not a SIGSTOP like > Linux and FreeBSD do. > Thank you for looking at it! I noted that it doesn't work for me. attach1 still hangs and there is regression in attach2 and attach3 in the t_ptrace_wait family. $ atf-run t_ptrace_wait6|atf-report Tests root: /usr/tests/kernel t_ptrace_wait6 (1/1): 7 test cases attach1: sorry, pid 21393 was killed: orphaned traced process [0.002638s] Failed: /usr/src/tests/kernel/t_ptrace_wait.c:636: rv == sizeof(msg) not met attach2: sorry, pid 17474 was killed: orphaned traced process [0.001444s] Failed: /usr/src/tests/kernel/t_ptrace_wait.c:776: rv == sizeof(msg) not met attach3: sorry, pid 2725 was killed: orphaned traced process [301.145040s] Failed: Test case timed out after 300 seconds traceme1: [0.001639s] Passed. traceme2: [0.001406s] Passed. traceme3: [0.001359s] Passed. traceme4: [0.001391s] Passed. [301.155536s] Failed test cases: t_ptrace_wait6:attach1, t_ptrace_wait6:attach2, t_ptrace_wait6:attach3 Summary for 1 test programs: 4 passed test cases. 3 failed test cases. 0 expected failed test cases. 0 skipped test cases. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys
2016-11-11 11:52 GMT+01:00 J. Hannken-Illjes :
> Why do we need a TAILQ here, the number of deletions after the first
> element is nearly zero and the list is quite short.
The element would be somewhere towards the end of the list, but likely
not the very last one. Yes, the list is usually short (IIRC the limit
is around 60 per 1MB of log / 1GB of filesystem), so it might not
matter much if we have the error path O(n).
> +wapbl_deallocation_free(struct wapbl *wl, struct wapbl_dealloc *wd,
> + bool locked)
>
> Better to let the caller always take/release the lock.
I wanted to let the pool_put() be called without the lock, but this
might be not very useful optimisation.
> Returning a pointer to an arbitrary list element and using it
> later is bad design. Would be better to define as:
>
> wapbl_unregister_deallocation(struct wapbl *wl, daddr_t blk, int len)
> {
I simply want to have it O(1). I think it is useful to keep the error
path there fast, as it would be quite common for wapbl case. Also just
passing the (opaque) pointer makes it simpler.
Thanks for the BAP_ASSIGN() fix. I started investigating only to later
notice you already committed fix.
What you think about removing the NULL initialization, to let compiler
trigger warning there? Seems modern gcc is intelligent enough to
correctly trigger warning, even with the conditional assignment. Of
course it might be not very useful, as hopefully this code won't be
touched again for a long time.
--- ffs_inode.c 11 Nov 2016 10:50:16 - 1.123
+++ ffs_inode.c 11 Nov 2016 20:47:04 -
@@ -583,8 +583,8 @@ ffs_indirtrunc(struct inode *ip, daddr_t
int i;
struct buf *bp;
struct fs *fs = ip->i_fs;
- int32_t *bap1 = NULL;
- int64_t *bap2 = NULL;
+ int32_t *bap1;
+ int64_t *bap2;
struct vnode *vp;
daddr_t nb, nlbn, last;
char *copy = NULL;
Jaromir
Re: CVS commit: src/sys
On 11.11.2016 11:52, J. Hannken-Illjes wrote:
>
>> On 10 Nov 2016, at 21:56, Jaromir Dolecek wrote:
>>
>> Module Name: src
>> Committed By:jdolecek
>> Date:Thu Nov 10 20:56:32 UTC 2016
>>
>> Modified Files:
>> src/sys/kern: vfs_wapbl.c
>> src/sys/sys: wapbl.h
>> src/sys/ufs/ffs: ffs_inode.c ffs_wapbl.c
>> src/sys/ufs/ufs: ufs_wapbl.h
>>
>> Log Message:
>> during truncate with wapbl, register deallocation for upper indirect block
>> before recursing into lower blocks, to make sure that it will be removed
>> after
>> all its referenced blocks are removed
>>
>> fixes 'ffs_blkfree_common: freeing free block' panic triggered by
>> ufs_truncate_retry() when just the upper indirect block registration failed,
>> code tried to free the lower blocks again after wapbl flush
>>
>> problem found by hannken@, thank you
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.85 -r1.86 src/sys/kern/vfs_wapbl.c
>
> - SIMPLEQ_HEAD(, wapbl_dealloc) wl_dealloclist; /* lm: list head */
> + TAILQ_HEAD(, wapbl_dealloc) wl_dealloclist; /* lm: list head */
>
> This should have been committed on its own. It makes reading the diff
> quite difficult.
>
> Why do we need a TAILQ here, the number of deletions after the first
> element is nearly zero and the list is quite short.
>
>
> +wapbl_deallocation_free(struct wapbl *wl, struct wapbl_dealloc *wd,
> + bool locked)
>
> Better to let the caller always take/release the lock.
>
>
> +wapbl_register_deallocation(struct wapbl *wl, daddr_t blk, int len, bool
> force,
> +void **cookiep)
>
> + if (cookiep)
> + *cookiep = wd;
>
> +wapbl_unregister_deallocation(struct wapbl *wl, void *cookie)
>
> Returning a pointer to an arbitrary list element and using it
> later is bad design. Would be better to define as:
>
> wapbl_unregister_deallocation(struct wapbl *wl, daddr_t blk, int len)
> {
> struct wapbl_dealloc *wd;
>
> mutex_enter(&wl->wl_mtx);
> TAILQ_FOREACH(wd, &wl->wl_dealloclist, wd_entries) {
> if (wd->wd_blkno == blk && wd->wd_len == len)
> break;
> }
> KASSERT(wd != NULL);
> wapbl_deallocation_free(wl, wd);
> mutex_exit(&wl->wl_mtx);
> }
>
>> cvs rdiff -u -r1.19 -r1.20 src/sys/sys/wapbl.h
>> cvs rdiff -u -r1.121 -r1.122 src/sys/ufs/ffs/ffs_inode.c
>> cvs rdiff -u -r1.35 -r1.36 src/sys/ufs/ffs/ffs_wapbl.c
>> cvs rdiff -u -r1.12 -r1.13 src/sys/ufs/ufs/ufs_wapbl.h
>
> --
> J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)
>
I don't know what exact commit was that, but tests panic i386 now.
http://releng.netbsd.org/b5reports/i386/commits-2016.11.html#end
e.g.
http://releng.netbsd.org/b5reports/i386/build/2016.11.10.23.47.23/test.log
Part of the log.
sbin/resize_ffs/t_shrink (457/676): 4 test cases
shrink_24M_16M_v0_32768: uvm_fault(0xc1890e20, 0x1000, 2) -> 0xe
fatal page fault in supervisor mode
trap type 6 code 2 eip c084150b cs 8 eflags 202 cr2 1fa0 ilevel 0 esp 4000
curlwp 0xc19c7540 pid 4299 lid 1 lowest kstack 0xc4e862c0
panic: trap
cpu0: Begin traceback...
vpanic(c0f3e4a6,c4e88b34,c4e88bb0,c01167d3,c0f3e4a6,c4e88bbc,c4e88bbc,1,c4e862c0,202)
at netbsd:vpanic+0x121
snprintf(c0f3e4a6,c4e88bbc,c4e88bbc,1,c4e862c0,202,1fa0,0,4000,0) at
netbsd:snprintf
trap() at netbsd:trap+0xca4
--- trap (number 6) ---
ffs_indirtrunc(21fac0,0,3f3,0,0,c4e88d20,c014826d,0,400,0) at
netbsd:ffs_indirtrunc+0x782
ffs_truncate(c1a644c4,100,0,0,c16f1a80,c01515c0,c186f008,0,c4e88ea4,0)
at netbsd:ffs_truncate+0xa5b
ufs_truncate_retry(c1a644c4,100,0,c16f1a80,0,c1a64568,c1a644c4,c1a644c4,c1a644c4,c4e88e60)
at netbsd:ufs_truncate_retry+0x95
ufs_setattr(c4e88e78,3,c4e88f68,c0ed0be4,c1a644c4,c4e88ea4,c16f1a80,c4e88ea4,c4e88f38,c0974066)
at netbsd:ufs_setattr+0x4bc
VOP_SETATTR(c1a644c4,c4e88ea4,c16f1a80,c1880a80,0,,,,,)
at netbsd:VOP_SETATTR+0x32
sys_ftruncate(c19c7540,c4e88f68,c4e88f60,c1890e20,1ba8000,c4e88f60,c4e88f68,c9,0,0)
at netbsd:sys_ftruncate+0xd4
syscall() at netbsd:syscall+0x13c
--- syscall (number 201) ---
b1eeeb07:
cpu0: End traceback...
dumping to dev 0,1 offset 2240
dump uvm_fault(0xc12c0400, 0xc4de6000, 1) -> 0xe
fatal page fault in supervisor mode
trap type 6 code 0 eip c011125c cs 8 eflags 246 cr2 c4de6dc0 ilevel 8
esp c1282bc0
curlwp 0xc19c7540 pid 4299 lid 1 lowest kstack 0xc4e862c0
Skipping crash dump on recursive panic
panic: trap
cpu0: Begin traceback...
vpanic(c0f3e4a6,c4e889c4,c4e88a40,c01167d3,c0f3e4a6,c4e88a4c,c4e88a4c,1,c4e862c0,246)
at netbsd:vpanic+0x121
snprintf(c0f3e4a6,c4e88a4c,c4e88a4c,1,c4e862c0,246,c4de6dc0,8,c1282bc0,0)
at netbsd:snprintf
trap() at netbsd:trap+0xca4
--- trap (number 6) ---
dodumpsys(c4e88b34,0,104,c011406a,8,c0fcc349,0,104,c0f3e4a6,c4e88b34) at
netbsd:dodumpsys+0x343
dumpsys(104,0,c0f3e4a6,c4e88b34,c19c7540,6,c4e88bbc,c4e88b28,c09207aa,c0f3e4a6)
at netbsd:dumpsys+0x14
vpanic(c0f3e4a6,c4e88b3
Re: CVS commit: src/sys
> On 10 Nov 2016, at 21:56, Jaromir Dolecek wrote:
>
> Module Name: src
> Committed By: jdolecek
> Date: Thu Nov 10 20:56:32 UTC 2016
>
> Modified Files:
> src/sys/kern: vfs_wapbl.c
> src/sys/sys: wapbl.h
> src/sys/ufs/ffs: ffs_inode.c ffs_wapbl.c
> src/sys/ufs/ufs: ufs_wapbl.h
>
> Log Message:
> during truncate with wapbl, register deallocation for upper indirect block
> before recursing into lower blocks, to make sure that it will be removed after
> all its referenced blocks are removed
>
> fixes 'ffs_blkfree_common: freeing free block' panic triggered by
> ufs_truncate_retry() when just the upper indirect block registration failed,
> code tried to free the lower blocks again after wapbl flush
>
> problem found by hannken@, thank you
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.85 -r1.86 src/sys/kern/vfs_wapbl.c
- SIMPLEQ_HEAD(, wapbl_dealloc) wl_dealloclist; /* lm: list head */
+ TAILQ_HEAD(, wapbl_dealloc) wl_dealloclist; /* lm: list head */
This should have been committed on its own. It makes reading the diff
quite difficult.
Why do we need a TAILQ here, the number of deletions after the first
element is nearly zero and the list is quite short.
+wapbl_deallocation_free(struct wapbl *wl, struct wapbl_dealloc *wd,
+ bool locked)
Better to let the caller always take/release the lock.
+wapbl_register_deallocation(struct wapbl *wl, daddr_t blk, int len, bool force,
+void **cookiep)
+ if (cookiep)
+ *cookiep = wd;
+wapbl_unregister_deallocation(struct wapbl *wl, void *cookie)
Returning a pointer to an arbitrary list element and using it
later is bad design. Would be better to define as:
wapbl_unregister_deallocation(struct wapbl *wl, daddr_t blk, int len)
{
struct wapbl_dealloc *wd;
mutex_enter(&wl->wl_mtx);
TAILQ_FOREACH(wd, &wl->wl_dealloclist, wd_entries) {
if (wd->wd_blkno == blk && wd->wd_len == len)
break;
}
KASSERT(wd != NULL);
wapbl_deallocation_free(wl, wd);
mutex_exit(&wl->wl_mtx);
}
> cvs rdiff -u -r1.19 -r1.20 src/sys/sys/wapbl.h
> cvs rdiff -u -r1.121 -r1.122 src/sys/ufs/ffs/ffs_inode.c
> cvs rdiff -u -r1.35 -r1.36 src/sys/ufs/ffs/ffs_wapbl.c
> cvs rdiff -u -r1.12 -r1.13 src/sys/ufs/ufs/ufs_wapbl.h
--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)
