On Sat, Jun 12, 2004 at 04:08:38AM +0200, Andi Kleen wrote:
>
> Hallo,
Hi there Andi,
>
> My theory is that one of the ENOSPC error paths forgets to brelse a buffer
> which
> causes the semaphore to get lost and causes the deadlocks later. This is
> likely
> a AGF buffer, since xfs_alloc_read_agf() and children are blocking too.
Yep, makes sense.
> While looking at the code I found some missing brelses in xfs_alloc errors
> paths that could explain some of the problems. With this patch applied
After a quick look, I found a few more too. I'll do a full review.
We need to widen the scope from just AGF buffers though (noticed at
least one AGI leak on another uncommon path).
> I unfortunately get still deadlocks with the same test case eventually
> (lost pagebuf sem again) but they take soemwhat longer.
OK.
> Most likely more audit of error paths is needed (i wish there was an automatic
> tool for that).
Yeah, that'd be nice. One thing that may help - any missing
buffer relse for code that also exists in userspace libxfs.a
(which is alot) will result in a memory leak, for which there
are several tools to check. Not sure valgrind ever was made
to work for us... (IIRC Eric has a patch?) ...due to our use
of ustat, ioctl, and several other outlandish syscalls.
Thats only going to be picked up if we go down each code path
in userspace too of course - a static source code checker is
likely to produce much better results than this.
>
> diff -u linux/fs/xfs/xfs_alloc.c-o linux/fs/xfs/xfs_alloc.c
> --- linux/fs/xfs/xfs_alloc.c-o 1970-01-01 01:12:51.000000000 +0100
> +++ linux/fs/xfs/xfs_alloc.c 2004-06-12 03:54:18.000000000 +0200
> ...
> @@ -1886,8 +1887,7 @@
> (int)(pag->pagf_freeblks + pag->pagf_flcount -
> need - args->total) <
> (int)args->minleft)) {
> - if (agbp)
> - xfs_trans_brelse(tp, agbp);
> + xfs_trans_brelse(tp, agbp);
> args->agbp = NULL;
> return 0;
> }
> ...
> diff -u linux/fs/xfs/xfs_trans_buf.c-o linux/fs/xfs/xfs_trans_buf.c
> --- linux/fs/xfs/xfs_trans_buf.c-o 2004-06-11 16:32:53.000000000 +0200
> +++ linux/fs/xfs/xfs_trans_buf.c 2004-06-12 03:45:11.000000000 +0200
> @@ -521,6 +521,9 @@
> xfs_log_item_t *lip;
> xfs_log_item_desc_t *lidp;
>
> + if (!bp)
> + return;
> +
Not sure I like this approach though - its inconsistent with
the other callers into here, who have their own local knowledge
of whether NULL is acceptable or not.
I'll rejig this, and spend some time reviewing all of the
other code paths to see what else I can turn up.
thanks Andi.
--
Nathan
|