ext2fs crash in -current (r218056)

[John Baldwin]

On Tuesday, February 01, 2011 2:42:12 am Doug Barton wrote:
> As I've discussed here previously I am using ext2fs in -current as a 
> general-purpose /home directory, which includes my ports tree, and at 
> the time of the crash my WRKDIRPREFIX. I was doing some heavy ports 
> building to re-create my amd64-current system from scratch when the 
> following crash happened (sorry for the fuzzy photo):
> 
> http://dougbarton.us/ext2fs-crash-dump.jpg
> http://dougbarton.us/ext2fs-crash-dump.txt
> 
> Previous to the recent changes in -current I hadn't been experiencing 
> actual crashes, so I can't help think this is related to some of the 
> changes that John has been shepherding in recently.

All of the changes so far have been cosmetic, not functional.

However, it seems to me that the ext2 code is rife with locking races that 
could explain this, and that this is just one of them.

I can take a look at the locking, but here is an example from this routine of 
how it is wrong:

/*
 * Determine whether an inode can be allocated.
 *
 * Check to see if an inode is available, and if it is,
 * allocate it using tode in the specified cylinder group.
 */
static daddr_t
ext2_nodealloccg(struct inode *ip, int cg, daddr_t ipref, int mode)
{
	struct m_ext2fs *fs;
	struct buf *bp;
	struct ext2mount *ump;
	int error, start, len, loc, map, i;
	char *ibp;
	ipref--; /* to avoid a lot of (ipref -1) */
	if (ipref == -1)
		ipref = 0;
	fs = ip->i_e2fs;
	ump = ip->i_ump;
	if (fs->e2fs_gd[cg].ext2bgd_nifree == 0)
		return (0);
	EXT2_UNLOCK(ump);	
	error = bread(ip->i_devvp, fsbtodb(fs,
		fs->e2fs_gd[cg].ext2bgd_i_bitmap),
		(int)fs->e2fs_bsize, NOCRED, &bp);
	if (error) {
		brelse(bp);
		EXT2_LOCK(ump);
		return (0);
	}
	ibp = (char *)bp->b_data;
	if (ipref) {
		ipref %= fs->e2fs->e2fs_ipg;
		if (isclr(ibp, ipref))
			goto gotit;
	}
	start = ipref / NBBY;
	len = howmany(fs->e2fs->e2fs_ipg - ipref, NBBY);
	loc = skpc(0xff, len, &ibp[start]);
	if (loc == 0) {
		len = start + 1;
		start = 0;
		loc = skpc(0xff, len, &ibp[0]);
		if (loc == 0) {
			printf("cg = %d, ipref = %lld, fs = %s\n",
				cg, (long long)ipref, fs->e2fs_fsmnt);
			panic("ext2fs_nodealloccg: map corrupted");
			/* NOTREACHED */
		}
	} 
	i = start + len - loc;
	map = ibp[i];
	ipref = i * NBBY;
	for (i = 1; i < (1 << NBBY); i <<= 1, ipref++) {
		if ((map & i) == 0) {
			goto gotit;
		}
	}
	printf("fs = %s\n", fs->e2fs_fsmnt);
	panic("ext2fs_nodealloccg: block not in map");
	/* NOTREACHED */
gotit:
	setbit(ibp, ipref);
	EXT2_LOCK(ump);
	fs->e2fs_gd[cg].ext2bgd_nifree--;
	fs->e2fs->e2fs_ficount--;
	fs->e2fs_fmod = 1;
	if ((mode & IFMT) == IFDIR) {
		fs->e2fs_gd[cg].ext2bgd_ndirs++;
		fs->e2fs_total_dir++;
	}
	EXT2_UNLOCK(ump);
	bdwrite(bp);
	return (cg * fs->e2fs->e2fs_ipg + ipref +1);
}

Note that we don't hold any locks while we scan for a free i-node.  Two 
concurrent attempts to allocate an i-node could be running when there is only 
one i-node available.  The first one could then grab that free i-node and the 
second thread would find a full bitmap.

I think the simplest solution is to lock the ump far earlier after the bread() 
returns and to recheck nifree at that time.

It may be that having the buffer locked makes this race occur less often, but 
it could still be the case even then that we would need to recheck nifree 
after the bread() returns.

-- 
John Baldwin